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]

This was getting called when Button's implementation is created, but it presupposes the existence of the button's style — which isn't there until Button calls super.__init__(). It also gets called when the button's font, bounds, or icon are set, and commenting it out here doesn't affect any testbed tests, so it seems it's still getting sufficiently applied. (I'm betting there are a few spots like this in the other implementation layers, though.)

[HalfWhitt]

With the reordering I just did, this line no longer causes an issue. However... since leaving it out doesn't have any observable effect and the tests all pass, just leave it out I guess?

[freakboy3742]

_set_button_style() will be invoked by set_bounds, which will be invoked at least once after styles have been established, so I think you're right that the call in the constructor is a no-op (or, at least, not required). It may not have been in the past, if styles weren't being consistently re-applied after a font change, height change, or adding an icon.

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

[HalfWhitt]

Okay. I've changed and reverted several things over the course of this PR so far, so here's an updated summary of what's currently in it:

• Widgets create and assign their implementation in a _create() method.
  • For an existing user-created widget that subclasses a Toga widget and uses the same implementation, nothing has changed.
  • Existing user-created widgets that create their own implementation in their initializer, before calling super().__init__(), should still work, but will issue a RuntimeWarning.
  • Existing user-created widgets that create their own implementation after calling super().__init__() will, unfortunately, now raise an Exception. This includes toga-chart.
  • I think I'm going to defer the question of how best to specifically enable the widget-masking usage pattern for now. Simply assigning the inner widget's _impl to the outer one works in simple cases; if we want to provide a more robust hook for cases where the implementation needs to access various attributes of its interface, that can be a separate PR.
• The extraneous assignment of self.interface._impl = self and the style reapplication in the implementation layers have been removed.
• I've added a try/except in Pack.apply so that things won't crash and burn on backends that don't implement Font. (This was what was needed to get Textual passing its startup test.)
• Several slightly different versions of ExampleWidget and ExampleLeafWidget were defined in various test files, so I pulled them out into a separate utils file.
• In a previous ordering of things, I removed a self._set_button_style() from the startup of the Cocoa button implementation to avoid an error because Node.__init__() hadn't been called yet. That's no longer an issue — but as far as I can see, its absence has no visible effect. The tests all pass, and that method is guaranteed to be called when the style is first reapplied... so I've left it out.

I keep thinking this, but... "reapply" is honestly a misleading name for its method, especially since it's the first thing called on the style when a widget is created. It's really more of an "apply_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.

[freakboy3742]

_set_button_style() will be invoked by set_bounds, which will be invoked at least once after styles have been established, so I think you're right that the call in the constructor is a no-op (or, at least, not required). It may not have been in the past, if styles weren't being consistently re-applied after a font change, height change, or adding an icon.

[freakboy3742]

Would be worth marking this as deprecated as well - if any code is using it, that code should be notified of the potential problem.

[HalfWhitt]

That was my first thought too, but — does it do any harm? I actually reverted my initial change of the existing uses of applicator.widget; my thinking being that in the context of Toga, a node should always be a widget, so "widget" is a more specific (and for anyone reading Toga code, more obvious) name for it. Does that track?

[freakboy3742]

Yeah - I think that makes sense. Good call.

[freakboy3742]

Can't argue with the logic; but how did you discover this (or, more importantly, why didn't it occur before)? I'm guessing it's Web or Textual that was the problem; I'm wondering if the fix here is to require a bare-bones Font implementation as a minimum for a working backend, rather than making the applicator more complex.

[HalfWhitt]

It was for Textual, and it never came up before because Textual's Widget base initializer didn't call a style reapply. With this PR, the core initializer reapplies the style, meaning it tries it for all tested backends — including Textual, which threw a NotImplemented error. (I'm sure it would for web too, if it were tested at all.)

I don't know that I have any particular opinion on whether it's better to check in the applicator or require Font in the backend. The latter seems somewhat arbitrary, but it would streamline this bit in core.

[freakboy3742]

I'm not sure I'd say requiring a representation of Font is arbitrary; Textual is the only platform I can think of that doesn't have a strong concept of a font - and even there, there are console variants like oblique and bold. We require all platforms to have a concept of Window; I don't think requiring a stub concept of Font is that far off the same requirement.

[HalfWhitt]

Fair enough, that makes sense. Would a new requirement like that go under a removal changenote?

[freakboy3742]

No - there aren't any alternate backends, so it's only a concern for us internally at this point.

[freakboy3742]

Why move the factory to here? Since every widget will need a factory, and we're in control of when _create() is invoked, I'm not sure I see why it makes sense to set self.factory, rather than setting it in the base widget and then assuming it exists in _create().

Also - would it make sense to make the contract of _create() return the _impl? A backend might define other attributes, but if we require that "the" impl is returned, we can guarantee that every widget will have an attribute named _impl, rather than relying on the backend to use the "right" name.

[HalfWhitt]

Why move the factory to here? Since every widget will need a factory, and we're in control of when _create() is invoked, I'm not sure I see why it makes sense to set self.factory, rather than setting it in the base widget and then assuming it exists in _create().

Because different widgets — in particular third-party widgets — need to be able to customize where they get their implementations from. BitmapView, for instance, rolls its own factory, and Chart doesn't even need one.

Also - would it make sense to make the contract of _create() return the _impl? A backend might define other attributes, but if we require that "the" impl is returned, we can guarantee that every widget will have an attribute named _impl, rather than relying on the backend to use the "right" name.

That makes a lot of sense, I like it.

[freakboy3742]

Because different widgets — in particular third-party widgets — need to be able to customize where they get their implementations from. BitmapView, for instance, rolls its own factory, and Chart doesn't even need one.

That's true; however, in the Chart case, having the factory around doesn't hurt; and in the BitmapView case, right now it can be overwritten, and the long term game is to allow widgets to register themselves, so the system default factory will be useful. Moving it to the _create() methods means a lot of code churn to add it, and then take it out again in the future when we can rely on a system wide factory.

[HalfWhitt]

While we're creating a new API requirement: how do you feel about the name _create? I just pulled it from the existing code of NumberInput and PasswordInput, but I'm starting to think _create_impl (or something similar) would be clearer.

[freakboy3742]

I guess my counter would be "what else are you planning to create"? It's pretty much an existential method for the widget; on that basis, I don't mind _create() as a method name.

When I was reviewing, my first reaction was whether we should promote this to a public API - but I convinced myself that wasn't a bridge we needed to cross here. If it turns out there's value in making the method public, we can do that in the future; but the counterexample - that it's a bad idea to be public, because we need to change it as an API - is easy to walk back if it's private.

[HalfWhitt]

That makes sense — I was probably just overthinking it.

[freakboy3742]

Could we leave this assignment off? None should be the default value, even in Travertino 0.3.0; I'm not sure there's a gain from being explicit in this instance.

[HalfWhitt]

Ah, good point. A case of just following the template of what was there.

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