Skip to content

Alternative approach to organizing uniform manager creation #22895

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

Closed

Conversation

timhoffm
Copy link
Member

Alternative to #18854.

Basic idea:

  • The canvas feels like it should be a relatively simple widget class and not have to care about the logic of creating a manager.
  • Still we want canvas.new_manager() for convenience.
  • So let's move the responsibility of wiring everything up to the Manager class and let the Canvas class only know which manager class to ask for creating a manager.

For easier understanding of the differences, this builds on top of #18854, but commits may be squashed.

This still needs reorganization/tweaking as the Manager classes are currently defined after the Canvas classes in the backends, so setting a Canvas class variable to a manager does not work out of the box (either change the definition order or make manager_class a class property to delay lookup).

The `new_manager` classmethod may appear to belong more naturally to the
FigureManager class rather than the FigureCanvas class, but putting it
on FigureCanvas has multiple advantages:

- One may want to create managers at times where all one has is a
  FigureCanvas instance, which may not even have a corresponding manager
  (e.g. `subplot_tool`).
- A given FigureManager class can be associated with many different
  FigureCanvas classes (indeed, FigureManagerQT can manage both
  FigureCanvasQTAgg and FigureCanvasQTCairo and also the mplcairo Qt
  canvas classes), whereas we don't have multiple FigureManager classes
  for a given FigureCanvas class.
@timhoffm timhoffm force-pushed the uniform-manager-creation-thof branch from 9288a0c to 645204f Compare April 25, 2022 21:16
@anntzer
Copy link
Contributor

anntzer commented Apr 26, 2022

Certainly, this proposal works, is very reasonable, and achieves the goals I had in #18854. (FWIW I would just hide the manager_class into a classproperty to avoid having to move huge chunks of code around.) I also agree that putting the logic in the manager class is semantically reasonable, although I don't really like the additional layer of indirection (but that's mostly personal taste). Only minor point: This probably needs documentation stating (I guess?) that new_manager is the intended callable for end users (typically not to be overridden by backends) and create_with_canvas the intended customization point for backends (typically not to be called by end users).

@timhoffm
Copy link
Member Author

timhoffm commented Apr 26, 2022

I agree with all your suggestions. This isn't meant to be a full PR but should rather illustrate the idea using more code and less words. I'm happy to hand this back to you - either here or via your original PR.

@anntzer
Copy link
Contributor

anntzer commented Apr 27, 2022

@tacaswell Thoughts on this API? I can integrate it into #18854 if we agree on that.

@tacaswell
Copy link
Member

TL;DR: I am in favor of this proposal with slight reservations.


Given that Python only gives us 1 __init__ method to be The One True entry point for creating instances of a class, I think it is the right choice to make that API verbose and exact and then use classmethod s to provide other, nicer in some contexts, entry points for creating versions of the object with varying assumptions (e.g. "please make the GUI window for me"). It that sense I am 👍🏻 on all of the variations of this we have in question.

I think one of the issues here is that we have circular concepts (not just code/references). From the point of view of the UI frameworks there is a very clear order the window (which almost the manager but not quite) contains the toolbar and the canvas so it makes sense to build this from the top (outside) down (in). This is taken to the extreme in Wx which requires that we make the container first!

On the other hand the Figure, which is tightly coupled to a Canvas, is the base conceptual container of Matplotlib. Thus from a user point of view it makes sense to say "I want a Figure (and, you know, take care of those UI details so I can see it)". This is exactly what plt.figure is doing, is the use case that we want to spawn helper windows for adjuster UIs, and is the animating idea of mpl-gui. Thus, it makes sense for the canvas to be the entry point and build the UI from the bottom (inside) up (out).

It is also the case currently, and encoded in this proposal, that a given canvas only works with one manager, but a given manager can work with multiple canvas classes. If you want to call Mangar.create_with_canvas then you have to also have in hand the Canvas class you want to use. However, that class should know what Manger it needs to use the API is a bit redundant (and maybe we should check that they are consistent?). These classes are circular enough, maybe we want to take this as a chance to streamline some things.

On the other hand, if we consider the manager class on the canvas to be a suggested / default / sure to work Manager, than having the hook to have the Manager class make a new manager given a Figure + the canvas class would make it easy to build a Manger class that creates each of the canvases in a single tabbed window.

On net, while I agree with @anntzer that extra layer of indirection is not the best, I support this API of putting the guts of building the manager + canvas combo in the manager. It reflects the inherent ouroborus nature of these classes that we have class methods on both of them and the extra hook point opens up the possibility of some nice new functionality.


As a side point, it is not clear to me why Canvas is a UIWidget, but Manager has a UIWindow.


I had a long message typed out on #18854, did not finish it, and then rebooted my computer and lost it 😞 . I had hoped re-writing it would make it shorter. Friends: it did not.

@anntzer
Copy link
Contributor

anntzer commented Apr 28, 2022

and maybe we should check that they are consistent?

I initially also did not like the need for the extra check, but I then realized that create_with_canvas will make some further calls either to FigureManagerFoo.__init__(..., canvas, ...) (non-wx case) or something else for wx, which can be the ones in charge for the type checking; i.e., I don't think create_with_canvas needs to perform the typecheck itself.

@jklymak
Copy link
Member

jklymak commented Apr 28, 2022

Not 100% following all of this, but this seems a situation where reasonable people would arrange the relationship between the modules differently. Would it be helpful if a final product from this was a write up and/or diagram of what role we see each component occupying so that future us, or other contributors understand the model?

@timhoffm
Copy link
Member Author

Would it be helpful if a final product from this was a write up and/or diagram of what role we see each component occupying so that future us, or other contributors understand the model?

That would certainly be helpful.

where reasonable people would arrange the relationship between the modules differently.

I agree that the relationships are somewhat messy, but it may well be that this is an inherent problem of conflicting priorities. As @tacaswell has pointed out from a usage point of view we want to build up everything from canvas widget. OTOH GUI Frameworks may want to start with the window and thus the manager. I suppose we cannot have both in the current structures. It may be that with additional classes (possibly either a super-manager that controls both window and canvas or splitting the logic canvas and it's gui-widget-aspekt). But this is a fundamental rearchitecturing of the backends and would likely break all external backends.

Overall, I think this solution is good enough. On the down side it creates a relatively tight coupling between manager and canvas. But on the plus side, responsibilites are well separated - "One side has to ask the other for help and vice-versa".

@anntzer
Copy link
Contributor

anntzer commented Apr 28, 2022

I can try to write up some more docs (although I got an out from @tacaswell at #18854 (comment) :-)) but note that right now this is still only touching private API (via _Backend); ultimately I think we should get rid of _Backend (notwithstanding its recent adoption into ipympl -- matplotlib/ipympl#305) and make manager_class/create_with_canvas the "blessed" API for defining backends (keeping support for new_figure_manager{,_given_figure} probably forever as these APIs have basically always existed). It may make more sense to wait for that switch before writing up the docs so that I don't have to write docs describing what is just an intermediate step; alternatively, we can alse make this latter change as part of this PR if we're not worried about this PR becoming too big to review.

@jklymak
Copy link
Member

jklymak commented Apr 28, 2022

maybe just a few notes as a comment at the top of the relevant module as a cookie crumb for future us would be more than adequate as opposed to user-facing docs.

@anntzer
Copy link
Contributor

anntzer commented Apr 28, 2022

Do you want something in the style of #18854 (comment) / #18854 (comment) / #22895 (comment) or are you looking for something else?

@tacaswell
Copy link
Member

So, after several attempts to draw this by hand I gave up and used networkx:

import matplotlib.pyplot as plt
import networkx as nx

connections = [
    ("manager", "window"),
    ("manager", "toolbar"),
    ("manager", "canvas"),
    ("toolbar", "canvas"),
    ("canvas", "figure"),
    ("canvas", "manager"),
    ("canvas", "toolbar"),
    ("figure", "canvas"),
]

tool_manager = [
    ("toolmanager", "figure"),
    ("manager", "toolmanager"),
]

implicit = [
    # from callbacks
    ("figure", "toolbar"),
    # from UI    
    ("window", "canvas"),
    ("canvas", "window"),
    ("window", "toolbar"),
    ("toolbar", "window"),
]
tool_manager_implicit = [
    # from callbacks
    ("figure", "toolmanager"),
    # from UI
    ("window", "toolmanager"),
    ("toolmanager", "window"),
]

g1 = nx.DiGraph(connections + tool_manager)
g2 = nx.DiGraph(connections + tool_manager + implicit + tool_manager_implicit)

fig, (ax1, ax2) = plt.subplots(2, figsize=(5, 7), layout="constrained")

for g, ax in zip((g1, g2), (ax1, ax2)):
    nx.draw_networkx(
        g,
        node_color=[(0, 0, 0, 0)],
        node_size=2000,
        pos=nx.nx_agraph.graphviz_layout(g),
        bbox={"color": "w", "alpha": 0.5},
        arrowstyle="->",
        connectionstyle="Arc3, rad=0.1",
        ax=ax,
    )
ax1.set_title("direct")
ax2.set_title("implicit")
plt.show()

so

Part of the complexity is we have both toolbar and toolmanager than know about different things!

There are also a (maybe fair) distinction between direct connections (where a class we control has an explicit reference to another class we control/intentional created) and the implicit connections we get from either the UI toolkit or from callbacks.

I'm like 90% that these are completely correct.

@timhoffm
Copy link
Member Author

timhoffm commented May 1, 2022

Superseeded by #22925.

@timhoffm timhoffm closed this May 1, 2022
@timhoffm timhoffm deleted the uniform-manager-creation-thof branch July 19, 2024 11:34
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.

4 participants