Deprecate style.copy(applicator); rework style/applicator setting mechanism

[HalfWhitt]

Fixes beeware/toga#2916, fixes beeware/toga#2305

As discussed in beeware/toga#2919, the root cause wasn't in Toga, but in Travertino. Setting the applicator before assigning properties on the newly created duplicate style meant that those assignments were triggering apply, even though no implementation was yet available.

This passes both Travertino's tests as well as Toga's. CI will only verify the former, so double-checking my work on a local copy of the two together probably isn't a bad idea.

Speaking of which, that's something that seems to come up pretty often for me in working on what is primarily a subproject like this. I bet it would be complicated to set up some way to trigger running Toga's tests against a branch here, but it would be an extra bit of insurance... this bug would've been impossible to miss back in March.

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]

Actually now that I think about it, is there a reason the applicator needs to be set in the copy method? Couldn't the caller just set the applicator on the copy after making it?

[HalfWhitt]

Just for some more context of what's happening when, the tests in test_apply.py all behave pretty similarly:

def test_set_default_right_textalign_when_rtl():
    root = ExampleNode("app", style=Pack(text_direction=RTL))
    root.style.reapply()
    # Two calls; one caused by text_align, one because text_direction
    # implies a change to text alignment.
    assert root._impl.set_alignment.mock_calls == [call(RIGHT), call(RIGHT)]

This works as expected because ExampleNode._impl isn't mocked until after it's called super.__init__():

class ExampleNode(Node):
    def __init__(self, name, style, size=None, children=None):
        super().__init__(
            style=style, children=children, applicator=TogaApplicator(self)
        )

        self.name = name
        self._impl = Mock()

If you move self._impl = Mock() to before super.__init(), the mock records an extra call(RIGHT); the intended two during reapply and an extra time first, before Node is finished initializing. The equivalent happens for all the other tests in the file.

This extra call — which in the case of Widget always fails because there's no _impl — is what the except in Travertino has been hiding all this time.

I still don't 100% understand the intended behavior of all the internals at play here, but the way these tests are written leads me to believe those early calls are not intended behavior, right? If they are supposed to be doing something, then yeah, _impl needs to be set sooner. Either that or like you mentioned, an apply afterward.

[freakboy3742]

Actually now that I think about it, is there a reason the applicator needs to be set in the copy method? Couldn't the caller just set the applicator on the copy after making it?

I think the motivation to passing the applicator into the copy call was to ensure that the applicator is in place before the style values are set, so that all the properties being set are applied. This could also be achieved by applying all the values at the time the applicator is assigned - arguably, that would be a better approach, because at present, changing the applicator won't have the same effect as copying a style and passing in the new applicator.

I still don't 100% understand the intended behavior of all the internals at play here, but the way these tests are written leads me to believe those early calls are not intended behavior, right? If they are supposed to be doing something, then yeah, _impl needs to be set sooner. Either that or like you mentioned, an apply afterward.

If it makes you feel any better... I'm not sure I understand the intended behavior either... :-)

The current implementation definitely works... but it definitely appears that there's some implied behaviour and reliance on specific behaviours in Toga's usage of Travertino.

The core requirements are:

1. An applicator is needed to apply a style to a node.
2. We can't use an applicator fully until the node has been fully instiated.
3. We can set values on the style before the applicator is set.
4. We need all values of the style to be set using the applicator.

Trying to reverse engineer what I was thinking (mumble) years ago, I think I was trying to ensure that the applicator always existed, so you could always set properties on it, and then catch the consequences of an incomplete Node in the applicator. However, in retrospect, I think it might be better to use the existence of the applicator as the signal that a style can be applied, use the act of setting the applicator to apply the "initial" style, and then require that the node is ready to be applied onto at the time the applicator is assigned. Does that make sense?

Speaking of which, that's something that seems to come up pretty often for me in working on what is primarily a subproject like this. I bet it would be complicated to set up some way to trigger running Toga's tests against a branch here, but it would be an extra bit of insurance... this bug would've been impossible to miss back in March.

Agreed that a "canary" like this would be a worthwhile addition - since Toga is the main consumer of Travertino, it would be worth asserting that updates in Toga (or Travertino, for that matter), aren't going to break Toga on the next release.

[HalfWhitt]

So you're proposing that applicator become a property? I think that would make sense, and would go well with doing the same to style like I described in the latter half of my comment on the other pull request. Setting applicator always triggers a reapply, and setting style does so as well, but only if applicator is already set.

So, looking through the code, this is a sketch of what's currently happening:

WidgetSubclass.__init__(style)
|   
|   Widget.__init__(style=style)
|   |   
|   |   Node.__init__(style=style if style else Pack(), applicator=TogaApplicator(self))
|   |   |   
|   |   |   self.applicator = applicator
|   |   |   self.style = style.copy(applicator)
|   |   |   |   
|   |   |   |   BaseStyle.copy(applicator)
|   |   |   |   |   
|   |   |   |   |   dup.applicator = applicator
|   |   |   |   |   dup.update(**self)
|   |   |   |   |   # Each property added sends a call to Pack.apply, which fails
|   |   |   |   |   # with an AttributeError because the widget doesn't have _impl set yet.
|   |   |   |   |   
|   |   |   |   |   return dup
|   |   |   |   |___
|   |   |   |_______
|   |   |___________
|   |   
|   |   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.
|   |   |___________
|   |_______________
|___________________

That final self.interface.reapply() is, if I'm not mistaken, the place where the style is actually applied, and I imagine that's why it was never apparent that the earlier attempt was silently failing.

Now, currently the widget subclass is calling Widget.__init__ first, and then creating its implementation. It would be nice to put the final applicator assignment (and thus style application) in Widget.__init__, since it will have to happen for every widget. But we can't swap the order and create the implementation before Widget.__init__, since that's where the factory is being assigned.

As far as I can see, each widget always calls self.factory.NameOfThisSubclass. That could become getattr(self.factory, type(self).__name__) and get shoved into the base Widget initializer... except that would break any subclass of a Toga widget. Because of the shared logic and necessary order, I'm thinking it might still be worth factoring that out into a class attribute (or a read-only property?) of each widget subclass.

So I think we're looking at something like this:

WidgetSubclass.__init__(style)
|
|   Widget.__init__(style=style)
|   |   
|   |   Node.__init__(style=style if style else Pack(), applicator=None)
|   |   |   
|   |   |   self.applicator = applicator # which will have been passed in as None
|   |   |   self.style = style
|   |   |   # Style property triggers a copy when set.
|   |   |   |   
|   |   |   |   BaseStyle.copy()
|   |   |   |   |   dup.update(**self)
|   |   |   |   |   # Each call to Pack.apply will see a lack of applicator and do nothing.
|   |   |   |   |   
|   |   |   |   |   return dup
|   |   |   |   |___
|   |   |   |_______
|   |   |___________
|   |
|   |   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.
|   |_______________
|___________________

I will, of course, have to pay close attention that updates to Travertino are backwards compatible with Toga. One of the first things will be to deprecate the applicator argument to BaseStyle.copy().

Does all that seem like a reasonable starting point?

[freakboy3742]

So you're proposing that applicator become a property?

Correct.

I will, of course, have to pay close attention that updates to Travertino are backwards compatible with Toga. One of the first things will be to deprecate the applicator argument to BaseStyle.copy().

I don't think that's necessarily a requirement - it would be possible to accomodate the API that accepts applicator, and just assign applicator after all the properties have been assigned. However, I think it might be worth doing as a "why is this even possible in the first place" API cleanup.

Does all that seem like a reasonable starting point?

Yes - the summary of current/future state looks right to me, and the broad approach you've outlined looks like it makes sense.

[HalfWhitt]

I've generalized the style and applicator properties such that setting either one triggers a reapply, as long as the other is already set. That way it's up to the consumer to decide what order they want to assign things, as long as the node is ready for application when the second of the two is assigned.

I have two questions:

1. How should I handle the changenote for this? It's both a bugfix (for Toga), as well as a notable code alteration that includes a deprecation. Can Towncrier handle multiple fragments for the same number, like 224.misc.rst and 224.bugfix.rst?
2. I'm not crazy about the except AttributeError I'm currently using to make this backwards compatible with current Toga. It feels too broad, but I'm not sure how else to do it without reaching up into Toga specifics, namely checking for self._impl. I'm open to suggestions if you can think of anything better.

[freakboy3742]

1. How should I handle the changenote for this? It's both a bugfix (for Toga), as well as a notable code alteration that includes a deprecation. Can Towncrier handle multiple fragments for the same number, like 224.misc.rst and 224.bugfix.rst?

Yes. You can even have multiple fragments for the same number and type - 224.bugfix.1.rst and 224.bugfix.2.rst will generate 2 bugfix entries tagged against issue 224.

For a deprecation, using 224.removal.rst is preferred - it will appear in a section labeled "Backward Incompatible Changes".

2. I'm not crazy about the except AttributeError...

Comments inline as part of the review.

[freakboy3742]

Generally looks good. The one major issue is the one you've already flagged; comments inline.

[HalfWhitt]

It occurred to me that the bug this fixes for Toga was never in a released version of Travertino. As such, it doesn't seem like it makes sense to mention fixing it in the release notes for the next version, so I've removed that changenote. Let me know if I'm wrong.

[freakboy3742]

A couple of Qs about some late changes.

[HalfWhitt]

Running Toga's core tests against this branch generates a whopping 1,035 warnings from early style application. 🤣

[freakboy3742]

Just confirming whether you're ready for another review, or if you're expecting more changes.

[HalfWhitt]

Oh! Sorry, yes, I believe this is ready to be looked at again.

[freakboy3742]

Definitely in the final stretch. A couple of minor questions/touchups inline.

Also - as noted inline, it occurs to me that this set of changes may also be a fix for beeware/toga#2305. I'm not sure if we should mark this PR as closing that one, or if we wait until we bump Toga to point at a new Travertino release.

[freakboy3742]

I'm not sure the "normal usage" comment here is correct... or at least, it's open to a misleading intepretation. I think the interesting feature here is more that "when a new style is assigned to a node, any existing applicator on the node will be use for the new style" - which is more of a bug fix (see beeware/toga#2305), and independent of "applicator argument to copy deprecated". My inclination is to drop the "normal usage" comment here, and add a separate bug fix entry for being able to assign a new complete style to a node (which will also inherit any previous node applicator)

[HalfWhitt]

Hm, fair enough. I was just thinking in terms of Toga specifically, I think — and a Toga user should probably never encounter this anyway. And thank you for linking that issue! Didn't realize it was already being tracked.

[HalfWhitt]

Removed the parenthetical, added separate bugfix changenote, and edited that issue into the initial comment for the PR.

[HalfWhitt]

Oh, right. Had momentary amnesia about what you said about whether or not to mark this as fixing it. I think it makes sense... Toga only requires Travertino of at least 0.3.0, right? So as soon as this is included in a released version, it's available to current version of Toga. With the caveat that current installations aren't going to be magically auto-updated, of course...

[HalfWhitt]

I suppose it boils down to a sort of false positive question. Once Travertino 0.3.1 or 0.4.0 or whatever is released with this patch, it is entirely possible to have an installation of Toga 0.4.8 in which the bug is fixed / feature is added. And any new installation of it presumably will, unless the user specifically pins Travertino 0.3.0 as part of the install.

But it won't be guaranteed to be fixed on existing installations, unless the user updates Travertino, or until they update to the next version of Toga, which can require the newer version of Travertino.

I'm still inclined to think this counts, but as someone without much experience in project management conventions, it's not a hill I'd argue on if you disagree.

[freakboy3742]

I agree with your analysis - this PR will fix both bugs in Toga... but in a way that causes warnings in Toga. The catch will be how we release this in a way that minimises disruption to users. If existing Toga users are going to get a thousand warnings when they upgrade Travertino, we may need to do a near parallel release to make sure there's a clear way to avoid the warning.

[HalfWhitt]

I think that sounds like the way to go.

1. Finish Added composite property (and some nearby cleanups) #222
2. Finish Updated Pack properties to use new Travertino dataclass-based API toga#2443 (which can include Pack.font) and Restructure widget initialization toga#2937, but don't merge them yet
3. Release a new version of Travertino
4. Merge the PRs and release a new version of Toga
5. ???
6. Profit!

[freakboy3742]

That's definitely one approach; the downside being that it will be difficult to avoid some users getting the warnings from Travertino.

The other possible approach would be something like:

1. Land Restructure widget initialization toga#2937 in a way that it works with both Travertino releases.
2. Cut a Toga release
3. Complete the work on Added composite property (and some nearby cleanups) #222
4. Cut a Travertino release
5. Complete the work on Updated Pack properties to use new Travertino dataclass-based API toga#2443
6. Cut a second Toga release

That all hinges on (1) being possible; but if it is, this approach would give a little breathing room between the Toga release and the Travertino release, so the chances of people getting stuck in the middle zone is lessened. It also gives a little more space for (3) and (5) to be completed - and ironically, the longer they take to implement, the less the impact of the Travertino change will be.

[HalfWhitt]

Ah, that makes sense! I had assumed # 1 would be ungainly, but I tried it out just now and what do you know, it's totally doable.

[freakboy3742]

Looks good - thanks for the fix!