Set default background color for widgets on iOS #3009

proneon267 · 2024-12-02T17:42:19Z

Fixes #767

This PR is broken out of #2484.

This PR sets default background color as TRANSPARENT for Box, Canvas, ImageView, Label, ProgressBar, ScrollContainer and Slider widgets on iOS.

Context for tests:

Currently all the 3 background color tests on the testbed cannot be enabled for all widgets, since there are problems associated with the widget background color on some platforms like on Android and Winforms. If we enable all 3 background color tests on every widget, then the testbed would pass on iOS, but would fail on other platforms. At the same time, we cannot correct their behavior to make them pass the tests as fixes for those platforms exist on separate PRs.
For example, If we enable all the 3 tests: test_background_color, test_background_color_reset, test_background_transparent on the Box widget then the tests would pass on iOS, but would fail on Winforms, since the fix for winforms exists on a separate PR.

Since behavioral changes need tests that confirms their behavior. So, to prevent a deadlock situation that would prevent this PR from being merged, I want to propose that we create a new issue to keep track of the widgets on which background color tests are not enabled. Since all 3 tests are enabled on most widgets so the list would be small. Afterwards, when all 3 dependent PRs are merged, we can enable all the 3 background color tests on all the widgets.

Issue for tracking background color tests on widgets: #3015

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

freakboy3742

There's the start of some interesting cleanups here, but:

This is all in the iOS backend, so we can lean into using iOS native colors. Trying to convert to Toga colors means we end up spending effort converting back and forth between the two formats for no real gain
The changes to Button don't seem right. The existing implementation explicitly sets the background to None because that's the desired behavior. I will admit that I haven't run the testbed with these changes; but I spent a lot of time getting these settings right when they were originally implemented, so I'm starting from a point of scepticism that these changes are required for button.
It's a little concerning that other than button, there's no changes to testbed tests. That means this change is either a no-op, or there are untested changes. If it's a no-op, we need to be clear why we're making this change.

freakboy3742 · 2024-12-03T00:24:40Z

iOS/src/toga_iOS/widgets/base.py

+
+        # Set default background color
+        try:
+            # systemBackgroundColor() was introduced in iOS 13


We no longer need this branch; as a result of the PEP 730 changes, iOS 13 is the minimum supported iOS version. The iOS platform docs should also be updated to reflect this.

Thanks, I've removed that branch.

freakboy3742 · 2024-12-03T00:27:20Z

iOS/src/toga_iOS/widgets/base.py

-            except AttributeError:  # pragma: no cover
-                self.native.backgroundColor = UIColor.whiteColor
+        self.native.backgroundColor = (
+            native_color(self._default_background_color)


This seems unnecessarily complex - doing a round trip from UIColor to toga.Color to UIColor just to get a (cached, and possibly stale) UIColor.systemBackgroundColor().

I agree, I've modified it to work with the native colors directly.

freakboy3742 · 2024-12-03T00:29:47Z

iOS/src/toga_iOS/widgets/canvas.py

@@ -15,7 +15,7 @@
 )
 from travertino.size import at_least

-from toga.colors import BLACK, TRANSPARENT, color
+from toga.colors import BLACK, TRANSPARENT, color as named_color


Worth noting that this is a little misleading - color here isn't just named colors - it's a toga color that will accept any valid string.

I've renamed it to toga_color

freakboy3742 · 2024-12-03T00:33:05Z

iOS/src/toga_iOS/widgets/button.py

-            self.native.backgroundColor = native_color(color)
+        super().set_background_color(
+            self._default_background_color if color in {None, TRANSPARENT} else color
+        )


AFAICT, this is a change in behavior. backgroundColor=None is a required call in this instance, and this new implementation doesn't provide that as an option.

I've restored it. But it's worth noting that backgroundColor=None produces the same result as backgroundColor=UIColor.clearColor:

backgroundColor=None

backgroundColor=UIColor.clearColor

According to: https://developer.apple.com/documentation/uikit/uiview/1622591-backgroundcolor?language=objc:

Changes to this property can be animated. The default value is nil, which results in a transparent background color.

But according to: https://developer.apple.com/documentation/uikit/uibackgroundconfiguration/3600317-backgroundcolor

If the value is nil, the background color is the view’s tint color. Use clear for a transparent background with no color.

So, I have set the background color of widgets which should have transparent background by default to UIColor.clearColor. But for button, I've set the default background color to None.

Ok - I think that interpretation of "None = Transparent" is what I was remembering. Based on the examples you've presented here, it looks like you've preserved the historical behavior.

proneon267 · 2024-12-03T11:49:36Z

Here are the preview of the widgets that this PR changes:

Test Code:

"""
My first application
"""

import toga
from toga.colors import PINK
from toga.style import Pack
from toga.style.pack import COLUMN


class iOSBackground(toga.App):
    def startup(self):
        """Construct and show the Toga application.

        Usually, you would add your application to a main content box.
        We then create a main window (with a name matching the app), and
        show the main window.
        """
        self.content = toga.Box(
            style=Pack(
                direction=COLUMN,
                flex=1,
                padding=0,
                background_color=PINK,
            ),
            children=[
                toga.Box(style=Pack(flex=1.5)),
                toga.Button(text="Button"),
                toga.Box(style=Pack(flex=1.5)),
                toga.ImageView(toga.Image(self.paths.app / "resources/imageview.png")),
                toga.Box(style=Pack(flex=1.5)),
                toga.Label(text="Label"),
                toga.Box(style=Pack(flex=1.5)),
                toga.ProgressBar(max=100, value=50),
                toga.Box(style=Pack(flex=1.5)),
                toga.Selection(items=["Alice", "Bob", "Charlie"]),
                toga.Box(style=Pack(flex=1.5)),
                toga.Slider(range=(-5, 10), value=7),
                toga.Box(style=Pack(flex=1.5)),
                toga.NumberInput(value=8908),
                toga.Box(style=Pack(flex=1.5)),
                toga.MultilineTextInput(
                    value="Some text.\nIt can be multiple lines of text."
                ),
                toga.Box(style=Pack(flex=1.5)),
                toga.TextInput(value="Jane Developer"),
                toga.Box(style=Pack(flex=1.5)),
            ],
        )

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


def main():
    return iOSBackground()

proneon267 · 2024-12-03T15:25:58Z

Regarding enabling background color tests on all the widgets, I cannot enable background color tests on the testbed for all widgets, since there are problems associated with the widget background color on some platforms like on Android and Winforms. If I enable all 3 background color tests on every widget, then the testbed would pass on iOS, but would fail on other platforms. At the same time, I cannot correct their behavior to make them pass the tests as fixes for those platforms exist on separate PRs.
For example, If I enable all the 3 tests: test_background_color, test_background_color_reset, test_background_transparent on the Box widget then the tests would pass on iOS, but would fail on Winforms, since the fix for winforms exists on a separate PR.

I know you have told me that behavioral changes need tests that confirms their behavior. So, to prevent a deadlock situation that would prevent this PR from being merged, I want to propose that we create a new issue to keep track of the widgets on which background color tests are not enabled. Since, all 3 tests are enabled on most widgets so the list would be small. Afterwards, when all 3 dependent PRs are merged, we can enable all the 3 background color tests on all the widgets.

freakboy3742

We're getting closer - and the sample project you've posted definitely suggests this PR is on the right track.

There's still some overly convoluted logic around the color reset (on button, in particular); I've flagged my concerns inline.

Regarding testbed testing - your explanation makes sense. From a process perspective, if this is something you've given thought to, it's helpful to include that context in the pull request description. You might think it's obvious given the history of the ticket - but a reviewer (read: me) is doing a lot of other things, and so the connection to "obvious" things might not always be so obvious.

freakboy3742 · 2024-12-03T23:37:03Z

iOS/src/toga_iOS/widgets/base.py

-    def set_background_color_simple(self, value):
-        if value:
-            self.native.backgroundColor = native_color(value)
+    def set_background_color(self, color, is_native_color=False):


This isn't a good idea. The API either takes a native color, or it doesn't. And given that the only usage that I can see of is_native_color=True... uses native_color() on the invocation, it seems to be completely unnecessary.

set_background_color() will receive a toga color from the core interface, but on the backend since we are working directly with the native UIColor, so any invocation of set_background_color() from the backend requires is_native_color=True. This is why in the previous review, I was specifying _default_background_color in the form of toga color instead of the native UIColor.

But since now we are directly specfying the _default_background_color in the form of native UIColor, we need the additional indicator is_native_color=True to prevent any incorrect interpretation while assigning the background color.

You were right.

freakboy3742 · 2024-12-03T23:37:54Z

iOS/src/toga_iOS/widgets/base.py

+            default_background_color = getattr(self, "_default_background_color", None)
+            if default_background_color is None:
+                default_background_color = UIColor.systemBackgroundColor()


Why not just use _default_background_color as the default in the call to getattr?

Also - it feels like there's some ambiguity here about what "None" means in the context of default_background_color - is it "transparent", or "system background"? Does it make a difference if the property is explicitly set to None, or the property is not defined at all? (it probably should - because it gives you a way to differentiate between two interpretations of "reset" behaviour - and that interpretation should be documented here).

I have changed it to use UIColor.systemBackgroundColor() as the default in the call to getattr.

On the backends, there are 3 different possible values for set_background_color():

None Default background color

TRANSPARENT Actual transparency

Color Actual color

But for Button, the historical behavior is "None = TRANSPARENT", which I have preserved in this PR.

freakboy3742 · 2024-12-03T23:49:33Z

iOS/src/toga_iOS/widgets/button.py

-            self.native.backgroundColor = native_color(color)
+        super().set_background_color(
+            self._default_background_color if color in {None, TRANSPARENT} else color
+        )


Ok - I think that interpretation of "None = Transparent" is what I was remembering. Based on the examples you've presented here, it looks like you've preserved the historical behavior.

proneon267 · 2024-12-04T03:14:08Z

I agree, I should have added context for not enabling tests. I have edited the PR ticket to include the context and will keep in mind to add context on new PRs.

I have created #3015 to keep track of the widgets on which background color tests are not enabled.

proneon267 · 2024-12-04T21:02:48Z

Current Progress:

Source code:

"""
My first application
"""

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


class iOSBackground(toga.App):
    def startup(self):
        """Construct and show the Toga application.

        Usually, you would add your application to a main content box.
        We then create a main window (with a name matching the app), and
        show the main window.
        """
        self.content = toga.Box(
            style=Pack(
                direction=COLUMN,
                flex=1,
                padding=0,
            ),
            children=[
                toga.Box(style=Pack(flex=1.5)),
                toga.Button(text="Button"),
                toga.Box(style=Pack(flex=1.5)),
                toga.ImageView(toga.Image(self.paths.app / "resources/imageview.png")),
                toga.Box(style=Pack(flex=1.5)),
                toga.Label(text="Label"),
                toga.Box(style=Pack(flex=1.5)),
                toga.ProgressBar(max=100, value=50),
                toga.Box(style=Pack(flex=1.5)),
                toga.Selection(items=["Alice", "Bob", "Charlie"]),
                toga.Box(style=Pack(flex=1.5)),
                toga.Slider(range=(-5, 10), value=7),
                toga.Box(style=Pack(flex=1.5)),
                toga.NumberInput(value=8908),
                toga.Box(style=Pack(flex=1.5)),
                toga.MultilineTextInput(
                    value="Some text.\nIt can be multiple lines of text."
                ),
                toga.Box(style=Pack(flex=1.5)),
                toga.TextInput(value="Jane Developer"),
                toga.Box(style=Pack(flex=1.5)),
                toga.ActivityIndicator(running=True),
                toga.Box(style=Pack(flex=1.5)),
                toga.Switch(text="Switch"),
                toga.Box(style=Pack(flex=1.5)),
            ],
        )
        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = toga.ScrollContainer(content=self.content)
        self.main_window.show()


def main():
    return iOSBackground()

LightMode	DarkMode

freakboy3742

The structure of the PR is looking good now; the only question is about the actual colors used (and in particular the secondary background color change)

freakboy3742 · 2024-12-04T22:18:06Z

iOS/src/toga_iOS/widgets/base.py

-            except AttributeError:  # pragma: no cover
-                self.native.backgroundColor = UIColor.whiteColor
+        default_background_color = getattr(
+            self, "_default_background_color", UIColor.secondarySystemBackgroundColor()


Why secondarySystemBackgroundColor? This is a fairly big change - and looking at the most recent sample screenshot, it definitely doesn't look right for switch, and it's arguable whether it's correct for TextInput, MultilineTextInput et al

proneon267 · 2024-12-05T06:18:22Z

If we use systemBackgroundColor, the widgets are not distinguishable from the background:

LightMode	DarkMode

I had used secondarySystemBackgroundColor since it made the widgets distinguisable and similar in appearance as the native settings app:

LightMode	DarkMode

But looking closely, I now realize that the UISwitch widget itself has a background color of UIColor.clearColor and is inside a UIView which has a non-transparent background color.

The same also seems to be true for Input-related widgets:

So, should we use UIColor.clearColor as the default for all widgets, or
should we use UIColor.secondarySystemBackgroundColor() as the default for input-related widgets and UIColor.clearColor as the default for other widgtes?

freakboy3742 · 2024-12-09T00:18:13Z

If we use systemBackgroundColor, the widgets are not distinguishable from the background:

Yes - but that's part of iOS's style guide. You can disagree with that style if you like (I know I definitely have my issues with the "minimalist" iOS style guide), but that doesn't give us liberty to "fix" it.

I had used secondarySystemBackgroundColor since it made the widgets distinguisable and similar in appearance as the native settings app:

Yes - but the native settings app is specifically on a different background color. It's the "form" layout that is providing the background color, not the widget. The UISwitch is the widget that makes this distinction obvious.

So, should we use UIColor.clearColor as the default for all widgets, or

should we use UIColor.secondarySystemBackgroundColor() as the default for input-related widgets and UIColor.clearColor as the default for other widgtes?

By default, we shouldn't be making any change from the "default" value. Functionally, that may mean we need to encode (or capture at runtime) the default; but the background color of the widget should be the same as if we did nothing programatically.

Set default background color for widgets on iOS

046d8de

freakboy3742 requested changes Dec 3, 2024

View reviewed changes

Remove unnecessary color conversion

481ea93

proneon267 added 2 commits December 3, 2024 06:21

Enable background color tests for slider widget

c7553ed

Revert enabled slider background color tests

f18cbf1

freakboy3742 requested changes Dec 4, 2024

View reviewed changes

Use systemBackgroundColor as default fallback

5813bc0

proneon267 mentioned this pull request Dec 4, 2024

Enable all background color tests on all widgets #3015

Open

proneon267 added 2 commits December 4, 2024 00:09

Remove complex background color assignment on button

a68c366

Set default background color on activityindicator

2d39724

proneon267 requested a review from freakboy3742 December 4, 2024 21:04

freakboy3742 requested changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default background color for widgets on iOS #3009

Set default background color for widgets on iOS #3009

proneon267 commented Dec 2, 2024 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Dec 3, 2024

proneon267 Dec 3, 2024

freakboy3742 Dec 3, 2024

proneon267 Dec 3, 2024

freakboy3742 Dec 3, 2024

proneon267 Dec 3, 2024

freakboy3742 Dec 3, 2024

proneon267 Dec 3, 2024 •

edited

Loading

freakboy3742 Dec 3, 2024

proneon267 commented Dec 3, 2024

proneon267 commented Dec 3, 2024

freakboy3742 left a comment

freakboy3742 Dec 3, 2024

proneon267 Dec 4, 2024 •

edited

Loading

freakboy3742 Dec 3, 2024

proneon267 Dec 4, 2024

freakboy3742 Dec 3, 2024

proneon267 commented Dec 4, 2024 •

edited

Loading

proneon267 commented Dec 4, 2024

freakboy3742 left a comment

freakboy3742 Dec 4, 2024

proneon267 commented Dec 5, 2024

freakboy3742 commented Dec 9, 2024


None	Default background color
TRANSPARENT	Actual transparency
Color	Actual color

Set default background color for widgets on iOS #3009

Are you sure you want to change the base?

Set default background color for widgets on iOS #3009

Conversation

proneon267 commented Dec 2, 2024 • edited Loading

Context for tests:

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Dec 3, 2024

proneon267 commented Dec 3, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Dec 4, 2024 • edited Loading

proneon267 commented Dec 4, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Dec 5, 2024

freakboy3742 commented Dec 9, 2024

proneon267 commented Dec 2, 2024 •

edited

Loading

proneon267 Dec 3, 2024 •

edited

Loading

proneon267 Dec 4, 2024 •

edited

Loading

proneon267 commented Dec 4, 2024 •

edited

Loading