Changed to updated Pack.layout() signature (without node)

[HalfWhitt]

Once beeware/toga#3061 is merged, Toga's Pack.layout() will expect a single argument (the viewport), and doesn't need to be passed the node.

Since Toga 0.4.8 doesn't have an upward cap on Travertino version used, it's always possible someone could update to Travertino 0.4.0 while not having updated yet to Toga 0.5.0. So this checks what parameters the method expects, and adapts accordingly. I've tested this locally... not sure if there's any good way to verify this in CI.

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]

Well I'd tested it from both versions of Toga, that is. Hadn't thought to try Travertino's tests. 🤣

While I don't know of a good way to directly test this "in" both Toga 0.4.8 and beeware/toga#3061, we can at least test here that it works with style classes with both layout() signatures, which should be essentially the same thing.

[HalfWhitt]

BaseStyle should probably have a placeholder layout method, like it does for apply...

For that matter, any reason not to make it an ABC with abstract methods?

[freakboy3742]

Mechanically, this looks fine; my concern is that this is in the refresh() method, which is a potentially hot part of the layout process, and the compatibility check will be invoked on every invocation.

Would it not be better to invoke self.style.layout(viewport), and catch the TypeError if the argument isn't there? That optimises for the "expected" case, and avoids the need to check for a Toga-specific argument name. It also means the "backwards compatibility" comments can be wrapped around just the parts that need to be deleted (which is a minor thing, but I'll guarantee will take at least 2 attempts to get right when the time comes to finalise the deprecation :-) )

[freakboy3742]

BaseStyle should probably have a placeholder layout method, like it does for apply...

For that matter, any reason not to make it an ABC with abstract methods?

The main reason is the runtime overhead of an ABC for a class that most people won't be implementing. For my money, an the overhead of an ABC makes sense when the class is something you expect end-users to implement; but in this case, it's a class that will be implemented by probably 2 projects (and both of them are us :-) ).

I could go either way on an empty/NotImplemented "layout" method; I guess it doesn't hurt to add (and there's a lower overhead, because it's an entry in the __dict__ table that is overwritten by the parsing process), but again, it won't actually help any of the actual users.

[HalfWhitt]

The main reason is the runtime overhead of an ABC for a class that most people won't be implementing.

I keep forgetting that there's runtime overhead, yeah that makes sense.

I could go either way on an empty/NotImplemented "layout" method; I guess it doesn't hurt to add (and there's a lower overhead, because it's an entry in the __dict__ table that is overwritten by the parsing process), but again, it won't actually help any of the actual users.

I think I'd like to add one, just as a little bit of internal reminder documentation for our future selves. Or at least my future self.

Mechanically, this looks fine; my concern is that this is in the refresh() method, which is a potentially hot part of the layout process, and the compatibility check will be invoked on every invocation.

Would it not be better to invoke self.style.layout(viewport), and catch the TypeError if the argument isn't there? That optimises for the "expected" case, and avoids the need to check for a Toga-specific argument name.

That was my first thought, but I worried that could potentially mask other TypeErrors on down the stack, the way that the except AttributeError in Travertino was hiding beeware/toga#2916 for who knows how long. (Sometimes I wish Python had a way to specify "an error in calling this function, excluding anything the function does".)

But I take your point about optimizing such a frequently called thing. What if I only make the check once, and cache the "which version to use" result?

[HalfWhitt]

The old test_refresh used root as the argument name, and safeguarding that possibility was the only reason I was checking specifically for the _deprecated_usage argument. But considering only Toga uses this, and Toga named the parameter node, I think it's safe to retroactively call that the "correct" API usage, with the test being an outlier. Therefore I've switched the logic check back to a plain "is node there?".

So uh... let me know if you think my "caching" mechanism is too hacky. The first time any node's refresh() method is called, it should check layout()'s signature and set its class's calling mechanism appropriately. Any subsequent calls to refresh() will use the stored method, avoiding the check.

I've made one other change to control flow here: I've put the call to self.style.layout() call inside if self.applicator:. I don't think there's any case in which a node could/should be laid out without an applicator, is there? beeware/toga#3061 assumes we're counting on having the style._applicator.node link available when style.layout() is called. And the change here doesn't make any existing Travertino or Toga tests fail...

[HalfWhitt]

I've thought better of my previous approach, and pushed a less-sorcerous method of checking and caching; instead of a method that's called once and then alters its own attribute name to point to a different method, now it's a straightforward boolean stored on the class and used if present.

Either version should work — and I imagine this version will technically be slightly slower, since this patch now introduces one additional conditional check in each call to refresh. I doubt one added check against a stored boolean is anything significant compared to the layout mechanism itself, though.

[freakboy3742]

I can see what you're going for here - but is this actually sufficient? At least in theory, you can have different Style instances, each of which could have different layout() prototypes. I agree that's unlikely, but it is possible.

On top of that, while this is caching the value as a property, it's accessing that property via a function call, which is essentially a version of the overhead we were trying to avoid in the first place.

I agree that catching TypeError is prone to catching the nested errors - my inclination for addressing this would be to use try/catch, and in the case that a TypeError raised, invoke signature a that point to try and determine if the problem is the signature of the method being invoked (looking for 2 non-underscore prefixed arguments). If it's a "new style" layout method and it raised a TypeError internally, that error can be re-raised; but if it's an old-style layout method, even if it does raise a TypeError internally (which is less likely as it's a pre-existing method), the signature of the layout method is something that needs to be updated.

[HalfWhitt]

I can see what you're going for here - but is this actually sufficient? At least in theory, you can have different Style instances, each of which could have different layout() prototypes. I agree that's unlikely, but it is possible.

I realize that's technically possible, but it could never happen with Toga unless one somehow had two different versions installed, and if anyone is for some reason defining their own style classes with different layout signatures, I think that would be asking for trouble. However, that's a moot point because...

I agree that catching TypeError is prone to catching the nested errors - my inclination for addressing this would be to use try/catch, and in the case that a TypeError raised, invoke signature a that point to try and determine if the problem is the signature of the method being invoked (looking for 2 non-underscore prefixed arguments). If it's a "new style" layout method and it raised a TypeError internally, that error can be re-raised; but if it's an old-style layout method, even if it does raise a TypeError internally (which is less likely as it's a pre-existing method), the signature of the layout method is something that needs to be updated.

I don't know why it didn't occur to me to inspect the error raised, that makes perfect sense.

[HalfWhitt]

I'm really looking forward to 3.9 EOL and the across-the-board availability of kw_only, so this dataclass implementation can get simpler.

[freakboy3742]

Looks good. I've tweaked the release note - although layout() wasn't formally defined previously, the fact that there's a deprecation path means we should warn anyone using that API. I've also slightly broadened the exception check so that it isn't dependent on a specific argument name; I think the risk of a false positive is less of a concern than the false negative.