-
-
Notifications
You must be signed in to change notification settings - Fork 786
Exclude display=NONE nodes from layout #3323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@freakboy3742 Can you think of other ways this can/should be tested, or does this seem good enough? |
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks pretty good, and I think you've got most of the bases covered as far as testing is concerned. However, I've got 2 suggestions for additional tests (or clusters of tests) that I think would be worth it.
Firstly - do display and visibility play nice together? We've already had (and test for) issues where a grandchild of a non-visible widget is set visible=True; what happens when the grandparent is display=None and I set the grandchild visible=True? I think the logic works as written, but it's the sort of edge case that warrants an explicit check.
Secondly - would it be worth having a live check of this? test_display_none proves that the math is correct; but that's all theoretical. What we really care about is whether widgets are actually in the right place, and are visible/non-visible as appropriate. #2447 is effectively a manifestation of this - a case where the implementation of visibility potentially undoes all the good work done by Pack.
If it turns out that there is an issue with the one or more backends, then we don't have to include the "live" test in this PR (or, we can include it, and skip it on the platforms where there are known bugs); but a live test would give us a lot more confidence that the style language is doing the right thing.
|
Not sure why I closed this — pretty sure I only meant to mark it as draft. |
|
Of note: I just happened upon the toga/travertino/src/travertino/layout.py Lines 14 to 20 in d9ce422
Other than being initialized to |
I've parametrized
displayinto the existing tests forvisibility, and added a new test to verify that a non-displayed widget is removed from the layout. I'm not entirely sure if it's sufficient, or if there are layout situations that could behave differently and should be tested as well.PR Checklist: