Skip to content
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

Exclude display=NONE nodes from layout #3323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HalfWhitt
Copy link
Member

@HalfWhitt HalfWhitt commented Apr 3, 2025

I've parametrized display into the existing tests for visibility, 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:

  • 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 HalfWhitt changed the title Exclude hidden nodes from layout Exclude display=NONE nodes from layout Apr 3, 2025
@HalfWhitt HalfWhitt marked this pull request as ready for review April 4, 2025 01:13
@HalfWhitt
Copy link
Member Author

@freakboy3742 Can you think of other ways this can/should be tested, or does this seem good enough?

@HalfWhitt HalfWhitt requested a review from freakboy3742 April 4, 2025 01:14
Copy link
Member

@freakboy3742 freakboy3742 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display = NONE doesn't work like CSS Confusion around style properties which accept NONE values
2 participants