Skip to content
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

Provide mechanism for "wrapping" a widget #3003

Open
HalfWhitt opened this issue Nov 29, 2024 · 4 comments
Open

Provide mechanism for "wrapping" a widget #3003

HalfWhitt opened this issue Nov 29, 2024 · 4 comments
Labels
enhancement New features, or improvements to existing features.

Comments

@HalfWhitt
Copy link
Contributor

HalfWhitt commented Nov 29, 2024

What is the problem or limitation you are having?

Toga-chart uses an interesting method of creating a custom / third-party widget. Rather than create a separate implementation layer from scratch, or subclass an existing Toga widget, it subclasses base Widget, but creates and stores a reference to a stock toga.Canvas widget. This allows it to provide its own external API, blocking (straightforward) access to all of Canvas's usual methods while being able to call them internally to do its charting.

To make this work, Chart grabs the inner Canvas's implementation: self._impl = self.canvas._impl. From this point onward, the outer Chart is essentially the "real" core-level widget; it's the one that has a style assigned, and it's the one provided to layouts and added as a child of containers.1 And it has the implementation from the inner Canvas, which handles all the native rendering.

Flow chart of how the inner and outer widgets connect to each other

This works fine for Chart, but there would be issues if the inner widget — Canvas in this case — ever tries to access attributes it would normally have, but are currently only set on the outer widget, like its window, parent, app, style, etc. This includes if for some reason the implementation needs any of this info, because its interface is still the inner widget. This could lead to some subtle and confusing bugs for end users, and none of this is currently tested in Toga.


1Currently, both the outer and inner widgets are given a style, but it only functionally matters on the outer one.

Describe the solution you'd like

Assuming this is a use model we want to support — and it certainly seems useful — I propose providing a documented "hook" for doing so, and abstracting away the details and edge cases as much as possible.

A WidgetWrapper class could inherit from base Widget and receive an existing widget class in its initializer. A user subclasses WidgetWrapper and provides the relevant inner widget class to super.__init__(). This is used to create an instance and attach it as the inner widget, as well as monkey-patching that instance so that relevant attributes like app and window alias directly to those of the outer widget. Further investigation would be needed to best determine exactly what subset of attributes this should cover, and if any special-casing is needed.

However it's set up, wrapped widgets could be parametrized into a variety of existing tests in addition to their creation mechanism being tested directly.

Describe alternatives you've considered

Expecting third-party users to manually create their inner widget and assign its implementation should work at least for straightforward cases, and they could do whatever special-casing is needed if they hit unexpected behavior.

Also, instead of making the inner widget's attributes pass through to those of the outer, one could instead set things up so that setting an attribute on one also sets it on the other. This is already being done in Chart, for app and window:

    @Widget.app.setter
    def app(self, app):
        # Invoke the superclass property setter
        Widget.app.fset(self, app)
        # Point the canvas to the same app
        self.canvas.app = app

    @Widget.window.setter
    def window(self, window):
        # Invoke the superclass property setter
        Widget.window.fset(self, window)
        # Point the canvas to the same window
        self.canvas.window = window

(I'm not sure they're actually coming into play in this case though; removing them doesn't seem to affect how Chart currently works, at least in the example.)

Additional context

Prior discussion in the comments on #2942, particularly #2942 (comment) and the next several comments

@HalfWhitt HalfWhitt added the enhancement New features, or improvements to existing features. label Nov 29, 2024
@freakboy3742
Copy link
Member

Agreed that this should be a fully documented pattern for a "proxy" widget wrapper.

It might even be something where we can publish an abstract base class that sets up all the infrastructure, and does everything except describe which impl will be instantiated (i.e.... a definition for _create()?).

@HalfWhitt
Copy link
Contributor Author

It might even be something where we can publish an abstract base class that sets up all the infrastructure, and does everything except describe which impl will be instantiated (i.e.... a definition for _create()?).

Requiring the user to define _create would be one way to do it... but if the answer is always "return the impl of the inner widget", I feel like it would make more sense to just ask the user what they want the inner widget to be.

@mhsmith
Copy link
Member

mhsmith commented Dec 4, 2024

From #2942 (comment):

Thank you for the context. My brain's hurting a little figuring out how and why toga.Chart works the way it does. I would have expected something like that to either subclass [Canvas]

An earlier implementation did subclass Canvas - the problem becomes that the API of Canvas then becomes public API for this new widget (specifically, the redraw method), which isn't desirable. Chart can essentially be thought of as an API façade over the toga.Canvas API.

Why isn't it desirable? It seems simple enough to document "use this public Chart API; don't call any of the Canvas methods directly".

@freakboy3742
Copy link
Member

Why isn't it desirable? It seems simple enough to document "use this public Chart API; don't call any of the Canvas methods directly".

In the case of canvas, it was literally the need to re-used the name redraw, and the contract on how/when it was used - whether it was a "user requesting that a redraw occur" or "canvas responding to changes that require a redraw", which required subtly different arguments and semantics.

More generally - from a practical perspective, while the written documentation might include this admonition, it's difficult to enforce at the IDE level, because the method will appear public, and an in-GUI docstring viewer won't provide any indication that it shouldn't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

No branches or pull requests

3 participants