Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove erroneous note about _default_background_color override behavior on iOS #3125

Merged

Conversation

proneon267
Copy link
Contributor

@proneon267 proneon267 commented Jan 24, 2025

While working on #2484, I noticed that the setting of _default_background_color attribute on iOS is not correct:

def __init__(self, interface):
super().__init__()
self.interface = interface
self._container = None
self.constraints = None
self.native = None
self.create()
# Override this attribute to set a different default background
# color for a given widget.
self._default_background_color = self.native.backgroundColor

Although, currently no widget on iOS is setting a different _default_background_color, but if any widget did override it, then _default_background_color would be overridden during the create() call.

But we are unconditionally setting the _default_background_color on line 18, which would discard any custom _default_background_color that may be specified by a widget.

This PR was fixing this behavior by:

def __init__(self, interface):
super().__init__()
self.interface = interface
self._container = None
self.constraints = None
self.native = None
self.create()
# Override this attribute to set a different default background
# color for a given widget.
if getattr(self, "_default_background_color", None) is None:
self._default_background_color = self.native.backgroundColor

However, since no widget is specifying a custom default background color, so the erroneous note was removed instead.

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

@proneon267 proneon267 changed the title Correct _default_background_color override behavior on iOS Correct _default_background_color override behavior on iOS Jan 24, 2025
@proneon267 proneon267 force-pushed the iOS_fix_default_background_override branch from 1d7d2fa to f42302a Compare January 24, 2025 17:08
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So - if we have literally no widget that needs it... and the implementation has a bug anyway... why not remove the capability entirely? If we haven't found a need for it, I find it unlikely that anyone else will?

@proneon267
Copy link
Contributor Author

I agree with you, since no widget is defining a custom default background color, so this PR changes are not required.

But we still need to save the system assigned default widget background so that we can restore it when the background color is reset.

Therefore, the only change that this PR should be doing is to remove the note which is stating that widgets can override the attribute.

I'll make the changes and report back.

@proneon267 proneon267 changed the title Correct _default_background_color override behavior on iOS Remove erroneous note about _default_background_color override behavior on iOS Jan 25, 2025
@proneon267
Copy link
Contributor Author

The webview testbed failure on iOS is unrelated, as this PR doesn't make any code changes.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing my point. We're not using the feature. So why have the feature at all?

I'm guessing your argument is that it captures the initial background color - at which point, we're back to a comment that I made during my initial review - what happens when you switch from dark mode to light mode?

The only way I can see this approach working during a mode switch is:

  1. The background color is always transparent - in which case, we can use Transparent as the default value
  2. The background color is always None - in which case iOS is handling the "fall back to default" value... in which case, so can we.

In either case, capturing the initial default isn't actually helping - it's just adding a state variable that gives the illusion of state without actually provide any statefulness.

@proneon267
Copy link
Contributor Author

proneon267 commented Jan 26, 2025

As you had seen in #3104, the default background color of some widgets isn't actually UIColor.clearColor or None, it's something different like UIExtendedGrayColorSpace 0 0.
Although it may seem like the system assigned default background color is just the same asUIColor.clearColor or None, but using them as the default background color will result in bugs like in #3104.

Therefore, we definitely do need to preserve the system assigned default background color. Furthermore as you have noted previously, we shouldn't be changing the system assigned default as the system assigned default is the correct default.

As for correctly handling the change from light mode to dark mode, since we are using the system assigned default background, so the system handles it correctly for us. This is also what I had confirmed from manual testing.

Therefore, we should remove the erroneous note to prevent any incorrect presumptions.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - in which case, that is the detail we should be preserving in the comment.

iOS/src/toga_iOS/widgets/base.py Show resolved Hide resolved
Co-authored-by: Russell Keith-Magee <[email protected]>
@proneon267
Copy link
Contributor Author

Thank you for helping :)

@proneon267
Copy link
Contributor Author

Webview testbed failure on iOS is unrelated.

@freakboy3742 freakboy3742 merged commit 78d3e75 into beeware:main Jan 26, 2025
48 checks passed
@proneon267 proneon267 deleted the iOS_fix_default_background_override branch January 26, 2025 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants