Fix minimum size in gtk backend #1293
Conversation
|
@freakboy3742 could you please take a look at this |
…fied width or height
Codecov Report
|
There was a problem hiding this comment.
Cosmetically, the layout behavior does appear to be faster, and fixes a couple of the problems you've described in the bug report. I've flagged a couple of smaller issues inline, and there are some problems I could nitpick with behaviors that this code does/doesn't exhibit, but... those are secondary to the bigger problem:
I have no idea what is going on with this PR.
The fact that the layout of box needs access to travertino is the first red flag - it means the layout is hard coded to the internals of Travertino as a layout engine. This misses the reason why the style engine is abstracted away from the layout process.
The second red flag is that the code also references the native details of specific widgets, missing the reason why there is an abstraction between the implementation and native layer.
Algorithmically, it's iterating over rows and columns, and seems to be doing part of the work that is the responsibility of the layout. Maybe there is an underlying plan to what you're doing here... but it's impossible to tell, because there's no broader description to why you've done what you've done. There's no inline documentation of the approach (or explanations for why certain branches in the code need to exist), and there's no "meta" description in the ticket describing the approach that you've taken, other than "it fixes a bug". Which... ok; maybe some manifestation of the specific bug you've described is no longer there... but at the cost of trampling the rest of the design, and making ongoing maintenance much more difficult.
The fact that this PR needs to add so much code is perhaps the biggest red flag.
One of the design goals (and eventual implementation goals) is that we will be able to replace Toga's Pack algorithm with Colosseum, an actual implementation of CSS. That's still a way from becoming a reality, but the separation of concerns that the code currently implements are a part of that architecture. The layout algorithm should be computing where widgets go; the GTK backend should be putting those widgets in the locations requested. The GTK layout shouldn't be participating in the calculation process at all.
Every other backend is able to follow this architecture. Layout is computed based on minimum sizes coming from hints; layout provides specific screen positions for each widget; and the backend then applies that layout by setting those literal positions. If there's a performance issue, it's with how often layout calculations are being performed. If there's a layout issue, then is with when constraints are being invoked (and what values those constraints assume). While it may be possible to make a specific behavior change by gutting the internals of the algorithm, that doesn't mean the change is correct.
So... this PR requires either a massive set of explanatory notes for why this is the only way to make GTK behave correctly, or a complete rethink - and I suspect it's the latter.
| @@ -106,7 +106,8 @@ def startup(self): | |||
| ), | |||
| ) | |||
|
|
|||
| split_container = toga.SplitContainer(content=[left_box, self.info_view]) | |||
| # make the SplitContainer flex to occupy available Box widget space | |||
| split_container = toga.SplitContainer(style=Pack(flex=1), content=[left_box, self.info_view]) | |||
There was a problem hiding this comment.
It's not clear why you've made this change. The old layout should be sufficient.
There was a problem hiding this comment.
The structure of the app is as following:
outer_box
|...Label
|...SpliterContainer
|...|...left_box (Box widget)
|...|...|...table (Table widget)
|...|...|...button_box (Box widget has 2 button)
|...|...info_view (MultilineTextInput widget)
Are we need to tell the SpliterContainer to occupy available space of outer_box? Why it will occupy available space without specify flex=1.
There was a problem hiding this comment.
It's not as straightforward as that. outer_box is also the main content of the window, and the layout must conform to the height of the window. The only content in the box is a label and a splitter container, and of those two, label has a fixed height. As a result, the splitter will must absorb all the remaining height. flex=1 should have no effect here - there no ambiguity over where the expansion weight can go.
By way of (weak) proof - macOS and Windows both render this layout the same, and adding flex=1 changes nothing.
There is perhaps a design argument to be made that the interaction between outer_box and window size, and whether it should be possible to specify a window axis size of "None" that causes the window to conform to the minimum possible size; but that's definitely in territory of "feature", rather than "bug".
| @@ -61,10 +61,13 @@ def set_font(self, font): | |||
| self.interface.factory.not_implemented('NumberInput.set_font()') | |||
|
|
|||
| def rehint(self): | |||
| # Does not use self.interface.MIN_WIDTH as a minimum width; because | |||
| # the min width of the widget will fail when number input width be | |||
| # larger than self.interface.MIN_WIDTH | |||
There was a problem hiding this comment.
Sure - this makes sense as a failure case; but what happens when width[0] is less than MIN_WIDTH? Wouldn't it make more sense to use max(MIN_WIDTH, width[0])?
There was a problem hiding this comment.
Yes, good and then we need to inform the Toga end user with that the minimum width of the Widget may be bigger than MIN_WIDTH to prevent the confusing, is this correct? Since the end user has an access to MIN_WIDTH.
There was a problem hiding this comment.
Why? Minimum is, by definition, a minimum. Of course it can be bigger. The purpose of MIN_WIDTH is to ensure that you can't end up with nonsense "3 pixel wide" input fields. If the minimum size suggested by the GUI toolkit is bigger than MIN_WIDTH, it's presumably for a good visual reason. This is primarily useful for widgets like Canvas, where reducing to 0x0 size doesn't make any sense, but it's a useful safety catch for other widgets.
And yes, MIN_WIDTH is externally visible; it's arguable that MIN_WIDTH should be _MIN_WIDTH, since it won't actually be useful to the end user.
There was a problem hiding this comment.
As you wrote, I expect notifying the user with following will be good:
If the minimum size suggested by the GUI toolkit is bigger than MIN_WIDTH, it's presumably for a good visual reason. So, we will use it as the minimum size.
| # width = self.native.get_preferred_width() | ||
| # Does not use self.interface.MIN_WIDTH as a minimum width; | ||
| # because the min width of the widget will fail when item width | ||
| # be larger than self.interface.MIN_WIDTH |
There was a problem hiding this comment.
As above - wouldn't max(MIN_WIDTH, width[0]) make more sense here?
There was a problem hiding this comment.
Yes, the same comment as above.
| # Now that the content is visible, we can do our initial hinting, | ||
| # and use that as the basis for setting the minimum window size. | ||
| # Now we will do our initial hinting, and use that as the basis | ||
| # for setting the minimum window size. |
There was a problem hiding this comment.
This seems like a fairly substantial change. My recollection is that GTK widgets didn't return a meaningful size until they are visible; am I misremembering, or is there some other interaction in the changes here that means that doesn't impact on behavior?
There was a problem hiding this comment.
My recollection is that GTK widgets didn't return a meaningful size until they are visible
Yes this is correct, exactly when it is realize. However with this changes or without it there is no a returned meaningful size at all because Gtk.Fixed wait us to set these sizes. For other widgets, when we add it to the container we call show_all() in base.py file so, as I understand, we get the meaningful sizes for these widgets.
| if min_width == 0: | ||
| for widget in self.interface.children: | ||
| widget._impl.rehint() | ||
| if str(type(widget)) == "<class 'toga.widgets.box.Box'>": |
There was a problem hiding this comment.
This... is not ever going to be a good idea. Best case, this check would be type(widget) == toga.widgets.box.Box... but even then I'd be having very strong opinions about why you'd be referencing a specific widget by name.
In simple words, this PR computes the minimum size of the In more details, for minimum width, the code in this PR does the following:
Hopping this is clear now. I will return to the first flag at the end of this comment.
Where this is happening? Is widget._impl.native.get_preferred_width()[0]called only if the widget is box, also for some edge cases that is handled with
I agree, it seems but does not actually does. I mean that the layout does not provide us with the
So, we need to doing the following:
So, I calculated the minimum size that will help us in using
I excepted the print statement will be enough to explain each branch in code. However, I gave in the beginning of this comment a description and I hope it is clear now. Also, I am opening to add any comment or description if there is some thing that is not clear.
I am really not happy with these a lot lines of code, but of course you agree with me in that we are in trade off between correct behavior and other things that you are mention; the not correct behavior is worst than things that you are mention. Also, I am opening to the ideas that simplify this implementation.
We override on the
Given the previous explanation, what do you think about this point and how we can solve it.
The following is quoted from the
However, if the explanation does not support this implementation, please let me know, and I will close it. |
Why is this even needed? The layout engine most definitely computes the minimum size of a box - but that's not part of the API contract here - it's an entirely internal detail. The entire purpose of the layout engine is to compute the actual size of the box, and the actual position of the box, so that the platform backend can apply that position to each widget. There's no decision to be made, or negotiation to perform. The GUI toolkit provides size hints, the layout engine computes the layout. The only task to be performed at this point of the code is to ask the layout engine for the x/y/w/h of each widget, and put the widget at that position in the window.
The intrinsic size is a hint for the specific widget, should such hints exist. If there isn't a meaningful hint... don't provide one. The "Box" is perhaps the least interesting widget for layout purposes, because there's nothing "intrinsic" about it - it's a container.
That's my point. You're accessing More generally, ask yourself this: Why is there no analog of this processing loop and special handling of Box in any other platform backend? This is the comparable implementation on Cocoa; The Windows version is even simpler, because it doesn't require the dance through constraints. I won't claim the original GTK implementation is correct - there's definitely something going wrong - but if anything, I'd argue that's because we're not tying the layout algorithm into the backend correctly |
|
It's clear now, thanks for your clarification. |
|
closing this because incorrect implementation |
Describe your changes in detail
As discuss in #1263 PR, this PR addressing the bug of window resizing and widgets size in
gtkbackend and fix them by recalculating themin_widthandmin_heightusing layout provided by the style engine.What problem does this change solve?
This Will fix issue #1205.
If this PR relates to an issue:
#1205
PR Checklist: