-
-
Notifications
You must be signed in to change notification settings - Fork 685
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
Set default background color for widgets on iOS #3009
base: main
Are you sure you want to change the base?
Changes from 4 commits
046d8de
481ea93
c7553ed
f18cbf1
5813bc0
a68c366
2d39724
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
On iOS, the default background color is now TRANSPARENT for Box, Canvas, ImageView, Label, ProgressBar, ScrollContainer and Slider widgets. |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -89,21 +89,17 @@ def set_color(self, color): | |||||||||
# By default, color can't be changed | ||||||||||
pass | ||||||||||
|
||||||||||
def set_background_color(self, color): | ||||||||||
# By default, background color can't be changed | ||||||||||
pass | ||||||||||
|
||||||||||
# TODO: check if it's safe to make this the default implementation. | ||||||||||
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): | ||||||||||
if is_native_color: | ||||||||||
self.native.backgroundColor = color | ||||||||||
else: | ||||||||||
try: | ||||||||||
# systemBackgroundColor() was introduced in iOS 13 | ||||||||||
# We don't test on iOS 12, so mark the other branch as nocover | ||||||||||
self.native.backgroundColor = UIColor.systemBackgroundColor() | ||||||||||
except AttributeError: # pragma: no cover | ||||||||||
self.native.backgroundColor = UIColor.whiteColor | ||||||||||
default_background_color = getattr(self, "_default_background_color", None) | ||||||||||
if default_background_color is None: | ||||||||||
default_background_color = UIColor.systemBackgroundColor() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use Also - it feels like there's some ambiguity here about what "None" means in the context of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have changed it to use On the backends, there are 3 different possible values for set_background_color():
But for Button, the historical behavior is "None = TRANSPARENT", which I have preserved in this PR. |
||||||||||
|
||||||||||
self.native.backgroundColor = ( | ||||||||||
default_background_color if color is None else native_color(color) | ||||||||||
) | ||||||||||
|
||||||||||
# INTERFACE | ||||||||||
def add_child(self, child): | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -38,6 +38,7 @@ def create(self): | |||||||
|
||||||||
self._icon = None | ||||||||
|
||||||||
self._default_background_color = None | ||||||||
# Add the layout constraints | ||||||||
self.add_constraints() | ||||||||
|
||||||||
|
@@ -68,10 +69,10 @@ def set_color(self, color): | |||||||
) | ||||||||
|
||||||||
def set_background_color(self, color): | ||||||||
if color == TRANSPARENT or color is None: | ||||||||
self.native.backgroundColor = None | ||||||||
else: | ||||||||
self.native.backgroundColor = native_color(color) | ||||||||
super().set_background_color( | ||||||||
(None if color in {None, TRANSPARENT} else native_color(color)), | ||||||||
is_native_color=True, | ||||||||
) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, this is a change in behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've restored it. But it's worth noting that
According to: https://developer.apple.com/documentation/uikit/uiview/1622591-backgroundcolor?language=objc:
But according to: https://developer.apple.com/documentation/uikit/uibackgroundconfiguration/3600317-backgroundcolor
So, I have set the background color of widgets which should have transparent background by default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
|
||||||||
def set_font(self, font): | ||||||||
self.native.titleLabel.font = font._impl.native | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
... usesnative_color()
on the invocation, it seems to be completely unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_background_color()
will receive a toga color from the core interface, but on the backend since we are working directly with the nativeUIColor
, so any invocation ofset_background_color()
from the backend requiresis_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 nativeUIColor
.But since now we are directly specfying the_default_background_color
in the form of nativeUIColor
, we need the additional indicatoris_native_color=True
to prevent any incorrect interpretation while assigning the background color.You were right.