Restructured widget initialization order

[HalfWhitt]

Fixes #2937

As discussed in beeware/travertino#224 (review), I've written this to work with up-to-date Travertino (with beeware/travertino#232 applied), but also be backwards-compatible with Travertino 0.3.0.

The way I've currently set it up, a widget class (in core) has an _IMPL_NAME class attribute that tells Toga what its implementation is named. In all cases here, that's the same as the core/interface class, but this way, subclassing a Toga widget won't suddenly break it.

Could be relevant to #2687, in that I've grouped the factory- and implementation-fetching logic for all widgets into the base Widget init.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

Huh, I hadn't noticed that the Textual base widget (and for that matter the web one too, though that isn't tested here) didn't already call reapply. Now that it's being called from the interface's initializer, it crashes. I currently know nothing about the Textual backend; will have to look more into this.

[HalfWhitt]

A thought occurs to me. Rather than simply change all of TogaApplicator's uses of self.widget to self.node, would you object to a property that aliases widget to node? It would just be syntactic sugar... but it would be nice to keep the more specific / self-explanatory attribute name, since TogaApplicator will presumably only ever be operating on widgets.

[freakboy3742]

I'm not sure I completely follow the motivation for the _IMPL_NAME change - is that a required part of this change, or is this more of a cleanup opportunity that you've identified? If it's a cleanup, I think I'd rather tackle that as part of #2687, rather than have all the code churn on a temporary partial fix.

[HalfWhitt]

Yes and no, I suppose. This whole thing is essentially a cleanup, isn't it? My goal with this PR was to do the Toga half of the reorganization described in beeware/travertino#224. With the reorganization I proposed:

Before

WidgetSubclass.__init__(style)
|   
|   Widget.__init__(style=style)
|   |   
|   |   Node.__init__(style=style if style else Pack(), applicator=TogaApplicator(self))
|   |   |    ...
|   |   |___________
|   |   
|   |   self._impl = None
|   |   self.factory = get_platform_factory()
|   |______
| 
|   self._impl = self.factory.WidgetSubClass(interface=self)
|   |   
|   |   ImplementationWidget.__init__(interface=interface)
|   |   |   
|   |   |   self.interface = interface
|   |   |   self.interface._impl = self
|   |   |   # Unnecessary, since this is already being assigned to self._impl above
|   |   |   
|   |   |   self.create()
|   |   |   self.interface.style.reapply()
|   |   |   # This is where all the styles are actually applied for the first time.
|   |   |___________
|   |_______________
|___________________

After

WidgetSubclass.__init__(style)
|
|   Widget.__init__(style=style)
|   |   
|   |   Node.__init__(style=style if style else Pack(), applicator=None)
|   |   |   ...
|   |   |___________
|   |
|   |   self.factory = get_platform_factory()
|   |   self._impl = getattr(self.factory, self.however_we_store_implementation_name)(interface=self)
|   |   |
|   |   |   ImplementationWidget.__init__(interface=interface)
|   |   |   |   
|   |   |   |   self.interface = interface
|   |   |   |   self.create()
|   |   |   |_______
|   |   |___________
|   |
|   |   self.applicator = TogaApplicator(self)
|   |   # Applicator property will trigger a reapply when set.
|   |_______________
|___________________

Beforehand / currently, each widget subclass calls Node.__init__(), then creates and assigns its implementation. The tail end of the implementation's base __init__ applies the style.

If, as proposed, Widget assigns its applicator at the end of its __init__, then the widget needs to have its implementation before that point; it can't happen after Widget.__init__. I put the implementation creation/assignment inside Widget.__init__ to minimize repetition, but then that means each class needs a way to specify its implementation class name, in a way that:

• Won't break when a user subclasses a widget (which means we can't just use the class name), and
• Allows subclasses to easily override the implementation name if needed (which means we can't supply it as an argument Widget.__init__).

I suppose I could instead try putting it in each subclass __init__, before calling Widget.__init__, and see if it causes any weird problems doing so before the node is initialized.

[freakboy3742]

Yes and no, I suppose. This whole thing is essentially a cleanup, isn't it?

True... except that there are downstream consequences.

In particular, while I can appreciate the "consistent logic" portion of moving the create impl part into the base class, that makes some uses cases difficult (or impossible). The core of the problem is that the widget is no longer responsible for determining how to instantiate it's own _impl... which I'm not sure is a good idea.

For an example in the wild - toga-chart doesn't have an _impl, because it's a composite widget - a wrapper around a toga.Chart.

Another example is a third-party widget like BitmapView - that's a "normal" widget in the sense of it having an _impl and native, but it's a custom widget, so it can't use Toga's factory (at least, not without the rest of #2687); and in the interim, there's no obvious workaround.

I suppose I could instead try putting it in each subclass __init__, before calling Widget.__init__, and see if it causes any weird problems doing so before the node is initialized.

That would be my inclination. There's still going to be a migration issue for subclassed widgets, but at least there will be a viable path for moving forward.

Better still would be to find a way that doesn't have this backwards incompatibility... but I don't have any immediate ideas for what that would look like.

[HalfWhitt]

For an example in the wild - toga-chart doesn't have an _impl, because it's a composite widget - a wrapper around a toga.Chart.

Another example is a third-party widget like BitmapView - that's a "normal" widget in the sense of it having an _impl and native, but it's a custom widget, so it can't use Toga's factory (at least, not without the rest of #2687); and in the interim, there's no obvious workaround.

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 Chart, or to subclass something like Box and contain the Chart and/or other widgets; subclassing base Widget while using the _impl of the wrapped Canvas is not something that would have occurred to me. Perhaps it would be worth formalizing or at least documenting the process(es) for making custom widgets... but I guess that's more of #2687's territory, and at some point I have to stop chasing the Matroyshka yaks ever downward.

I've made a first stab at reordering all the _impl creation before calling Widget.__init__. Since I already had to make one tweak in the Cocoa implementation, I pushed the commit here largely just to see if and how the other testbeds crash. Looks like CI hit a snag before even getting that far, though.

[HalfWhitt]

One thing to note about this arrangement is that it still prevents a subclass of an existing widget from choosing a different implementation. TextInput gets around this by calling self._create() instead of a hard-coded _impl creation/assignment, which lets PasswordInput override _create() to pick its own implementation. I don't think it would break anything existing to add the same mechanism to all other widgets... but if we do so, in order to expose it as a public mechanism for subclassing, perhaps it should lose the underscore and/or get a more descriptive name.

Then again, I suspect this might be replaced entirely by a different mechanism in #2687.

[freakboy3742]

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 Chart,

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.

or to subclass something like Box and contain the Chart and/or other widgets; subclassing base Widget while using the _impl of the wrapped Canvas is not something that would have occurred to me.

This would have been another viable approach - and one that would make sense if there was more than 1 widget being composed. However, while the API surface of toga.Box isn't prone to the same issue as toga.Chart - but it still represents an extra overhead of a widget that isn't actually doing anything (a box that contains... a box).

Perhaps it would be worth formalizing or at least documenting the process(es) for making custom widgets...

That's definitely something worth doing. We haven't formally documented the contract for a custom widget, but if doing so will simplify the work here, then we should consider it.

but I guess that's more of #2687's territory, and at some point I have to stop chasing the Matroyshka yaks ever downward.

That's more than fair :-)

Understanding the direction the widget contract is heading doesn't necessarily mean fully formally defining and documenting it - but I definitely appreciate the desire to close off the scope for this issue at some point.

[HalfWhitt]

That's definitely something worth doing. We haven't formally documented the contract for a custom widget, but if doing so will simplify the work here, then we should consider it.

Understanding the direction the widget contract is heading doesn't necessarily mean fully formally defining and documenting it - but I definitely appreciate the desire to close off the scope for this issue at some point.

I'm not averse to getting into a few more weeds, and it does seem inextricably linked to getting this PR done sensibly.

So far I see five potential paths for making a custom widget:

1. Subclass a container like Box, and include other widgets inside it.
2. Only subclass base Widget, writing an entirely new widget class and implementation.
3. Subclass an existing Toga widget, keeping the same implementation and only modifying the interface level (adding methods, etc.)
4. Subclass an existing Toga widget, and provide a different implementation (like PasswordInput does to TextInput).
5. Wrap an existing Toga widget and self-assign its implementation in order to essentially be it while masking its API, like Chart does to Canvas.

Presuming we want to support all five:

• 1 and 2 are straightforward, as far as supporting them; 1 simply includes internal widgets as-is, and 2 doesn't have any existing specific implementation to interact with.
• 3 and 5 are both reasons we can't infer the class name of the implementation from that of the interface, since these want to use the same implementation as the parent/composed widget.
• 4 means a subclass of a Toga widget needs to be able to override the implementation of its parent class — perhaps the way TextInput does with _create(), or something similar.
  • Satisfying the previous two points was my rationale for adding _IMPL_NAME, but I was overlooking 5...
• I now understand the reasoning for 5, as unintuitive as it was initially. I'm still kind of confused to see that the style is provided to both the widget and canvas initializer... This is both a clearly useful design pattern and also one with a number of odd, probably subtle interactions. That makes me think we should, at minimum, document the heck out of it, or preferably, provide a useable hook that can be trusted to handle such things correctly.

There is, of course, always the option of choosing to explicitly not support one or more of the above, the way PIL.Image explicitly doesn't support inherited subclasses. Not ideal, but worth remembering.

I think I also need to look more into how the platform/factory/implementation selection works in the first place, so I better understand what's being proposed in #2928.

That's all I've got in my head so far. Any thoughts/comments/direction at this point?

[HalfWhitt]

With regards to type 5, with some initial tinkering, pulling the wrapper functionality of Chart out into a separate class seems to work, at least for the included example:

class WidgetWrapper(Widget):
    def __init__(
        self,
        WidgetClass: type[Widget],
        id: str = None,
        style=None,
        *args,
        **kwargs
    ):
        self.wrapped_widget = WidgetClass(*args, **kwargs)
        self._impl = self.wrapped_widget._impl
        super().__init__(id=id, style=style)

    @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.wrapped_widget.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.wrapped_widget.window = window


class Chart(WidgetWrapper):
    def __init__(
        self,
        id: str = None,
        style=None,
        on_resize: callable = None,
        on_draw: callable = None,
    ):
        """..."""
        self.on_draw = on_draw
        if on_resize is None:
            on_resize = self._on_resize

        super().__init__(Canvas, id=id, style=style, on_resize=on_resize)

        self.canvas = self.wrapped_widget

(At least, it works as well as the existing code. Resizing the window is identically buggy in both... case in point for this being subtle and hard to get right.)

[freakboy3742]

That's all I've got in my head so far. Any thoughts/comments/direction at this point?

It looks like a good summary of the options/usage patterns to me.

Regarding (5); I agree it's odd, but if locking out that particular design pattern in favor of an alternative approach makes sense, I'm not fundamentally opposed, as long as the broader use case can be satisfied in other ways.

As for your alternate toga-chart - two questions:

1. What's the window resizing bug? That's a new one for me...
2. Is the extra layer required? Couldn't you get the same net effect by setting self._impl = self.canvas._impl?

[HalfWhitt]

2. Is the extra layer required? Couldn't you get the same net effect by setting self._impl = self.canvas._impl?

Huh... apparently that is indeed all you need. What were those app and window setter overrides for? They were in there, so I assumed they were necessary. But taking them out doesn't seem to change anything. I'd already found that there's similarly no need to supply the style to the wrapped widget, either. I guess this is simpler than I thought it was. Except possibly for whatever's causing...

1. What's the window resizing bug? That's a new one for me...

I've submitted it here: beeware/toga-chart#191
Haven't dug into it yet.

[HalfWhitt]

What were those app and window setter overrides for?

On further reflection: if we treat the outer widget as the "real" one — it's the one inserted into layouts, assigned to the window, given style properties, etc. — and the inner widget is an orphan whose methods and _impl are borrowed — then that causes a problem if a method of the inner widget ever tries to determine anything about its app, window, or style. Or if the implementation needs to know any of those things about its interface. I don't know how many times, if any, that currently happens in the codebase. But to ensure this model is trustworthy, we need to ensure it doesn't.

Another option — and I suspect this is what you were going for with the setter overrides I mentioned — is to monkey about with what the inner widget has access to. If we went that route, I'd be inclined to try a WidgetWrapper class to handle it all in one place; however, rather than setting it up so things are set on the inner widget when they're set on the outer, I'd rather monkey patch the inner widget's attributes like app and window to alias directly to the ones on the outer widget. And probably be readonly, since I imagine those shouldn't ever be getting set on the inner widget.

Either way, it would probably be a good idea to parametrize a wrapped widget into some existing tests.

[HalfWhitt]

I suppose I could instead try putting it in each subclass __init__, before calling Widget.__init__, and see if it causes any weird problems doing so before the node is initialized.

Who would've guessed there would indeed be weird problems! I haven't really done any Toga testing on platforms besides macOS, so it may take me a minute to set up emulators and virtual machines and track these all down.

We may need to start formalizing what an implementation is and isn't allowed to reach back up and access on its interface... or at least when it can do so. I don't have any recommendations on that until I look more into this, though.

[HalfWhitt]

I've taken the cue from TextInput and PasswordInput and put each class's implementation-setting logic in a _create method. This should satisfy 3 and 4 above — except it may break existing code that already subclasses a Toga widget and sets a different implementation. Not sure if there's a way to get around that.

Using _create means I can put the implementation creation where it was before, in between the Node initializer and the applicator assignment, which clears up all the worries about backend initializers trying to access not-yet-set attributes on their interfaces.

I'm not sure how to proceed with Textual... The addition of the reapply in the interface layer means it runs into unimplemented errors in the initial testbed setup, before tests would even start. Which... how does Textual pass the testbed, anyway? Is there a configuration somewhere that skips all tests when it's the backend?

[HalfWhitt]

(Requesting review less in the sense of "Here, I think it's done" than "How the heck does Textual work?")

[HalfWhitt]

Which... how does Textual pass the testbed, anyway? Is there a configuration somewhere that skips all tests when it's the backend?

Found it, in testbed/tests/testbed.py:

# Textual backend does not yet support testing.
# However, this will verify a Textual app can at least start.
if app.factory.__name__.startswith("toga_textual"):
    time.sleep(1)  # wait for the Textual app to start
    app.returncode = 0 if app._impl.native.is_running else 1
    return

The quick fix, of course, would be to comment this out until Textual has enough of an implementation to survive a style.reapply()... Is that an acceptable temporary accommodation, until Textual gets some more love?

Edit: Guess that wasn't even a quick fix after all.

[freakboy3742]

FYI - I'm currently on a crunch to get a talk ready for PyCon AU; it might take me a bit to get to reviewing this. I'll try to review it sooner, but if you haven't heard anything by the end of next week, give me a poke.

[HalfWhitt]

No worries, thanks for the heads up! I'm going to be unusually busy this week myself, and may not be around here anyway. Good luck with the talk!

[freakboy3742]

This looks really good, and about as painless as a change of this magnitude could be. It does make me wonder if this should be a trigger for a 0.5 bump in Toga. The exact sequence of this change, the Travertino change (and whether that's a 0.4.0 change), and the Rubicon ObjC change (see #2978) will be something we need to work out.

A couple of questions about the final form of the _create() interface, but otherwise, I think this is in good shape.

[HalfWhitt]

It does make me wonder if this should be a trigger for a 0.5 bump in Toga.

That doesn't seem unreasonable to me, seeing as how it does change/tighten the API for subclassing from Widget.

The exact sequence of this change, the Travertino change (and whether that's a 0.4.0 change), and the Rubicon ObjC change (see #2978) will be something we need to work out.

I believe we talked about doing this one before the Travertino change, to minimize the likelihood of users seeing lots of warnings. 0.4.0 definitely makes sense for Travertino, I think, especially if we wait till composite property is ready to go.

I've been following the Rubicon developments, though I'd be lying if I said I entirely understood them. I'll defer to you on whether/how that interacts with this.

[HalfWhitt]

Looks like I ran afoul of one of those intermittent testbed failures again.

[freakboy3742]

One minor update to a comment (so we know how old the backwards incompatibility code is); but otherwise I think I'm happy with this.

I'd like another set of eyes (@mhsmith) to take a look at this just to make sure I haven't missed/forgotten something through having been buried in this.

Just to make sure we're on the same page (and I haven't missed/forgotten anything in the landing sequence for this and related work):

• Land this PR. This will work with Travertino 0.3.0
• Land Added composite property (and some nearby cleanups) travertino#222 0 - not strictly required, but I think you've indicated it's the last "big feature" you want to get in
• Cut Travertino 0.4.0 release
• Add a PR that bumps the Travertino minimum requirement, and drops the backwards compatibility code for travertino 0.3.0
• Land any other big pieces for 0.5.0 (most notably - the Rubicon issues)
• Cut a Toga 0.5.0 release

Have I missed anything?

[freakboy3742]

It would also be helpful to see a "forked" version of this PR that deliberately uses the travertino main branch to prove that this PR works with the new branch as well. That PR will eventually become the "bump the travertino minimum version" update; but as an initial draft, it can install direct from GitHub main to prove the new code will be compatible.

[HalfWhitt]

Just to make sure we're on the same page (and I haven't missed/forgotten anything in the landing sequence for this and related work):

• Land this PR. This will work with Travertino 0.3.0
• Land Added composite property (and some nearby cleanups) travertino#222 0 - not strictly required, but I think you've indicated it's the last "big feature" you want to get in
• Cut Travertino 0.4.0 release
• Add a PR that bumps the Travertino minimum requirement, and drops the backwards compatibility code for travertino 0.3.0
• Land any other big pieces for 0.5.0 (most notably - the Rubicon issues)
• Cut a Toga 0.5.0 release

Have I missed anything?

Hm. Going off what you laid out in beeware/travertino#224 (comment), I was picturing something more like:

• Land this PR. This will work with Travertino 0.3.0
• Land any other big pieces for 0.5.0 (most notably - the Rubicon issues)
• Cut a Toga 0.5.0 release
  ...
• Land Added composite property (and some nearby cleanups) travertino#222
• Cut Travertino 0.4.0 release
• Cut a Toga release that requires Travertino >= 0.4.0

As you pointed out, this minimizes the likelihood of users getting stuck in the middle, with Toga 0.4.8 and Travertino 0.4.0 — which will work, but with lots of errors. If there's some time in between the former and latter parts, so much the better, in that respect.

It would also be helpful to see a "forked" version of this PR that deliberately uses the travertino main branch to prove that this PR works with the new branch as well. That PR will eventually become the "bump the travertino minimum version" update; but as an initial draft, it can install direct from GitHub main to prove the new code will be compatible.

It might take me a minute to figure out how to do that, but will do!

[freakboy3742]

Hm. Going off what you laid out in beeware/travertino#224 (comment), I was picturing something more like:

• Land this PR. This will work with Travertino 0.3.0
• Land any other big pieces for 0.5.0 (most notably - the Rubicon issues)
• Cut a Toga 0.5.0 release
  ...
• Land Added composite property (and some nearby cleanups) travertino#222
• Cut Travertino 0.4.0 release
• Cut a Toga release that requires Travertino >= 0.4.0

As you pointed out, this minimizes the likelihood of users getting stuck in the middle, with Toga 0.4.8 and Travertino 0.4.0 — which will work, but with lots of errors. If there's some time in between the former and latter parts, so much the better, in that respect.

Good catch - I'd completely forgotten that previous plan.

It would also be helpful to see a "forked" version of this PR that deliberately uses the travertino main branch to prove that this PR works with the new branch as well. That PR will eventually become the "bump the travertino minimum version" update; but as an initial draft, it can install direct from GitHub main to prove the new code will be compatible.

It might take me a minute to figure out how to do that, but will do!

From your current checkout of the widget_initialization branch, run git checkout -b new-travertino; then make the changes, commit, and push as a new (draft) PR. You'll get a second PR that contains all the changes in this one, plus the additional travertino change. Once this PR lands, merge main into the draft PR and make it final, and you'll get a PR that only has the 1 extra delta to fix travertino (or rebase and it will literally be 1 commit).

[HalfWhitt]

Thanks for the tip! And I found the syntax for pointing to Github from the Rubicon update. : )

Of course I found one more bug I'd missed, amidst checking against the two different versions...

[mhsmith]

Looks fine to me.

On the subject of the version numbers and the release sequence: should we set an upper limit to the Travertino requirement, as we already have for Rubicon? Otherwise it'll be impossible to make backward incompatible changes to Travertino without breaking all existing apps.

[HalfWhitt]

That seems reasonable to me. < 0.5.0, I guess?

[freakboy3742]

That seems reasonable to me. < 0.5.0, I guess?

Right now, it would be < 0.4.0, getting bumped to 0.5.0 when 0.4.0 is released. Getting the pin in place now provides some protection against the version bump for existing users.

[HalfWhitt]

I don't know if < 0.4.0 is really necessary, is it? This PR sets Toga up so that it will work with (what will be) Travertino 0.4.0.

[freakboy3742]

It's probably not necessary, but it's a safety catch that adds a layer of protection just in case we've missed something in our analysis; and more importantly, it would be consistent with the version pinning strategy we use elsewhere in the project.