Remove value parameter from style's apply() method

[HalfWhitt]

BaseStyle.apply() / Pack.apply() currently accepts two parameters, a name and a value for the property to apply. However, the correct value to pass is always the the value of that property on the style object in question. Thus I propose removing the value parameter.

I've also renamed the other parameter from property to name, in line with the consistent naming scheme I've been using in Travertino to distinguish between styles, properties, property names, and property values.

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]

This is a solid implementation of the proposed change; my question is whether the change is the right thing to do in the first place.

I can definitely see the position that passing in the value is, at some level, redundant.

However, it's also... not. With the exception of the iteration in reapply(), every case where apply() is being invoked, the code invoking has the current value. By not passing in that value, the attribute value needs to be recomputed - which means hitting a getter, possibly doing some argument name transforms... granted, this isn't a huge overhead, but it's also not nothing. When you add this up over a full style definition, you're adding a bunch of attribute lookup function calls... and not really gaining anything.

So - we do a bunch of code churn and deprecation to gain... code that is (marginally) slower as a result.

It also removes one possible use case - directly invoking apply() with a specific value. We're not actually doing this (nor can I see an obvious usage on the horizon), but the capability does exist.

You could even view passing in the value as part of decoupling the style definition from the applicator process. In that worldview, the fact that the value is being passed in explicitly is a good thing, because it means the process of applying a style value isn't directly coupled to the definition of that style value. Again, this is fairly marginal interpretation... but it exists, and it's faster than the alternative.

Ultimately, I'm not completely sold that the change is worth it. Is there anything specific motivating this, or is this more a case of noticing and cleaning up an apparent redundancy? I'd also be interested in hearing @mhsmith's thoughts on this.

[HalfWhitt]

With the exception of the iteration in reapply(), every case where apply() is being invoked, the code invoking has the current value. By not passing in that value, the attribute value needs to be recomputed - which means hitting a getter, possibly doing some argument name transforms... granted, this isn't a huge overhead, but it's also not nothing. When you add this up over a full style definition, you're adding a bunch of attribute lookup function calls... and not really gaining anything.

So - we do a bunch of code churn and deprecation to gain... code that is (marginally) slower as a result.

I'll admit, I hadn't thought of that. I do wonder how much of an effect it would actually have, in practice, and whether this might be premature optimization. Or... premature avoidance of de-optimization? 😄

I'm curious to profile a test app now.

It also removes one possible use case - directly invoking apply() with a specific value. We're not actually doing this (nor can I see an obvious usage on the horizon), but the capability does exist.

You could even view passing in the value as part of decoupling the style definition from the applicator process. In that worldview, the fact that the value is being passed in explicitly is a good thing, because it means the process of applying a style value isn't directly coupled to the definition of that style value.

Counterpoint: one could say the same thing about passing node as an argument to BaseStyle/Pack.layout(), which was removed in #3061. It's hard to imagine why, in the model Toga has adopted, you'd ever want a style to lay out a node it's not assigned to... but I'm not sure it's any more likely you'd want a style to apply a property with a value other than its own. (Worldview / architectural design aside, if it were specifically a matter of enabling some rare but necessary esoteric edge case, one could always make it an optional argument that uses the style's own value if not provided.)

Is there anything specific motivating this, or is this more a case of noticing and cleaning up an apparent redundancy? I'd also be interested in hearing @mhsmith's thoughts on this.

Mostly the latter; the name of reapply has been bugging me for a while now, and I thought I'd bring this up while I was at it (and while other Travertino things are changing for 0.5). I guess the only practical advantage I can think of is that it removes the ability to do it wrong when writing new code. I'll admit that doesn't seem incredibly likely to happen (although I can absolutely imagine Future Charles scratching their head for an hour wondering why something isn't being applied correctly).

[freakboy3742]

So - we do a bunch of code churn and deprecation to gain... code that is (marginally) slower as a result.

I'll admit, I hadn't thought of that. I do wonder how much of an effect it would actually have, in practice, and whether this might be premature optimization. Or... premature avoidance of de-optimization? 😄

That's definitely fair. I don't think this is/would be a major performance hole; but if we get to a place where it's a line ball decision, it's a notable difference.

Counterpoint: one could say the same thing about passing node as an argument to BaseStyle/Pack.layout(), which was removed in #3061.

True; but in that case node and self were literally the same object, so it was redundant to pass it in; and, in the micro-optimisation case, passing it in was (marginally) slower, because there's another value on the stack. In this case, value requires additional lookups under the new scheme.

[HalfWhitt]

I'll admit, I hadn't thought of that. I do wonder how much of an effect it would actually have, in practice, and whether this might be premature optimization. Or... premature avoidance of de-optimization? 😄

That's definitely fair. I don't think this is/would be a major performance hole; but if we get to a place where it's a line ball decision, it's a notable difference.

Also definitely fair!

True; but in that case node and self were literally the same object, so it was redundant to pass it in;

self was the style object, not the node / widget; the node parameter is replaced by a lookup to self._applicator.node. (Come to think of it, it's a little odd that the style maintains a direct link to its applicator but not to its node.)

If I had a nickel for every time either of us conflated styles and nodes, I would have quite a few nickels. I swear I was going cross-eyed working on combining the row and column layout methods. 😆

[HalfWhitt]

I've done a rudimentary performance test and found surprising results.

I made a test file that generates a bunch of widgets and styles, including deprecated names and an Invent synonym. I timed ten runs of it with the main branch, and ten on this one. These were the results, in seconds:

Main

10.58
10.64
10.62
10.62
10.64
10.57
10.58
10.65
10.67
10.61
Mean: 10.62

Parameter removed

10.33
10.20
10.26
10.30
10.25
10.19
10.25
10.18
10.23
10.24
Mean: 10.23

I have no particular insight on why, but it appears that removing the argument and doing more lookups is actually consistently faster. By a very small amount, anyway.

[freakboy3742]

I have no particular insight on why, but it appears that removing the argument and doing more lookups is actually consistently faster. By a very small amount, anyway.

I agree that's a surprising result. Out of interest - what OS and Python version is this on? I'm wondering if there might be interpreter-level optimisations that make the property lookup faster for some reason.

[HalfWhitt]

I have no particular insight on why, but it appears that removing the argument and doing more lookups is actually consistently faster. By a very small amount, anyway.

I agree that's a surprising result. Out of interest - what OS and Python version is this on? I'm wondering if there might be interpreter-level optimisations that make the property lookup faster for some reason.

• macOS Sonoma 14.6.1
• Python 3.13.0

Oh hey, I should probably update both of those...

[mhsmith]

Python dictionary lookups are highly optimized, and I doubt that adding or removing one from each style application would make any measurable difference. So if it simplifies the code and removes a possible source of confusion, I'm in favor of it.

As far as I know, 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.

[freakboy3742]

Python dictionary lookups are highly optimized, and I doubt that adding or removing one from each style application would make any measurable difference. So if it simplifies the code and removes a possible source of confusion, I'm in favor of it.

Ok - my objection was pretty weakly held; given it's faster, lets do it.

As far as I know, 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.

Agreed - and with 0.4.9 out, we've now got additional safety on upgrades.

[freakboy3742]

One minor tweak to the deprecation comment following the release of 0.4.9, but otherwise this looks good to go.

[freakboy3742]

Suggested change

	# 2025-02: Backwards compatibility for Toga <= 0.4.8
	# 2025-02: Backwards compatibility for Toga < 0.5

[freakboy3742]

Actually... there's some other comments in the same space that need to be updated in the same way. I'll piggyback those onto this PR.