toga icon indicating copy to clipboard operation
toga copied to clipboard

Provide mechanism for "wrapping" a widget

Open HalfWhitt opened this issue 1 year ago • 4 comments

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 https://github.com/beeware/toga/pull/2942#issuecomment-2463845957 and the next several comments

HalfWhitt avatar Nov 29 '24 19:11 HalfWhitt

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()?).

freakboy3742 avatar Dec 02 '24 00:12 freakboy3742

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.

HalfWhitt avatar Dec 02 '24 02:12 HalfWhitt

From https://github.com/beeware/toga/pull/2942#issuecomment-2463723907:

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".

mhsmith avatar Dec 04 '24 22:12 mhsmith

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.

freakboy3742 avatar Dec 04 '24 22:12 freakboy3742