NavigationToolbar2Tk: make packing optional. #15274

jmehne · 2019-09-16T18:42:23Z

PR Summary

Currently, NavigationToolbar2Tk cannot be easily used with the grid layout manager.
When initalizing the toolbar, it is immediately packed: self.pack(side=tk.BOTTOM, fill=tk.X)

I think this is unfortunate behavior, because:

It is unexpected.
Usually, you initialize a widget and afterwards it must be added to one of the layout managers.
It makes it hard to use the toolbar with the grid layout manager.
You need to call pack_forget() on the toolbar first, before you can add it to the grid layout manager.
When resizing a window that contains the canvas and the toolbar in the same frame: how do you make sure, that the toolbar is not resized first and thus disappears before the canvas widget?
You need to initialize it before the canvas, which again, is not super obvious.
But then, how do you place the toolbar on top of the canvas - it is packed with side=tk.BOTTOM, so basically you have to call pack_forget and repack it with side=tk.TOP.
Or, you initialize the canvas before the toolbar, but then you get the resizing problem again.

I assume we cannot change the default behavior for backwards compatibility, so this PR makes packing optional.

No idea how to write a test for this. Couldn't get the tests to run locally on master, so I'm relying on the CI tests.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

ImportanceOfBeingErnest · 2019-09-16T18:58:11Z

lib/matplotlib/backends/_backend_tk.py

+        If True, add the toolbar to the parent's pack manager's packing list
+        during initialization with *side='bottom'* and *fill='x'*.
+        If you want to use the toolbar with a different layout manager, you
+        must first call *pack_forget* on the toolbar object.


I was confused by this sentence when first reading it, After all, the change proposed here is meant to prevent the need to call pack_forget. Maybe move it to the bottom and let it say

If you want to use the toolbar with a different layout manager, use pack_toolbar=False.

jmehne · 2019-09-16T20:28:50Z

I changed the sentence accordingly.

I also changed the "Embedding in Tk" example again to use the new parameter, because I believe it is usually the better option:

If you want to use pack, it is now easier to infer from the example how to place the toolbar at the top.
If you don't want to use pack, it should just work.
Explicit is better than implicit :)

timhoffm

Thanks, this looks good modulo some minor documentation.

lib/matplotlib/backends/_backend_tk.py

jmehne · 2019-09-17T05:51:16Z

Thanks for the reviews! I changed the docstring accordingly.

jmehne · 2019-09-17T06:09:26Z

Hm, please don't merge this yet. In the example the toolbar height gets suddenly resized. Will take a look later today.

anntzer · 2019-09-17T06:32:06Z

lib/matplotlib/backends/_backend_tk.py

    """
-    def __init__(self, canvas, window):
+    def __init__(self, canvas, window, pack_toolbar=True):


please make the argument keyword only

After some googling the signature is now:

def __init__(self, canvas, window, *, pack_toolbar=True):

I didn't know this was possible at all, so please correct me if there is a more canonical way to do it.

jmehne · 2019-09-17T06:33:10Z

Okay, I fixed the example. I used expand=True for the toolbar in the example which didn't make sense.
Looks good now:

anntzer · 2019-09-17T06:34:25Z

lib/matplotlib/backends/_backend_tk.py

        self.canvas = canvas
        # Avoid using self.window (prefer self.canvas.get_tk_widget().master),
        # so that Tool implementations can reuse the methods.
        self.window = window
+        self.pack_toolbar = pack_toolbar


This doesn't need to be a public attribute -- it's just there for coordination between __init__ and _init_toolbar -- which gets called by __init__ anyways.
In fact you could even move the call to pack out of _init_toolbar and put it here:

if pack_toolbar: self.pack(...)

I moved the call.

anntzer · 2019-09-17T06:40:26Z

I agree that the old behavior looked strange (well, at least from a qt PoV; dunno what's standard with tk...); if you think we should ultimately switch the default to not packing, see e.g. the deprecation strategy used in #12380

Currently, the toolbar cannot be easily used with Tk's grid layout manager, because it is already packed during initialization. Thus, it is necessary to manually call `pack_forget` on the toolbar. Add a parameter, that makes this a bit easier to discover.

jmehne · 2019-09-17T07:08:10Z

I'm not really a Tkinter expert, so I don't feel like I can decide at the moment if the default behavior should ultimately be switched or not. My gut feeling says yes, but there is no urgency to switch so I think it's a good idea to just introduce the change and let it sit for a bit.

lib/matplotlib/backends/_backend_tk.py

anntzer

lgtm, just tiny formatting things

jmehne · 2019-09-17T16:31:48Z

Docstring updated to use double backticks.

timhoffm · 2019-10-02T21:49:05Z

Thanks!

ImportanceOfBeingErnest reviewed Sep 16, 2019

View reviewed changes

jmehne force-pushed the tk-toolbar-grid branch 3 times, most recently from 60a4243 to 4910587 Compare September 16, 2019 20:26

timhoffm reviewed Sep 16, 2019

View reviewed changes

lib/matplotlib/backends/_backend_tk.py Outdated Show resolved Hide resolved

lib/matplotlib/backends/_backend_tk.py Outdated Show resolved Hide resolved

jmehne force-pushed the tk-toolbar-grid branch from 694b56e to f208cd3 Compare September 17, 2019 05:50

anntzer reviewed Sep 17, 2019

View reviewed changes

jmehne force-pushed the tk-toolbar-grid branch from f208cd3 to e7d365f Compare September 17, 2019 06:32

anntzer reviewed Sep 17, 2019

View reviewed changes

jmehne force-pushed the tk-toolbar-grid branch from e7d365f to f8b73d6 Compare September 17, 2019 06:58

anntzer added the GUI: tk label Sep 17, 2019

anntzer reviewed Sep 17, 2019

View reviewed changes

lib/matplotlib/backends/_backend_tk.py Outdated Show resolved Hide resolved

anntzer approved these changes Sep 17, 2019

View reviewed changes

Embedding in Tk example: be more layout manager agnostic.

9737747

jmehne force-pushed the tk-toolbar-grid branch from f8b73d6 to 9737747 Compare September 17, 2019 16:29

timhoffm approved these changes Oct 2, 2019

View reviewed changes

timhoffm added this to the v3.3.0 milestone Oct 2, 2019

timhoffm merged commit f49b7d1 into matplotlib:master Oct 2, 2019

jmehne deleted the tk-toolbar-grid branch October 3, 2019 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NavigationToolbar2Tk: make packing optional. #15274

NavigationToolbar2Tk: make packing optional. #15274

jmehne commented Sep 16, 2019 •

edited

Loading

ImportanceOfBeingErnest Sep 16, 2019

jmehne commented Sep 16, 2019 •

edited

Loading

timhoffm left a comment

jmehne commented Sep 17, 2019

jmehne commented Sep 17, 2019

anntzer Sep 17, 2019

jmehne Sep 17, 2019

jmehne commented Sep 17, 2019

anntzer Sep 17, 2019

jmehne Sep 17, 2019

anntzer commented Sep 17, 2019

jmehne commented Sep 17, 2019

anntzer left a comment

jmehne commented Sep 17, 2019

timhoffm commented Oct 2, 2019

NavigationToolbar2Tk: make packing optional. #15274

NavigationToolbar2Tk: make packing optional. #15274

Conversation

jmehne commented Sep 16, 2019 • edited Loading

PR Summary

PR Checklist

ImportanceOfBeingErnest Sep 16, 2019

Choose a reason for hiding this comment

jmehne commented Sep 16, 2019 • edited Loading

timhoffm left a comment

Choose a reason for hiding this comment

jmehne commented Sep 17, 2019

jmehne commented Sep 17, 2019

anntzer Sep 17, 2019

Choose a reason for hiding this comment

jmehne Sep 17, 2019

Choose a reason for hiding this comment

jmehne commented Sep 17, 2019

anntzer Sep 17, 2019

Choose a reason for hiding this comment

jmehne Sep 17, 2019

Choose a reason for hiding this comment

anntzer commented Sep 17, 2019

jmehne commented Sep 17, 2019

anntzer left a comment

Choose a reason for hiding this comment

jmehne commented Sep 17, 2019

timhoffm commented Oct 2, 2019

jmehne commented Sep 16, 2019 •

edited

Loading

jmehne commented Sep 16, 2019 •

edited

Loading