-
Notifications
You must be signed in to change notification settings - Fork 1
Navigation decoupling using events #2
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 decoupling using events #2
Conversation
I will comment on specific lines as I understand the changes but the one thing that is clear. Please split this in two PRs.
|
Don't worry for the GTK changes. When we finish all the changes I can take care of that |
if name in self._instances: | ||
del self._instances[name] | ||
self._handle_toggle(name) | ||
if name in self._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.
In what case, name is not present in self._tools
?
Hmm, I thought I may have done too much for one pull request, but it seemed like one would go in as a very small request, the other much larger, but I can try and separate them out and see, in which case it will become a part 2 and a part 3. Do you know of a way to do that on github? If not, would it make it easier for you to wait until I have split the PRs before you start commenting? What do you mean by |
Sorry, I don't know. |
Please describe the problem with the |
Problem with home/back/forward: PR: I will probably do this tomorrow as it gets late here. |
What I would do (I'm far from expert) I would create a new branch based on the original code. And port only the instances relate code. This way you keep this branch as reference. But do it as you like, at the end is the same |
Reading up on the subject, I think I will perform a rebase as it seems cleaner. Thinking about the code, I can actually see three PRs here, dividing the tool instances into two parts. Separating out the code should help you to see the logic behind the various bits of code. I will let you know when it has become "safe" the code again ;). |
@OceanWolf |
@fariza No problem, my laptop died on me on Monday (general system board failure :'(), so trying to sort something out. But yes, the reason for my silence so far. Enjoy your holiday! On Wednesday, 6 August 2014, 15:02, Federico Ariza notifications@github.com wrote: @OceanWolf |
@OceanWolf I'm back, did you have time to advance on the split of the PR? |
No, sorry, laptop still fried. Slowly upgrading this desktop to a point where I can work on it. Hopefully only a few days more. As I remember, when I split it, the first patch contained around 20 lines of additions, and around 80 deletions, or something like that anyway. |
@OceanWolf |
BUG : missing manager assignment
FIX: respect alpha on alt face color
Suggestions for matplotlib#7072
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 ```
Have only made this work on TkAgg, will convert for GTK3 later. In fixing
ConfigureSubplotsTk
, I have noticed a marked decrease in dialogue reopening time :), I guess the "figure" creation slowed it down.I have noticed problems with the home/back/forward buttons on testing, though I guess this bug already existed, so I will look into it and fix it with a separate PR later.