-
Notifications
You must be signed in to change notification settings - Fork 1
Navigation toolbar coexistence event framework #1
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
Navigation toolbar coexistence event framework #1
Conversation
…, before filling it with tools. Added a nice utility API function, Navigation.addTools.
@@ -31,6 +31,7 @@ | |||
from matplotlib.cbook import _string_to_bool | |||
from matplotlib import docstring | |||
from matplotlib.backend_bases import FigureCanvasBase | |||
from matplotlib.backend_tools import tools as default_tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definetily remove this from pyplot.py
@fariza Have removed from pyplot. What do you think of this new location? I have some more changes to put into add_tools, shall I do them now, or wait for you to merge? Also testing GTK3{Cairo,Agg} in |
@@ -2556,6 +2556,10 @@ def __init__(self, canvas, num): | |||
|
|||
""" | |||
|
|||
self.toolbar = self._get_toolbar() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the backend doesn't have toolbar? PDF, SVG, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, my mistake, I should have fixed that now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or didn't you mean that. If the backend doesn't have a toolbar, then as far as I can see, this will make no difference.
I have added fallback methods below to None, and if they use the new navigation system, then it will set up the tools with just the hotkeys given by keymap, as happened in your branch, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do it this way, we have to make sure, no backend is setting the toolbar
and/or navigation
attribute before calling FigureManagerBase.__init__
Are you sure abut this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FigureManagerBase
is pretty clean, adding this here will polute the class. I am not sure it is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought it looked pretty clean to me. I understand your concern that other backends do it differently, looking at your example of backend_wx.py
. If I get your point correctly, you have concern over the backward compatibility, as we only plan to touch just these two backends with this MEP, if so, I completely agree with all of it. I guess its possible to put in a dirty fix until they have all been modified to follow a standard API, but...
As you say, several big changes will come later, so no point in trying to figure out the right way to do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this MEP we are planning on touching all the backends, but slowly, starting with these two.
Hopefully, we will touch the other backends when the bigger changes are implemented, so we have to touch them only once.
Is not the right/wrong way to do it, it is that the right way now, might be the wrong way latter. So it is preferable not touching big Base classes, unless strictly necessary for the moment.
@OceanWolf We have a couple of more things coming, my MEP23 and the PR matplotlib#2624 both of them, will require HARD touching of all the backends. When we are going to touch all the |
Done! Onwards and upwards! :D |
…vent-framework Navigation toolbar coexistence event framework
One question that I can't find in the workflow. What happens next? It said above that I could now delete the branch. So I guess I click that, and then, I have to delete the branch in my local repo too before creating a new one? What do you do? |
Now your local copy of the MEP branch is up to date with the branch in my repo. |
@OceanWolf this is more or less the same as As I told you at the begining, this is not as straight forward as doing it against upstream/master. |
…qt_fix Instantiate Sip Mock before use
Create colormap_normalizations.py
One too many assertions Closes #1
DOC minor fixes on the documentation of errorbar
This fixes some possible heap buffer overflows, such as the following triggered by our cmmi10.ttf: ``` ERROR: AddressSanitizer: heap-buffer-overflow on address 0x617000235709 at pc 0x7f95efd3c48a bp 0x7ffe41b6ecc0 sp 0x7ffe41b6ecb0 READ of size 1 at 0x617000235709 thread T0 #0 0x7f95efd3c489 in utf16be_to_ascii extern/ttconv/pprdrv_tt.cpp:178 #1 0x7f95efd3c489 in Read_name(TTFONT*) extern/ttconv/pprdrv_tt.cpp:339 #2 0x7f95efd499ef in read_font(...) extern/ttconv/pprdrv_tt.cpp:1325 #3 0x7f95efd4c602 in get_pdf_charprocs(...) extern/ttconv/pprdrv_tt.cpp:1420 #4 0x7f95efd35c22 in py_get_pdf_charprocs src/_ttconv.cpp:217 0x617000235709 is located 1 bytes to the right of 648-byte region [0x617000235480,0x617000235708) allocated by thread T0 here: #0 0x7f9612262a38 in __interceptor_calloc (/usr/lib64/libasan.so.4+0xdea38) #1 0x7f95efd3b261 in GetTable(TTFONT*, char const*) extern/ttconv/pprdrv_tt.cpp:140 ```
No description provided.