Skip to content

Conversation

OceanWolf
Copy link

No description provided.

…, 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
Copy link
Owner

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

@OceanWolf
Copy link
Author

@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 navigation.py won't display the plot, toolbar works, but gives errors when trying to interact with the non-existent plot-area. Can you confirm on your branch please?

@@ -2556,6 +2556,10 @@ def __init__(self, canvas, num):

"""

self.toolbar = self._get_toolbar()
Copy link
Owner

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...

Copy link
Author

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?

Copy link
Author

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?

Copy link
Owner

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?

Copy link
Owner

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.

Copy link
Author

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.

Copy link
Owner

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.

@fariza
Copy link
Owner

fariza commented Jul 29, 2014

@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.
Why don't put the add_tools after the _get_navigation inside the backend implementation.

When we are going to touch all the backends we can find the perfect spot to clear this.

@OceanWolf
Copy link
Author

Done! Onwards and upwards! :D

fariza added a commit that referenced this pull request Jul 29, 2014
…vent-framework

Navigation toolbar coexistence event framework
@fariza fariza merged commit ff94301 into fariza:navigation-toolbar-coexistence Jul 29, 2014
@OceanWolf OceanWolf deleted the navigation-toolbar-coexistence-event-framework branch July 29, 2014 22:15
@OceanWolf
Copy link
Author

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?

@fariza
Copy link
Owner

fariza commented Jul 30, 2014

  • Delete your branch in github (with the click)
  • Switch to your local copy of this MEP branch (navigation-toolbar-coexistence)
  • Delete locally your branch (that I just merged navigation-toolbar-coexistence-event-framework if I'm not wrong)
  • Being in your local copy of the MEP branch, pull the changes from my repo branch.

Now your local copy of the MEP branch is up to date with the branch in my repo.
Now if you want to make more changes, just start again making a branch based on this MEP branch and so on....

@fariza
Copy link
Owner

fariza commented Jul 30, 2014

@OceanWolf this is more or less the same as
http://matplotlib.org/devel/gitwash/development_workflow.html#staying-up-to-date-with-changes-in-the-central-repository
and http://matplotlib.org/devel/gitwash/development_workflow.html#deleting-a-branch-on-github
The difference is that upstream is my repo, and you are pulling not from upstream/master but from myrepo/navigation-toolbar-coexistence

As I told you at the begining, this is not as straight forward as doing it against upstream/master.

fariza pushed a commit that referenced this pull request Aug 5, 2014
fariza pushed a commit that referenced this pull request Feb 4, 2015
fariza pushed a commit that referenced this pull request Feb 4, 2015
fariza pushed a commit that referenced this pull request Aug 8, 2015
fariza pushed a commit that referenced this pull request May 11, 2016
One too many assertions

Closes #1
fariza added a commit that referenced this pull request Sep 22, 2016
fariza pushed a commit that referenced this pull request Oct 21, 2016
DOC minor fixes on the documentation of errorbar
fariza pushed a commit that referenced this pull request Dec 5, 2017
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants