Added API to insert and remove children

[samschott]

Fixes #30

In addition to adding children to a widget, this PR adds methods to remove and insert them. The syntax mirrors the remove and insert syntax of Python lists.

No errors are raised when attempting to remove a widget which is not a child or to insert a widget which is already a child. In addition, the method add has been adapted to only add new children and ignore those which are already present.

This PR requires corresponding API changes to travertino, submitted here.

Currently, only the Cocoa backend has been updated.

Example code to test the new API:

import toga
from toga.constants import COLUMN, CENTER
from toga.style import Pack


class ExampleApp(toga.App):

    def startup(self):

        self.button_add = toga.Button(
            label='Add image',
            style=Pack(padding=10, width=120),
            on_press=self.add_widget,
        )

        self.button_remove = toga.Button(
            label='Remove image',
            style=Pack(padding=10, width=120),
            on_press=self.remove_widget,
        )

        self.button_insert = toga.Button(
            label='Insert image',
            style=Pack(padding=10, width=120),
            on_press=self.insert_widget,
        )

        icon = toga.Icon('')
        self.image_view = toga.ImageView(icon, style=Pack(padding=10, width=120, height=120))
        self.box = toga.Box(
            children=[self.button_add, self.button_remove, self.button_insert],
            style=Pack(direction=COLUMN, alignment=CENTER, flex=1)
        )

        self.main_window = toga.MainWindow()
        self.main_window.content = self.box
        self.main_window.show()

    def add_widget(self, sender):
        self.box.add(self.image_view)

    def insert_widget(self, sender):
        self.box.insert(1, self.image_view)

    def remove_widget(self, sender):
        self.box.remove(self.image_view)

def main():
    return ExampleApp('Example App', 'org.beeware.app.example')


if __name__ == '__main__':
    main().main_loop()

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

[samschott]

Hm, I can't do much to make the CI happy here. Method names such as constraintWithItem_attribute_relatedBy_toItem_attribute_multiplier_constant_ just result in long lines. Since the argument attribute appears twice, I can't rewrite the objc method to take keyword arguments either.

[samschott]

I also realised that there still is an issue with the Widget.add method. It doesn't properly work when adding children to a widget which is the content of another widget (e.g., ScrollContainer).

Widgets which are content, either of a window or of another widget, currently have container = None. This makes sense because the their layout is not determined by the window but rather their direct parent (?). However, the attribute names are very misleading because container is the semantic counterpart of content while parent is the counterpart of child.

Currently, when adding a child to a widget which does not have a container, the code falls back to the window content as container:

def add_child(self, child):
    # if we don't have a window, we won't have a container
    if self.interface.window:
        child.container = self.container or self.interface.window.content._impl

As a result, it becomes a subview of the window. One should instead fall back to use the parent's content as container (i.e., the widget itself).

[samschott]

And one more thing: With the current implementation of Widget.add, the constraints of the added child are correctly taken into account but the constraints of the parent (padding, etc) are ignored. I am struggling to find out why. @freakboy3742, maybe you have an idea?

Edit: To clarify, paddings will be applied relative to the window and not the box that the widget has been added to.

[samschott]

I think I tracked down the reason for ignoring the parent's padding. The origin of the child layout is not set correctly because travertino.layout.BaseBox will only update the children's origin if the parent's padding has changed. But the parent's padding of course remains unchanged when we are just adding a new child.

Here is the "offending" code from content_top (lines 122 to 126 in BaseBox):

def content_top(self, value):
    if value != self._content_top:
        self._content_top = value
        for child in self.node.children:
            if child.layout:
                child.layout._origin_top = self.absolute_content_top

Maybe I am just new to travertino and the styling approach but the code is a bit of a maze to an outsider.

I have now removed the check for value != self._content_top in the PR for travertino.

[freakboy3742]

This all looks fundamentally sound. However, between this and your previous PR with adding new content, I think we're reaching a point where we need a test app in the examples folder that allows testing of these features. The sample app you've provided is more than enough - we just need something that we can use as part of release testing to validate that we haven't broken anything.

Given that you've flagged the interaction with scrollview as an edge case, it would seem appropriate to include a scrollview in that demo.

Longer term, I'd like to start merging some of these test cases into a single unified "prerelease testing app". This would be an app that can be started, and the app itself contains a clear workflow and in-GUI instructions to validate that it's operating correctly (e.g., text labels that say things like "press the button, this text should turn red" or "resize the window - the button should expand with the size of the window").

[samschott]

I have now added a test app that can add both an image to a box or labels to a box in a scroll container. Please consider it work in progress - the user interface is a bit vague about what will happen when pressing the buttons.

The example shows that I am still struggling with the ScrollContainer. The Labels are correctly added to the content of the ScrollContainer (a Box, referred to as the Box in the following). When adding a Label, its container is correctly set to the Box, refresh is called on the Box (since it is the Label's root) and the constraints of both Label and Box are updated correctly. However, the Labels are not correctly laid out on screen until I call refresh on the window content or manually resize the window. And I really cannot find out why this should be necessary. As far as I can see, all dimensions and paddings have already been correctly translated to Cocoa constraints.

If you find some time, could you maybe have a look at it? I may be missing something obvious...

[samschott]

Also, there is still one "issue" with the current implementation of add or refresh, depending on user expectations: The minimum window size is not updated when adding new widgets. The minimum window size is currently computed when calling toga_coca.Window.show and therefore does not get updated when calling refresh on a widget.

One could work around this by moving the calls to determine the minimum window size from toga_coca.Window.show to a separate method toga_coca.Window.refresh and calling this method every time a widget is added / removed. This would also fix the issue described above but circumvent the carefully constructed toga.Widget.refresh API.

[samschott]

Apologies for the long wait. I have finally fixed the issue with ScrollContainer - somewhat brute-force by refreshing the entire window when adding / removing children.

I have also renamed the example to layout_test so that it can be expanded towards more general tests and does not conflict with the box example.

Could you have another look at this PR?

[freakboy3742]

This is definitely heading in the right direction - there's some really solid clarification here in the general lifecycle of widgets and subwidgets. However, it's not quite right yet - if only because the demo app is currently raising errors for me. Some details/questions inline.

Given some of the edge cases , it might also be worth adding a "reparent" option in the demo app. I'm thinking maybe two side-by-side vertical boxes in the main layout; and once you've added the image, "reparent" moves it to the end of the list of widgets, from one box to the other. That would verify whether reparenting works, and would ensure layouts are being re-evalauted as well.

[freakboy3742]

Running the demo app, this line raises errors about super() has no method insert.

[samschott]

This PR requires beeware/travertino#10 to work. That PR adds insert and remove methods to Node, tests those methods, and fixes an issue where a widget's origin is not recalculated when it is added as a child.

[freakboy3742]

/me facepalms. My apologies - I completely forgot about that dependency.

[samschott]

I have now decided that automatic reparenting on add is the most user friendly solution. Therefore, a widget will be removed from an existing parent when added to a new one.

I also improved the example:

1. Buttons are disabled when they don't do anything.
2. The icon is by default added to the scroll box.
3. Reparenting moves it to the button box or back, depending on where it is.
4. Removing it always works, no matter where it is.

[freakboy3742]

This is looking really good; essentially ready for merge.

I've flagged one small potential improvement inline; the other big change that will be required is a bump on the travertino version, as this code won't work with travertino 0.12.

One other small detail: This PR is still marked as a WIP. Is there additional features/changes you were hoping to include? If not, I'll cut a travertino release, which will enable you to bump the travertino dependency, and we can merge this.

[freakboy3742]

/me facepalms. My apologies - I completely forgot about that dependency.

[samschott]

I've removed the WIP tag, this should be ready now. And good catch about converting the O(n) check to O(1)!

[samschott]

Ah, of course the checks are failing when trying to install travertino...

[freakboy3742]

Ok - this is all looking good, and Travertino 0.1.3 is now out; so I guess it's time to merge!