Fix visibility and display support in Pack() #2435
Conversation
There was a problem hiding this comment.
Thanks for the PR - this is definitely a crunchy problem, so a huge thanks for taking this on.
As a minor administrative note - the current test failure being reported by CI is being caused because of the lack of a change note. If you add a change note, the rest of the test suite should run.
The handling of display=NONE appears to be broadly on the right track.
However, as noted inline, I'm not wild about the way you've tackled the sizing issue with visibility - I can't point you at a better approach off the top of my head, but I think we should definitely rule out that (a) there isn't a better approach, or (b) that the "visibility toggle" approach that you've used is actually functionally neutral. If visibility does impact on widget sizing, it seems inevitable that changing visibility will, at the very least, trigger a response from GTK's rendering, because that would be the logical side effect of a change in size. That response might not be visible, but it might result in additional rendering passes - which would be a performance hit as it's being triggered on every widget measurement.
Lastly, the one big piece missing is tests. You've ticked the "has been tested" box when you submitted the PR; I don't doubt you've done manual testing, but we require automated tests of any feature or change (unless there's a very good reason why something can't be tested). I suspect that when you resolve the change note issue, you'll discover that the tests start failing because you've added code paths that aren't being tested, both in the core API and in the GTK backend.
However, even if there wasn't missing coverage, there would still be a need for additional test cases. This PR proposes to the change the current behaviour of Pack, both at the core level, and in the specific GTK implementation. We need to validate that we know what the correct behavior is, and ensure that these changes don't regress in the future.
As an example of why this is needed - we have an existing test of visibility in the testbed. This test is passing on GTK; so either there isn't anything wrong with visibility, or our test is incomplete. It's evidently the latter - so we need to either extend that test, or add a new test that demonstrates the problem you've found. There's isn't a test of display=NONE behavior - so there's a whole new test case that is required.
If you need pointers on how to write those tests, we're happy to help; if you're able to get the test working on some platforms, but not all, and you're not sure how to fix the issue on the other platforms, yell out - if we've got a working fix on 1 platform, that will usually be enough to nerd-snipe me (or someone else) into extending the patch to other platforms.
gtk/src/toga_gtk/widgets/base.py
Outdated
| @@ -84,6 +83,18 @@ def get_tab_index(self): | |||
| def set_tab_index(self, tab_index): | |||
| self.interface.factory.not_implemented("Widget.set_tab_index()") | |||
|
|
|||
| def _get_preferred_size(self, native=None): | |||
There was a problem hiding this comment.
Two comments here:
-
I'd rather see this as a utility method that is given an explicit native widget, rather than the "default to self.native" behavior.
-
I'm really not comfortable with the "make visible, measure, then make invisible" pattern. Unless we can prove that this won't cause flicker as a result of measurement, I'd prefer to see an implementation that doesn't require altering the physical properties of the widget. That might mean we need to lean on lower level APIs than
get_preferred_width/height- I can't say I know what those would be off the top of my head, though.
If there really isn't a workaround to get a widget measurement using GTK's API, this needs some explanatory documentation so that future travellers understand what is going on and why.
|
@freakboy3742 Thanks for your review. Honestly, I will need some help to complete this change. (1) I've added a note to the change logs. (2) Regarding the addition or fixing the current test, I have no clue where to start. But I would like to agree about the implementation before writing some test to avoid any rework. (3) I've read the GTK documentation and I can't find another solution. If the widget is hidden, we need to make it visible in order to measure it. I changed the implementation to avoid toggling the visibility if the widget is visible to avoid extra calls. Visually, I don't see any flickering with current implementations. But I'm not fluent enough in GTK to know if another approach exists. |
No problems - that's what we're here for.
Honestly - regarding ordering: this is backwards. The correct behavior is completely independent of the implementation, and in an ideal world you always build the tests of correct behavior before you have the implementation, because that way the test is independent of the implementation. Having tests before implementing is preferable, because you start with tests that fail, and you keep implementing until the tests pass :-) As for how to write them: Toga has two sets of tests. The first is the core tests; these are a set of vanilla pytests, running against a "dummy" backend that doesn't have any visual manifestation. This is where the literal math of the Pack algorithm is validated - the The second is the testbed tests. This is the "live" test against actual backends, validating that the GTK/Cocoa/etc backends actually do what the core tests say they should. The contribution guide has a section on how the testbed tests work. In this case, we have a test of visibility; but (a) it's evidently it's either incorrect, or not validating a case that it needs to; and (b) there isn't an analogous test of
Empirically, the way to validate this will be to instrument Toga's own code to see how many resize/recompute passes are occurring. The GTK |
This is a first attempt to resolve two issues within the Pack() implementation.
The the visibility problem on GTK. It seams to be related to the fact we are making call to
show_all()which effectively make all children of the container visible. There are a couple of way to get this fixed. I decide to manually callshow()on every widget to fine tune when to show or hide it.To match the behavior of other platform and reserve enough space for the widget in the interface, when getting the preferred size, we need to make the widget visible. Otherwise, the return values are zero (0).
For display=NONE, the problem is mush simpler, I simply skip all the calculation in Pack() for every children with display==NONE. This exclude them from the layout.
PR Checklist: