-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
This is the easiest fix. |
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 fig = Figure()
figmgr = FigureManager(fig, title) |
Maybe for simplicity, we should just use your fix, and with MEP27 we can fix the warnings handling |
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. |
ok, we fix this with MEP27 and the warnings in a separate PR |
Please remember to backport |
@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. |
We are starting to have a lot of merge conflicts in backports… |
Backport where? We don't have a 2.0.1 branch yet, as far as I can see... confused. |
the 2.0.1 goes into v2.x. All minor release share the same branch. |
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? |
#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. |
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:
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. |
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. |
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' |
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. |
Ahh okay, confusion solved. |
#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)? |
Ahh yes, it does need backporting. |
FIX: MPL should not use new tool manager unless explicited asked for. Closes #7404
Backported to v2.x as 7371b52. |
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.