Updating MEP22

@fariza

Although I like, in general, the idea of MEP22, there are a number of details that I would like to change. If we can agree on how to handle them, I would be happy to push the qt implementation. We'd still need a wx implementation (which would be painful for me to write but I guess I could cargo-cult it if no one else volunteers) and a macos implementation (which I would have no way to work on).

A quick look suggests the following points (I may think of other stuff later):

Suggested changes to MEP22

Change `_get_cls_to_instantiate`

Edit: Fixed by #20990.

Currently, backend_tools defines a default_tools dict, which maps tool names to either classes, or names of classes; for the latter, toolmanager.add_tool resolves the name in the current module context (e.g., the 'cursor': 'ToolSetCursor' tool resolves to whatever class is named ToolSetCursor in the current backend module). This looks brittle (e.g., one cannot override a tool that is specified directly as a class in default_tools). Instead, I propose that that default_tools simply becomes a list of default tools, that the name becomes a class attribute, and that overrides in backends get specified using a decorator

@tools.override(backend="qt5agg")
class ToolX(backend_tools.ToolX):
    ...

(possibly stacking decorators if the override applies to multiple backends). The decorator registers an override for the ancestor class(es), and would store the relevant override information somewhere for later use in add_tool.

Possibly, some default tools could be abstract, always requiring the backend to reimplement them, but not erroring if this is not the case (the tool is then just not available).

Get rid of the `data` kwarg to `Tool.trigger`

AFAICT this kwarg is only present so that ZoomTool can pass to RubberbandTool the coordinates where a rubberband needs to be drawn. I would simply get rid of RubberbandTool and let ZoomTool define the relevant drawing methods as abstract methods that need to be overriden by the individual backends.

Get rid of the `sender` kwarg to `Tool.trigger`?

(Its purpose is not clear to me, but perhaps I am missing something.)

Let togglable tools be implemented as contextmanagers (instead of enable/disable)

In general, this helps having to stick a bunch of temporary attributes on the instance to store the state. The caller code can always call __enter__ and __exit__ manually.

`ToolBase.destroy(self, args)` should lose the `args` in the signature

Edit: Fixed by #22509.

Why do we need it?

What is happening with NavigationBase?

The class does not seem to be implemented anywhere. A similar but not identical API seems implemented by ToolManager.

Support sub-tools as menus to the tool button?

Edit: Essentially fixed (from my POV) by figure.hooks.

e.g. #7696 (comment)

@OceanWolf

@OceanWolf (cc)

@anntzer Thanks for the comments you point out some things that I have thinking about for some time.

Before getting into details, I want to remind everybody that we are waiting for resolution of MEP27.
Among other things, MEP27 brings generic windows and filechooser using those the amount of tools have to be implemented for each backend is reduced to 1 (cursor) and if we move the set_cursor isnside MEP27 all the default tools can be backend independent.

Regarding your comments

Change _get_cls_to_instantiate
I agree with you that this is not the best way to do it and it need improvement.
Maybe I'm missing something, I find your solution a little bit too complex.
One of the goals was to make all backends the same, so the user experience doesn't change from one to another that is why only the tools that are available for all backends are default tools.
Let's define goals for the default tools and how do we want to control what gets instantiated (control by the user), then we can discuss implementation details

Get rid of the data kwarg to Tool.trigger
I have to double check if data isn't used anywhere else. I like the idea to be able to pass some data to the tools at trigger time but please share your thoughts regarding this.
On the side of the RubberBand and ZoomTool If you move the drawing methods inside ZoomTool then nobody else can use the ruberband, If I'm not wrong that was one point brought to us during development, but if there is no oposition, why not.

Get rid of the sender kwarg to Tool.trigger
If I am not wrong (maybe I am) this was introduced to prevent "re-trigger" the tool by itself, as some tools trigger other tools. Needs to be checked carefuly but I like the thought of getting rid of that.

**ToolBase.destroy(self, args) should lose the args in the signature
I don't see the *args in the signature https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backend_tools.py#L135

What is happening with NavigationBase?
I don't see this class?

Support sub-tools as menus to the tool button?
I like the idea, we were looking for "context menu" or something similar. Are you suggesting as tool or as tool modifier/configuration

The comments are regarding https://matplotlib.org/devdocs/devel/MEP/MEP22.html, not the actual implementation. If things changed (e.g. for ToolBase.destroy's signature, and NavigationBase), let's update the MEP.

Regarding _get_cls_to_instantiate, I can try to implement something and we can then see whether it's too complicated :-)

If we want to make rubberbanding reusable, I'd just make it a free function. Trying to make it a Tool feels like shoehorning it into the wrong API.

Do you have an actual example of problems with retriggering? I see your point, but let's not overengineer things too.

No great idea yet as of a possible sub-tool API.

@anntzer

Hi @anntzer, I already made branches for all the existing backends (apart from the mac backend(s) and GTK2, and NbAgg) for MEP27, I will need to tweak them a bit as we have slightly changed the API since I created those branches, but a lot of the work has already been done. I should become free to take a look at this closer in a week or two.

Oh, and yes NavigationBase got changed to ToolManager during the long PR review phase of the first MEP22 PR that made it to trunk.

I am totally not familiar with MEP27, so I'll need to review it carefully. Should it go before or after MEP22?

Note that Qt also has the "edit axes curves and image parameters" tool which should definitely be kept by default, but until someone implements a formlayout-like API (... or just reimplements the whole tool) for other backends it's going to be tough to provide it for other backends as well (see also discussion at #1543).

Logically MEP27 goes before MEP22 as a more fundamental API as it refactors FigureManager and abstracts a lot of backend concepts (see http://matplotlib.org/devel/MEP/MEP27.html); in terms of timeline though it goes the other way around with MEP22 inspiring MEP27 and superseded another refactor that Tom was working on. This has meant that MEP27 refactors the MEP22 code a bit (although no changes to API).

@anntzer

@anntzer I just updated #4889 trying to reflect the current state of affairs.
I prefer to direct users to the documentation, in principle it is more updated than the MEP document

@anntzer

@anntzer
Get rid of the sender kwarg to Tool.trigger
If a tool needs access to a widgetlock, it needs the the sender to be the owner of the widgetlock
Currently it is only used by the rubberband.
This decision is delayed until we agree on rubberband future

@anntzer

@anntzer
Change _get_cls_to_instantiate
I don't fully understand what you mean, do you have a little sample code as reference?

@fariza

@fariza see #9941. Of course the other points raised originally still remain.

Also, is ToolManager supposed to keep a ref to a FigureManager object? Its docstring says yes, but in practice it only has a ref to the Figure itself.
In other words, is ToolManager supposed to work with non-FigureManaged Figures? If yes, then the docstring should be updated; if not, then perhaps it should take the FigureManager as argument in the ctor rather than the figure.

I agree, the problem is in the doc. I'll fix it

@anntzer

@anntzer docstring fixed in #9949

So it's supposed to work with nonFigureManaged figures?

The idea is to be able to change the figure. The figuremanager has only one figure and is not really needed, so why keep it?

@timhoffm

@timhoffm I could say the same about many things that I don't use (that is not worth it). I agree that there is added complexity but I am pretty sure it won't present a problem for most new tools.

@anntzer please take your dozen lines and try to use it with several figures (real life) not just to test.

Don't you find annoying that the state of the Pan/Zoom button is not persistent across tabs?
Don't you find annoying that you have several instances of the Toolbar?
If you open the "configure subplots" it doesn't let you change tabs.
- Some backends will let you change tabs because that sub-window is implemented as a window and not as a dialog (it is like that and I wasn't there, so not my decision/fault)
- Imagine for a second that it does let you change tabs. So wouldn't it make sense to refresh the content of that sub-window based on the figure?
Don't you want to move that toolbar somewhere else that is not inside the tab? why? why not?
Forget tabs, what if you change the figure programatically without changing windows?

I use that feature. And I can say that everybody inside my team (at work) uses that feature. And the feedback that I got from them, was that indeed I needed to make the toolbar state follow the figure.
My first implementation was as simple as yours, but it wasn't enough for our use.

Why is that particular feature bothering you so much? Are you trying to implement a tool that got too complex because of that?

@fariza

Ahh, okay, I get you on figure reassignment within user created tools as that passes on complexity for the end user, but I counter that by saying tools shouldn't store such data anyway, think of tools we use in real life, like a knife, or a hammer, or a screwdriver, do these items store state? No, because they exist as tools, tools don't store state. You pick up the tool you want for a job and expect it to work just the same the last time you picked it up (I remember having a similar debate with @fariza way back when).

This follows through to tools here, if something should keep state, like history, then that should probably get stored either on the figure or on the figure manager, not as part of the tools or toolbar. Stepping aside to the multifigure thing, I guess such logic could get implemented there for tracking state? Otherwise that means that tabbing from one figure to the next would permanently erase the history stacks which makes this feature pointless if you tab a lot.

@fariza

@fariza I will give it a try once your tabbed_backend is back in a working state (or you can just tell me what version of matplotlib it is compatible with). I won't compare my implementation with... stuff I can't compare with.

@fariza

@fariza what is the line that you envision between this work and a full-fledged GUI application built using a GUI toolkit and Matplotlib as the plotting engine? I think we should be clear on that line...

I personally appreciate the tools on the current interactive backends, and I see a value in allowing users to create new lightweight tools that suit their needs. But I am leery of mission creep.

@OceanWolf

@OceanWolf that state is already stored on the figure (that's how the pre-toolmanager nav buttons work, after all).

Ah, I realized that tabeed_backend does work with 2.2, not master, apologies for that. So I gave it a try.

Don't you find annoying that the state of the Pan/Zoom button is not persistent across tabs?

... no, I just press "O" or "P" and trigger the tool I want (well in the example I gave you need to connect the global key_press_handler, but again that's one line that's standard)

Don't you find annoying that you have several instances of the Toolbar?

How does that affect the end user in any way?

If you open the "configure subplots" it doesn't let you change tabs.
Some backends will let you change tabs because that sub-window is implemented as a window and not as a dialog (it is like that and I wasn't there, so not my decision/fault)
Imagine for a second that it does let you change tabs. So wouldn't it make sense to refresh the content of that sub-window based on the figure?

The tabbed backend doesn't refresh that content either.

Don't you want to move that toolbar somewhere else that is not inside the tab? why? why not?

... no? why would I? but note that it's just a boring regular old widget anyways, you can indeed add it anywhere you want. If you really want it outside of the tab, you can use a QStackedWidget (http://doc.qt.io/qt-5/qstackedwidget.html) and switch the "real" toolbar depending on the tab that's active -- see e.g. the figureoptions editor for something similar (there's one form per artist, that get switched based on the artist combobox).

Forget tabs, what if you change the figure programatically without changing windows?

The toolbar is attached to the canvas, not to the figure, so it doesn't matter.

@fariza

Don't you find annoying that the state of the Pan/Zoom button is not persistent across tabs?

... no, I just press "O" or "P" and trigger the tool I want (well in the example I gave you need to connect the global key_press_handler, but again that's one line that's standard)

You say that now, but what about switching between hundreds of figures as a power user, it can get quite annoying and proves a disruption in the thought process. Maybe you don't find it a problem, but why deprive that for others who do? This comes back to my point about fluidity earlier, about making code that's reusable and adaptable, that seems the quintessential function of a library, if we don't matplobLib doesn't live up to its name. It doesn't take too much work to allow the user to choose whether they want to lock the tool state when changing tabs or keep them the same, anyway we digress as we talk about a different MEP, and now I wonder if the multitab MEP should exist as a separate package or not... but that exists as a separate discussion, the point here exists as to why we as a library we should force other people to do it a certain way creating their own packages extending our own.

@fariza Was there a reason why the pan/zoom history was moved to the tool and taken away from the figure?

@anntzer

@anntzer You are right, the "configure subplots" is not refreshing. I have to fix that, thanks
The figure and the canvas are interconnected, so why does it matter if the toolbar is attached to one or the other?

That state is already stored on the figure (that's how the pre-toolmanager nav buttons work, after all).

Correct me if I'm wrong. looking at backend_bases.NavigatioNToolbar2 there is self._nav_stack = cbook.Stack() that stores that information.

The views history in the tools, toolmanager or figure was discussed. One thing that was clear was that storing that in the figure was not an acceptable solution (). And I don't think it ever was in the figure, was it at some point?.
@OceanWolf I remember a comment from a core member basically banning the use of the figure to store that info there. I just can not find it.

@jklymak As it is right now, even a full fledged GUI with mpl as plotting engine might use the toolmanager and tools.
Probably not the Toolbar (fancy new GUI after all).
Would you like to re-implement all the Zoom/Pan logic? With the ToolManager you can reuse that without having to use the Toolbar.

@OceanWolf

@OceanWolf It is not difficult to keep actions in sync across toolbars -- just connect the relevant event handlers. Extending on my earlier example:

from PyQt5.QtCore import *
from PyQt5.QtWidgets import *
from matplotlib.backend_bases import key_press_handler
from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg, NavigationToolbar2QT
from matplotlib.figure import Figure

qapp = QApplication([])

win = QMainWindow()
tw = QTabWidget()
win.setCentralWidget(tw)

# Yeah, globals are ugly, just imagine this is stored in whatever relevant manager object.
_actions_sync = []

def add_page(tabname, fig):
    widget = QWidget()
    layout = QVBoxLayout()
    widget.setLayout(layout)
    canvas = FigureCanvasQTAgg(fig)
    canvas.setFocusPolicy(Qt.StrongFocus)
    canvas.setFocus()
    navbar = NavigationToolbar2QT(canvas, None)

    checkable_actions = {action.text(): action for action in navbar.actions()
                         if action.isCheckable()}
    _actions_sync.append(checkable_actions)
    for action in checkable_actions.values():
        @action.toggled.connect
        def sync_actions(state, *, action=action):
            for checkable_actions in _actions_sync:
                sibling = checkable_actions[action.text()]
                if sibling.isChecked() == state:  # Don't recurse.
                    continue
                sibling.trigger()

    layout.addWidget(navbar)
    layout.addWidget(canvas)
    tw.addTab(widget, tabname)
    canvas.mpl_connect(
        "key_press_event",
        lambda event: key_press_handler(event, canvas, navbar))

fig = Figure(); add_page("tab1", fig)
fig.subplots().plot([0, 1])

fig = Figure(); add_page("tab2", fig)
fig.subplots().plot([1, 0])


win.show()
qapp.exec_()

Again, note that this would work even if you decide that after all you want multiple windows instead of just one, which is not the case for the current toolmanager implementation. So in a sense it is the toolmanager implementation that "force other people to do it a certain way creating their own packages extending our own" rather than letting them use standard APIs.

The figure and the canvas are interconnected, so why does it matter if the toolbar is attached to one or the other?

Because you can reassign them in either direction. For example that's actually how saving to pdf works... (the figure is reparented to a pdf canvas.)

Indeed the _nav_stack is in the toolbar, I was wrong and stand corrected. What I meant, however, was that it was stored on a global object that is associated to the figure (it could just as well be stored in the figure, a PR doing so would just be moving it from A to B), not on reparentable objects.

Note that I am not saying that MEP22 should be thrown out of the window. Yes, the ability to reuse e.g. zoom/pan in a pluggable fashion is nice. The only thing we're discussing in this subdiscussion is whether tools need to be reparentable or not...

So in a sense it is the toolmanager implementation that "force other people to do it a certain way creating their own packages extending our own" rather than letting them use standard APIs.

I don't see how making tools reparentable forces other people to reparent them.

I will need to take some time to think through this properly though, time I do not currently have...

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

I think the important parts have been fixed and the rest is just small API tweaks, which are not so crucial.

Navigation Menu

Repository navigation

Updating MEP22 #8712

Change `_get_cls_to_instantiate`

Get rid of the `data` kwarg to `Tool.trigger`

Get rid of the `sender` kwarg to `Tool.trigger`?

Let togglable tools be implemented as contextmanagers (instead of enable/disable)

`ToolBase.destroy(self, args)` should lose the `args` in the signature

What is happening with NavigationBase?

Support sub-tools as menus to the tool button?

18 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Updating MEP22 #8712

Description

Change _get_cls_to_instantiate

Get rid of the data kwarg to Tool.trigger

Get rid of the sender kwarg to Tool.trigger?

Let togglable tools be implemented as contextmanagers (instead of enable/disable)

ToolBase.destroy(self, *args) should lose the *args in the signature

What is happening with NavigationBase?

Support sub-tools as menus to the tool button?

Activity

fariza commented on Jun 5, 2017

anntzer commented on Jun 5, 2017

OceanWolf commented on Jun 5, 2017

anntzer commented on Jun 5, 2017

OceanWolf commented on Jun 5, 2017

fariza commented on Dec 5, 2017

fariza commented on Dec 5, 2017

fariza commented on Dec 5, 2017

anntzer commented on Dec 6, 2017

anntzer commented on Dec 6, 2017

fariza commented on Dec 6, 2017

fariza commented on Dec 6, 2017

anntzer commented on Dec 6, 2017

fariza commented on Dec 6, 2017

18 remaining items

fariza commented on Apr 19, 2018

OceanWolf commented on Apr 19, 2018

anntzer commented on Apr 19, 2018

jklymak commented on Apr 19, 2018

anntzer commented on Apr 19, 2018

anntzer commented on Apr 19, 2018

OceanWolf commented on Apr 19, 2018

fariza commented on Apr 19, 2018

anntzer commented on Apr 19, 2018

anntzer commented on Apr 19, 2018

OceanWolf commented on Apr 19, 2018

github-actions commented on Apr 12, 2023

anntzer commented on Apr 12, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Change `_get_cls_to_instantiate`

Get rid of the `data` kwarg to `Tool.trigger`

Get rid of the `sender` kwarg to `Tool.trigger`?

`ToolBase.destroy(self, args)` should lose the `args` in the signature