Replace reapply() with bare apply()

[HalfWhitt]

The name of BaseStyle.reapply() doesn't really reflect what it does. The presence of apply() and reapply() implies they do at least roughly the same thing, except one is for the first time and the other is for subsequent uses — whereas the actual difference is that apply() applies a single property, while reapply() applies all of them. Ironically, it's the first of the two methods to be (successfully) called, once the applicator is assigned.

I'd like to rename reapply to apply_all. I'm also all ears to alternate spellings, if you think something else would be preferable.

EDIT: Based on discussion, this now modifies apply() such that calling it with no arguments replaces the use of reapply().

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

[freakboy3742]

As with #3159, a fine implementation of a feature... if we want to pursue that feature.

To my mind. apply() and reapply() as names aren't causing major issues at present. Maybe they could be a little clearer, but they're fairly deep internals that most users won't need to see, and they're not fundmentally wrong... and naming is hard 😄. In the places the methods are seen in Toga APIs, the existing naming makes at least some sense - Android needs to reapply all styles after a widget is constructed, for example.

If anything, it's problematic that they're visible methods at all - we've had plenty of examples of users in the past who are trying to get (for example) background colors to work on a widget that doesn't support them, and exclaim "I've even called reapply(), and it didn't work!!".

If we're going to go to the trouble of a rename, I'd rather a name that moves them out of the public namespace to remove any illusion that they should be user-callable methods.

[freakboy3742]

Also - this is another one where I'd be interested to hear @mhsmith's thoughts.

[HalfWhitt]

If we're going to go to the trouble of a rename, I'd rather a name that moves them out of the public namespace to remove any illusion that they should be user-callable methods.

I hadn't thought of that, but it's not a bad idea. apply, reapply, and even layout could all be underscored, leaving only the actual style properties publicly visible.

I guess it depends on where one draws the API line. These methods could be said to be part of Travertino's "public" API, and consumed by Toga... but since they're not designed to ultimately be interacted with by non-library code at all, it isn't ideal that they end up visible on Pack.

[freakboy3742]

If we're going to go to the trouble of a rename, I'd rather a name that moves them out of the public namespace to remove any illusion that they should be user-callable methods.

Wait - scratch that. I'm getting myself tied in knots between Node.refresh() and BaseStyle.apply(). It's the former that is/was causing confusion. Node.refresh() also has some other complications, because you can't add a refresh() API to widgets like Canvas without causing naming collisions.

[HalfWhitt]

Naming things certainly is hard...

Am I correct in guessing that in proposals like this, where the implementation is simple but the goal isn't certain, it would probably be smarter to open an issue first? I was erring on the "Show up with code attached to your idea" side, but I'm realizing now that probably doesn't apply as much when the code is mostly just renaming.

[freakboy3742]

Am I correct in guessing that in proposals like this, where the implementation is simple but the goal isn't certain, it would probably be smarter to open an issue first?

Generally speaking, yes. The only notable exceptions would be (a) when the subject has already been discussed as a side effect of some other change, or (b) when there's some question about technical feasibility (e.g., a complicated set of changes needed for compatibility reasons, where having a proof of concept is likely more helpful than a description of proposed changes).

[mhsmith]

I agree the rename makes sense, as the current name is a bit misleading. But adding underscores to all the non-public methods probably isn't worth the churn, especially since we have no indication that people are using them.

As I said in #3159, this method has never been in any documentation or example code, so I don't think it requires a deprecation cycle or a public change note.

[HalfWhitt]

As I said in #3159, this method has never been in any documentation or example code, so I don't think it requires a deprecation cycle or a public change note.

There's a difference though: unlike .apply(), Toga itself (at least pre-0.5) calls .reapply() in a bunch of places, so it will need a shim even though no end user is probably messing with it.

[mhsmith]

Good point – I guess even after locking the Toga and Travertino versions to each other, we still need to keep Travertino compatible with old Toga versions until a reasonable time has passed since the 0.5.0 release.

[freakboy3742]

Throwing this out there on the basis of "there's no such thing as a bad idea"...

Another possibility would be to modify apply() so that invoking it with no arguments means it applies all properties... I'm not sure I see a way to implement this without pushing the iteration logic down to the subclass implementation of apply()... but at a high level, I like the idea of only having one apply method, and that avoids the "what's the right name for apply all" question entirely...

[HalfWhitt]

Another possibility would be to modify apply() so that invoking it with no arguments means it applies all properties... I'm not sure I see a way to implement this without pushing the iteration logic down to the subclass implementation of apply()... but at a high level, I like the idea of only having one apply method, and that avoids the "what's the right name for apply all" question entirely...

This was the first thing I thought of, and in a vacuum, I'd prefer the API usage. It's definitely strange how it would interact with Pack defining its apply method....

Although, here's a thought. What if base apply applies all with no arguments, and raises NotImplemented if given one. Pack's job, then, is to just call super().apply() if it doesn't have arguments, and otherwise do its thing.

[freakboy3742]

What if base apply applies all with no arguments, and raises NotImplemented if given one. Pack's job, then, is to just call super().apply() if it doesn't have arguments, and otherwise do its thing.

Yeah, but that gets into all sorts of weird circular calls where a subclass is calling the superclass to call the subclass... and any historical usage will straight up break, with a warning raised by reapply() being the only point at which to warn the user. As much as I prefer the "single method" API, that sounds like a bunch of complexity we should be avoiding.

If we accept that every end-user writing an apply() method for their own style will need to have "if name is None: apply all" logic, it essentially reframes apply as a generic "apply this style" method, with the property name being an optimization for only applying part of the style. That's a little annoying, but I don't hate it.

We actually avoid a bunch of function calls in the apply all case since we won't need to call apply() a bunch of times. Plus, it should be possible to make backwards compatible, as the one place in Travertino that calls reapply() is already catching backwards compatibility issues, and direct calls to reapply() can also raise warnings.

[HalfWhitt]

I suppose there'd be nothing stopping us, even, from letting apply() take an arbitrary number of names, each of which it applies. Which... I dunno, might be useful somewhere? Just, while we're poking at ideas.

[HalfWhitt]

(I didn't actually change anything just now, only based it onto main.)

[freakboy3742]

I suppose there'd be nothing stopping us, even, from letting apply() take an arbitrary number of names, each of which it applies. Which... I dunno, might be useful somewhere? Just, while we're poking at ideas.

True - the boilerplate for an end-user's apply method essentially becomes:

def apply(target=None):
    if target is None:
         properties = self._PROPERTIES
    elif isinstance(target, (list, tuple)):
        properties = target
    else:
        properties = [target]

    for name in properties:
        ... apply `name`.

There's even some optimization opportunities, because you can make one call to set_font if any font property is mentioned, rather than calling applicator.set_font() once for each font property that is mentioned.

[HalfWhitt]

Yeah, it'll be good to not set the font four times on every single reapply!

[HalfWhitt]

Would it be better to rename this PR, commit a revert, and start over based on what we've discussed, or close this one and open a new one?

[freakboy3742]

Would it be better to rename this PR, commit a revert, and start over based on what we've discussed, or close this one and open a new one?

The whole thing will be turned into a single merge commit when it's accepted, so unless it's especially onerous, I'd be inclined to keep the discussion all in one place and update this PR.

[mhsmith]

def apply(target=None):
    if target is None:
         properties = self._PROPERTIES
    elif isinstance(target, (list, tuple)):
        properties = target
    else:
        properties = [target]

More convenient for both the caller and callee would be:

def apply(*properties):
    if not properties:
         properties = self._PROPERTIES

However, to avoid redundant applications of font, we might also need to combine apply with update to some extent.

[HalfWhitt]

More convenient for both the caller and callee would be:

def apply(*properties):
    if not properties:
         properties = self._PROPERTIES

👍

However, to avoid redundant applications of font, we might also need to combine apply with update to some extent.

That's an interesting one. Currently, calling any property's setter will apply it after setting the value. We'll need some way to avoid doing that on each assignment in update, in order to then do them all at once at the end. Could set a flag on the style, or give it a context manager...

[HalfWhitt]

Interesting. I'm not sure how declaration lines 147 and 260 aren't being called all of a sudden, when they're there to handle being assigned no value in the dataclass constructor... I think I'm beginning to feel you on the "troubleshooting doing anything novel with dataclasses" front, @freakboy3742.

I know the primary motivation for changing the style property syntax in beeware/travertino#141 was to get code completion of property names in IDEs when creating Pack instances... However, now that style properties will probably be declared directly on widgets most of the time, I'm starting to wonder if it's worth the complexity of using a dataclass just for that, considering the descriptors all work perfectly well without it.

[HalfWhitt]

Another thought: even if we were to abandon the use of dataclass for the time being, we could always revisit it once we drop support for Python 3.9. Consistent availability of the kw_only argument would remove at least one of the layers of abstraction and complexity over it...

[freakboy3742]

However, to avoid redundant applications of font, we might also need to combine apply with update to some extent.

That's an interesting one. Currently, calling any property's setter will apply it after setting the value. We'll need some way to avoid doing that on each assignment in update, in order to then do them all at once at the end. Could set a flag on the style, or give it a context manager...

That's starting to sound like an overlap with #2938...

Interesting. I'm not sure how declaration lines 147 and 260 aren't being called all of a sudden,

I'd be looking at the changes to the reapply/apply mocking - the apply mocking now affects more properties, so individual property validation may not be triggered.

I know the primary motivation for changing the style property syntax in beeware/travertino#141 was to get code completion of property names in IDEs when creating Pack instances... However, now that style properties will probably be declared directly on widgets most of the time, I'm starting to wonder if it's worth the complexity of using a dataclass just for that, considering the descriptors all work perfectly well without it.

Honestly, I'd be more inclined to accelerate the deprecation of 3.9. It's only got 8 months left regardless; if it's causing problems, I could almost get on board with 0.5 being 3.10+ only.

[HalfWhitt]

However, to avoid redundant applications of font, we might also need to combine apply with update to some extent.

That's an interesting one. Currently, calling any property's setter will apply it after setting the value. We'll need some way to avoid doing that on each assignment in update, in order to then do them all at once at the end. Could set a flag on the style, or give it a context manager...

That's starting to sound like an overlap with #2938...

Conceptually, at least. But I'm not sure they're actually intertwined. Layout calculations can beget further layout calculations, and they also have to happen all over again when things are resized. Styles, on the other hand, should stay the same until explicitly changed, so they shouldn't need to worry about being run asynchronously or deferred to once-per-loop. This should be much simpler (I say now)...

Interesting. I'm not sure how declaration lines 147 and 260 aren't being called all of a sudden,

I'd be looking at the changes to the reapply/apply mocking - the apply mocking now affects more properties, so individual property validation may not be triggered.

I think you're right; when I have time to really sit down with it, that's where I'll look first.

I know the primary motivation for changing the style property syntax in beeware/travertino#141 was to get code completion of property names in IDEs when creating Pack instances... However, now that style properties will probably be declared directly on widgets most of the time, I'm starting to wonder if it's worth the complexity of using a dataclass just for that, considering the descriptors all work perfectly well without it.

Honestly, I'd be more inclined to accelerate the deprecation of 3.9. It's only got 8 months left regardless; if it's causing problems, I could almost get on board with 0.5 being 3.10+ only.

I wouldn't say I know that it's causing problems, but... I guess that is itself the problem. It adds complexity to the implementation and testing, and it's one more layer to have to look through when troubleshooting.

I believe this is the only place I personally have tripped over 3.9 support, but I see that removing it would also simplify use of TypeAlias and entry points.

[HalfWhitt]

Turns out it was the ordering of applying dataclass and mocking apply, the latter of which explicitly assigns an __init__ to the class and was just preventing dataclass from generating one.

I've also split the mock for apply() in two, one each for what used to be apply and reapply. This isn't strictly necessary, but it separates by logical concern of what's being tested, and helps with some granularity Mock apparently lacks. It's easy to check that a mock has received exactly one call, with specified arguments. But if you check that it's gotten multiple calls, I don't know of a way to check those are the only calls it has. This makes the tests less conclusive in places where both apply() and apply(name) end up being called.

[freakboy3742]

But if you check that it's gotten multiple calls, I don't know of a way to check those are the only calls it has.

assert my_mock.mock_calls == [
    mock.Call(...),
    mock.Call(...),
]

[HalfWhitt]

...Well shit, I somehow missed that in the docs.

[HalfWhitt]

So I guess that's less necessary than I thought. I can change it back if it seems frivolous, though I think it still makes some of the tests' intent clearer.

[HalfWhitt]

Regarding the possibility of a "lock" to prevent redundant Font creation — while I think it seems relatively straightforward, I think I'd still rather tackle it in a discrete PR.

[freakboy3742]

...Well shit, I somehow missed that in the docs.

Oh - I agree it's 100% a gap in the API for mock. There should be an assert_has_calls(only=True) or assert_only_these_calls() method. assert_has_calls() has a flag that lets you control whether ordering is significant; so it doesn't seem like a huge stretch to add it, either.

It would make for a nice little CPython contribution project... but then we wouldn't be able to use the feature for 5 years, so the motivation to add it isn't high.

So I guess that's less necessary than I thought. I can change it back if it seems frivolous, though I think it still makes some of the tests' intent clearer.

I don't think it's frivolous at all - verifying that we're not applying a property multiple times (or similar) is an important part of what we're verifying here.

[HalfWhitt]

So I guess that's less necessary than I thought. I can change it back if it seems frivolous, though I think it still makes some of the tests' intent clearer.

I don't think it's frivolous at all - verifying that we're not applying a property multiple times (or similar) is an important part of what we're verifying here.

Well, what I meant is that we can verify that by using one mock and then comparing directly against its call_list. Splitting it up is just a more convenient and legible wrapper.

[freakboy3742]

Well, what I meant is that we can verify that by using one mock and then comparing directly against its call_list. Splitting it up is just a more convenient and legible wrapper.

Ah - I hadn't looked at the code yet to see what you've done.

I think I can understand where you're coming from, but I think that approach potentially misses some cases. Consider a case where due to some error, both apply() and apply("foo"); apply("bar") are invoked. We're only expecting the individual calls to apply foo and bar; so we assert the call list on _apply_names... and miss that _apply_all has been invoked. Essentially every time we assert apply_names(), we need to assert that _apply_all() hasn't been invoked, and vice versa.

Otherwise, the PR is looking fairly good; a couple of minor comments inline.

[HalfWhitt]

I think I can understand where you're coming from, but I think that approach potentially misses some cases. Consider a case where due to some error, both apply() and apply("foo"); apply("bar") are invoked. We're only expecting the individual calls to apply foo and bar; so we assert the call list on _apply_names... and miss that _apply_all has been invoked. Essentially every time we assert apply_names(), we need to assert that _apply_all() hasn't been invoked, and vice versa.

True. It's the same situation we were/are in, before applying this PR, with apply and reapply: tests that check one don't usually check the other, and most tests don't even mock reapply at all. So this maintains parity with what was previously being tested, and (mostly) how. But that doesn't mean it's a bad idea to make things more comprehensive.

[freakboy3742]

Looks good, except for a couple of errors of usage in some mock_call assertions, and a need for some clarity on one kind of those assertions.

[freakboy3742]

This is a nice set of cleanups - thanks!