Skip to content

FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404 #7409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 8, 2016

Conversation

OceanWolf
Copy link
Member

@OceanWolf OceanWolf commented Nov 5, 2016

A quick fix to close the bug, but we probably need to come up with a better PR strategy for this.

@fariza Any other ideas/alternatives to this patch? IIRC we compromised on this before saying that toolbar == None should not prevent the tools from working with shortcut keys, just that the toolbar would not get displayed.

Of course the main MEP27 PR fixes this straight away as we decided the new toolmanager should only work with the new figuremanager code, so this got taken care over there yonks ago.

@OceanWolf OceanWolf added this to the 2.0.1 (next bug fix release) milestone Nov 5, 2016
@fariza
Copy link
Member

fariza commented Nov 5, 2016

This is the easiest fix.
I would prefer to change the warning to use our internal warnings that we can disable by config.
I'll give it a try this afternoon to see how doable it is.

@OceanWolf
Copy link
Member Author

Internal warnings? Great, I was thinking about that yesterday that we should do something like:

def mpl_warn(message, warning_type):
  if warning_type in std_warn_list:
    warnings.warn(message)

Does our internal warnings do something like that?

And yes this fix brings it on par with how the MEP27 branch works now (i.e. prior to the removal of the FigureManagerX classes), with the exception that in the MEP27 branch the user can still create an MEP22 enabled FigureManager without a toolmanager by having toolbar set to None and explicitly making the new FigureManager class with:

fig = Figure()
figmgr = FigureManager(fig, title)

@fariza
Copy link
Member

fariza commented Nov 5, 2016

Maybe for simplicity, we should just use your fix, and with MEP27 we can fix the warnings handling

@OceanWolf
Copy link
Member Author

Well, with MEP27 too late to fix warning handling as we will have hopefully worked out all the experimental nature of this by then... with all backends working.

I think we should put the warnings in as a separate PR as they don't really relate to MEP27 specifically, nor anything else.

@fariza
Copy link
Member

fariza commented Nov 5, 2016

ok, we fix this with MEP27 and the warnings in a separate PR

@tacaswell tacaswell changed the title FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404 [MRG+1] FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404 Nov 8, 2016
@tacaswell
Copy link
Member

Please remember to backport

@NelleV NelleV merged commit ba019f7 into matplotlib:master Nov 8, 2016
@OceanWolf
Copy link
Member Author

@tacaswell I thought I had already asked you here (obviously the comment never sent) about whether or not we should keep this PR open to backport.

@NelleV
Copy link
Member

NelleV commented Nov 8, 2016

We are starting to have a lot of merge conflicts in backports…
This cherry-pick does not apply cleanly. I'll create a PR with the backport.

@OceanWolf
Copy link
Member Author

Backport where? We don't have a 2.0.1 branch yet, as far as I can see... confused.

@NelleV
Copy link
Member

NelleV commented Nov 8, 2016

the 2.0.1 goes into v2.x. All minor release share the same branch.
But this has diverged from v2.x and doesn't need to be backported.

@NelleV NelleV modified the milestones: 2.1 (next point release), 2.0.1 (next bug fix release) Nov 8, 2016
@OceanWolf
Copy link
Member Author

Okay, a bit weird though since we haven't released 2.0 yet and we won't backport 2.1 as that will come direct from master, so to me it makes more sense to have a 2.0.x branch once 2.0 gets released and then a 2.1.x branch after 2.1 gets released, or do I miss something here?

@tacaswell
Copy link
Member

#7404 is reported against 1.5.3 so presumably it is broken in 2.x as well?

What you suggest does make sense under normal circumstances, but for now we are in a weird state of not having a 1.5.x and 2.x being a 'live' branch as well as master so 2.x is serving as both the target of 'new' stuff for 2.0 and all of the bug-fixes that are reported against older version.

@OceanWolf
Copy link
Member Author

Yes, broken MPL 1.5+.

Okay, I find that confusing. Did that get written somewhere up how this branches currently work and I missed it?

So as I understand it:

  • 1.5.x no longer exists because we don't want to release another bug fix before 2.0
  • 2.0 will launch from the 2.x branch which in turn branched from 1.5.x? This kind of makes sense apart from I thought 2.0 was supposed to only include the style changes, no features (and I presumed that meant bug fixes as well).
  • 2.1 will come from master and then we will branch that to create a 2.1.x branch if we have bug-fixes.

I would have expected it to flow more logically, like we have 1.5.x bug fix branch from which we branch a 2.0.x branch which the style changes go into. Bug fixes go into 1.5.x, then every time we do a bug-fix release we rebase the 2.0 branch onto the 1.5.x branch. When we release 2.0 we tag in the 2.0.x branch and then do a final rebase from the 1.5.x branch to make sure we have all the bugs that didn't make in into a 1.5.x release ready for our first 2.0.x bug fix release.

@QuLogic
Copy link
Member

QuLogic commented Nov 8, 2016

Public commits in a popular repo like this should not be rebased. You are essentially saying the same thing, except we merge instead of rebase.

@tacaswell
Copy link
Member

Bug fixes can always go in ;) There have been a couple of emails to the mailing list about it, but I do not think it was ever explicitly stated that bug-fixes and documentation were fair-game to backport.

We can not rebase any branch on the mpl org, too many people treat it as an 'upstream'

@QuLogic
Copy link
Member

QuLogic commented Nov 8, 2016

Also, the 2.0.1 milestone is code for a nice-to-have, non-blocker issue, but it's really just the same as 2.0 once it's merged. No-one has been updating them, but I might just do that after a release is made.

@QuLogic QuLogic changed the title [MRG+1] FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404 FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404 Nov 8, 2016
@OceanWolf
Copy link
Member Author

Ahh okay, confusion solved.

@QuLogic
Copy link
Member

QuLogic commented Dec 7, 2016

#7404 was reported against 1.5.3 and was intended to be backported, but it appears this hasn't been; should it be (to 2.0.1 if necessary)?

@OceanWolf
Copy link
Member Author

Ahh yes, it does need backporting.

QuLogic pushed a commit that referenced this pull request Dec 8, 2016
FIX: MPL should not use new tool manager unless explicited asked for.  Closes #7404
@QuLogic
Copy link
Member

QuLogic commented Dec 8, 2016

Backported to v2.x as 7371b52.

@QuLogic QuLogic added this to the 2.0 (style change major release) milestone Dec 8, 2016
@QuLogic QuLogic removed this from the 2.1 (next point release) milestone Dec 8, 2016
@QuLogic QuLogic added the MEP: MEP27 decouple pyplot from backends label Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MEP: MEP27 decouple pyplot from backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants