Modify container handling for GTK

[mhsmith]

[Originally from #1778 by @freakboy3742]

In the process of performing the audit of Button, it became clear that GTK had some fundamental issues with the layout of widgets, and the root widget in particular.

This PR performs a radical change to how layout is achieved on GTK, which fixes a lot of edge case bugs.

• A new TogaContainer object has been introduced. This is a GTK-specific object that is a GTK widget implementing the Pack layout.
• The Window has an internal Box layout that is used to store the toolbar, and the TogaContainer.
• Instead of the root widget of Toga layout also serving as the native container, all Toga content is added to the TogaContainer.
• The TogaContainer merges 2 historical concepts: Container and Viewport.
• Any change in size or exposure of the TogaContainer triggers a GTK layout. This is used to re-compute the minimum and preferred layout sizes, and to re-layout the container.
• Rehinting can't be reliably performed until a the TogaContainer (the widget implementing layout) is ready to be laid out. As a result, the Toga-level rehint() request doesn't do an immediate layout; instead, it adds the widget to a list of "dirty" widgets. When a layout occurs, all dirty widgets are actually re-hinted, ensuring that the hints are the right size going into a layout.
• The TogaContainer tracks whether it is dirty and needs a redraw; this property can be used by testing tools to evaluate whether all changes in style have been reflected in the layout.

We should also investigate whether this change (or platform-appropriate variations) should be made on other backends. I strongly suspect that the root widget handling is subtly wrong on most platforms (except GTK, at this point). As an interim measure, the implementation maintains an interface that allows for _impl.container and _impl.viewport, even though they are now the same object on the GTK backend.

• Fixes Making a window with certain widgets smaller doesn't work on Ubuntu 18.04 and 20.04 #1205
• Fixes Selection does not stay inside window when inside split container #1796

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

[mhsmith]

@freakboy3742: Can you explain why the two calls to make_dirty below are needed? They both seem to be requesting a layout as the result of another layout, and I guess the only reason this isn't causing an infinite loop is because, as it says here:

Calls to gtk_widget_queue_resize() from inside GtkWidgetClass::size_allocate will be silently ignored.

I tried commenting both of them out and testing with the following app, and everything seemed to work fine, including the minimum window size.

Example app

import toga
from toga.style import Pack
from toga.style.pack import COLUMN, ROW


INITIAL_SIZE = 2
MAX_SIZE = 5


class HelloWorld(toga.App):

    def startup(self):
        self.width_button = toga.Button("", on_press=self.change_width)
        self.height_button = toga.Button("", on_press=self.change_height)
        self.label = toga.Label("", style=Pack(background_color="cyan"))

        main_box = toga.Box(style=Pack(direction=COLUMN), children=[
            toga.Box(style=Pack(direction=ROW), children=[
                self.width_button,
                self.height_button,
            ]),
            toga.Box(style=Pack(direction=ROW), children=[
                self.label,
                toga.Label("", style=Pack(background_color="pink", flex=1)),
                toga.Label("", style=Pack(background_color="yellow", flex=1)),
            ])
        ])

        self.width = self.height = INITIAL_SIZE
        self.set_text()

        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = main_box
        self.main_window.show()

    def set_text(self):
        self.width_button.text = f"Width {self.width}"
        self.height_button.text = f"Height {self.height}"
        self.label.text = "\n".join(
            " ".join("X" for i in range(self.width))
            for j in range(self.height)
        )

    def change_width(self, button):
        self.width = (self.width + 1) % (MAX_SIZE + 1)
        self.set_text()

    def change_height(self, button):
        self.height = (self.height + 1) % (MAX_SIZE + 1)
        self.set_text()


def main():
    return HelloWorld()

[mhsmith]

See top-level review comment.

[freakboy3742]

This is a make_dirty() with no arguments, so it's marking the container as dirty. On GTK, this triggers a "queue_resize" event, which (eventually) causes the actual layout to occur.

In the GTK context, this results in a redundant layout calculation (the one 2 lines above the highlighted line); however, this call is the one that is required on other platforms.

To me, this is part of the problem with "container/viewport" abstraction. The refresh() API entry point is defined on Node, where it requires a viewport definition; but a good portion of the implementation in toga-core replicates the definition from Node so that the viewport can be injected.

The existence of refresh() on toga.core.Widget is also a source of confusion, with people using it as a public facing API in the hope of applying any style change. Admittedly, this has been needed due to various bugs; however, once we've fixed those bugs (and this PR and #1761 fixes a lot of those cases), manual calls to refresh() shouldn't be needed at all.

A lot of what refresh() has historically been used for could actually be replaced with make_dirty() on the container. If the places that are currently calling some_widget.refresh() was replaced with some_widget.container.make_dirty(), that would remove the redundant layout in the GTK case, and other platforms could implement make_dirty() as required (e.g., immediately invoking layout on the root widget of the layout with the appropriate viewport on Cocoa).

[freakboy3742]

Looking at the example you've provided - this make_dirty() won't be needed, because any change on the text widget that marks the text widget as needing a rehint (e.g., changing the text) will also flag the container as dirty, causing a layout. This specific mark_dirty() is needed for the case where a layout is happening but there's no widget-specific rehint requested. One example would be marking the text widget as style.flex=1. That will affect the layout, but not the hinting of a specific widget.

Again, this would be a situation where it would be preferable for the applicator to invoke mark_dirty() directly on the container, and have backends respond with a layout at the appropriate time (immediately on Cocoa; on a redraw for GTK).

[mhsmith]

As a result of the GTK restructuring, rehint always leads to a refresh on that platform. To obtain this useful behavior on all the other platforms, I've changed things as follows:

• rehint is no longer called directly from the core. Instead, the corerefresh method calls a backend refresh method.
• In most cases the backend refresh simply calls rehint, but the GTK backend delays the rehint until the native toolkit is ready, as in the previous version of this PR.
• On Android, the backend refresh will also be a good place to put the requestLayout call required to fix the problem I had a few days ago, but I'll put that in a separate PR.

I've added a new "resize" example app for testing all of this.

[mhsmith]

I suspect these event handlers may no longer be necessary. @freakboy3742: Do you know what they're for?

[freakboy3742]

The GTK "Show" event is the event that fires when the widget is first visible on screen; as I recall, it was possible (at least, previously, anyway), for the layout algorithm to fire when the widget hadn't yet been made visible, which resulted in initial sizes of (0,0). This call forced a rehint when the widget was made visible, to ensure the first visible size was sensible.

However, this may now be reundant, as displaying the container will cause a layout, and (I think?) implicitly rehint of all widgets.

[mhsmith]

I can reproduce the iOS testbed crash locally, but I don't understand what it means. @freakboy3742: Any idea?

[freakboy3742]

I can reproduce the iOS testbed crash locally, but I don't understand what it means. @freakboy3742: Any idea?

Yup - I've seen that one, and fixed it in the audit-button branch. It's effectively a segfault with a weird presentation (and sometimes, if the wind is blowing just right, it does occur as a segfault). The issue is that colors are being stored in a cache to remove the need to repeatedly re-allocate "RED"; but the object references aren't being retained correctly, so the instance is being garbage collected on the ObjC side.

I've pushed the fix from the button branch.

[freakboy3742]

There's some good conceptual cleanup here.

To make sure I've understood the broad strokes:

• Core widget.refresh() - Instructs the backend widget to refresh, then, passes the request to the root element in the layout tree to computes a layout, ensuring that the entire widget tree has been laid out.
• Backend widget.rehint() - recomputes geometry for the widget (or, queues such a request to occur as soon as possible)
• Backend widget.refresh() - On most platforms, does an immediate backend widget rehint. On GTK, marks the widget as dirty, which will cause a backend widget rehint to occur when the UI is next redrawn.

On the interface, when any widget property (like text) or style property (like font or flex=1) changes, that widget gets an interface refresh. This implies a rehint of that widget (soon, if not immediately), followed by a re-layout.

The problem I can see is that there's a redundant layout pass on GTK. Calling an interface level refresh() does a rehint, which marks the GTK widget and container dirty, queuing a redraw, which calls do_size_allocate, doing a layout. However, the interface level refresh also does an immediate layout, which calls set_bounds on each widget, marking the container dirty, queuing a redraw, etc.

My initial read on this is that the issue is the dual purpose being carried by refresh() on the core widget. That API endpoint is inherited from Node, and is a key part of doing layout. However, we don't want to do a layout on GTK unless we're actually allocating the container; on other platforms, calling Node.refresh() needs to follow directly after the rehint. It seems to me that the call to refresh the Node should possibly be invoked from the backend - either directly, as part of the refresh() method, or indirectly in the case of GTK. By extension, that means the "reflect these changes visually" API is _impl.refresh(), not core refresh(). Does that make sense?

[freakboy3742]

Flagging this because I'm not 100% sure - in the process of writing my original version of this PR, I came across a lot of commentary talking about the relationship between gtk_widget_size_allocate() and do_size_allocate() (e.g. in the docs for get_allocation) - effectively flagging that calling size allocation methods while you're doing a size allocation can be complex.

AFAICT, I think what you've got here is correct - you're getting the current allocation (indirectly, via the self.width and self.height properties), and then setting the new allocation on the next line; and in our case, we're not doing anything complex (just setting the provided allocation, as is, to consume all available space) - but I wanted to confirm that is what you think it's doing as well.

This comment is also possibly of interest.

[mhsmith]

Yes, that's the idea. And it sounds as if the "adjustment" mentioned in those links is about GTK-level margins and alignment, which we're not using.

[freakboy3742]

Details from an in-person conversation:

• Yes, there's a redundant GTK layout call in this pass; however, what is here is better than it was before, and 1 additional travertino layout isn't a huge overhead. We can revisit this when we do a more comprehensive cleanup pass on Container, and/or optimise the bigger picture layout issues (e.g., if I change the font, then the text, then set flex=1 in three API calls, I don't really need to do 3 rendering passes - I should be able to either manually suspend rendering, or Toga should be a little smarter about how/when rendering updates occur).
• The at_least(0) change is legitimate - When evaluating size for child widgets, intrinsic at_least(0) causes child layout to be evaluated with the "minimum possible" size, whereas no intrinsic size is interpreted as consuming all available width. The former is definitely the right behaviour; there's probably a parallel-universe fix that corrects this in Pack, but it's just as easy to ensure every widget has an intrinsic size.

[freakboy3742]

As noted in the previous comment, there's still a lot of work needed in container layout, and the potential for a number of optimisations - but this is a significant improvement, and leaves everything in a stable state.