-
-
Notifications
You must be signed in to change notification settings - Fork 691
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
Android background_color
Transparency Fixes
#3118
base: main
Are you sure you want to change the base?
Conversation
Originally posted by @mhsmith in #2484 (comment)
I can confirm that this also happens Toga 0.4.5, so it wasn't introduced by any of the other changes in this PR [#2484]. if we can understand exactly why the problem is happening, then we'll have a better idea of whether this is the simplest solution. Widgets shouldn't even be aware of their parent's background. But in this case, the Box isn't actually the Selection's parent at the native level: both of them are direct children of the RelativeLayout, and the Box appears behind the Selection because of their order of creation. So maybe certain background features are being drawn directly on the native parent (the RelativeLayout), and the intervening Box is covering it up? Experimenting with semi-transparent backgrounds may answer this. It may also have something to do with elevation. |
Readthedocs test failure is unrelated, and is caused due to https://osmfoundation.org/ being down. |
I experimented with semi-transparent backgrounds, to confirm if the arrow and other effects were being drawn on the native parent instead, and @mhsmith was correct:
The background features are being drawn directly on the native parent(RelativeLayout), as the Box isn't actually the Selection's parent at the native level: both of them are direct children of the RelativeLayout. To fix this, I have tried the following, but none of them worked: Using
|
# Most of the Android Widget have different effects applied them which provide | |
# the native look and feel of Android. These widgets' background consists of | |
# Drawables like ColorDrawable, InsetDrawable and other animation Effect Drawables | |
# like RippleDrawable. Often when such Effect Drawables are used, they are stacked | |
# along with other Drawables in a LayerDrawable. | |
# | |
# LayerDrawable once created cannot be modified and attempting to modify it or | |
# creating a new LayerDrawable using the elements of the original LayerDrawable | |
# stack, will destroy the native look and feel of the widgets. The original | |
# LayerDrawable cannot also be properly cloned. Using `getConstantState()` on the | |
# Drawable will produce an erroneous version of the original Drawable. | |
# | |
# These widgets are also affected by the background color of the parent inside which | |
# they are present. Directly, setting background color also destroys the native look | |
# and feel of these widgets. Moreover, these effects also serve the purpose of | |
# providing visual aid for the action of these widgets. |
So, it seems like wrapping the native widget inside another view allows us to preserve the native look and feel, while setting the background color of the widgets.
I'll need to defer to @mhsmith for suggestions here - my understanding of the inner workings of Android rendering are hazy at best. |
Thanks, I'll look at this today or tomorrow. |
Thank you :) |
The ripple effects are supposed to be circular, but they're being clipped by their native parent: It may be possible to fix this with the "clip" properties (https://stackoverflow.com/questions/30626019). |
if isinstance(widget, ContainedWidget): | ||
self.native_content.addView(widget.native_widget_container) | ||
else: | ||
self.native_content.addView(widget.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.
Most of the places you're calling isinstance(widget, ContainedWidget)
, it's only to switch between widget.native
and widget.native_widget_container
. It would be cleaner to factor this out:
- Rename
native_widget_container
tonative_toplevel
- Make Widget implement
native_toplevel
by returningnative
. - Change all of these locations to use
native_toplevel
unconditionally.
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.
Thanks, this simplified the codebase so much!
class ContainedWidget(Widget): | ||
def __init__(self, interface): | ||
|
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.
Please refactor to avoid duplicating the whole of Widget .__init__
.
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.
I have refactored it out and have removed all of the duplicated operations.
def get_enabled(self): | ||
return self.native.isEnabled() and self.native_widget_container.isEnabled() | ||
|
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.
Are the enabled
and focus
overrides really necessary?
I agree we need something for hidden
, but could this be done by changing the Widget base class implementation to use native_toplevel
as discussed above?
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.
Are the enabled and focus overrides really necessary?
I agree we need something for hidden, but could this be done by changing the Widget base class implementation to use native_toplevel as discussed above?
They weren't necessary, so I have removed these duplicated methods.
@@ -96,7 +95,7 @@ def onRefresh(self): | |||
self._interface.on_refresh() | |||
|
|||
|
|||
class DetailedList(Widget): | |||
class DetailedList(ContainedWidget): |
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.
Do DetailedList and ProgressBar need to be ContainedWidget subclasses? I don't think either of them has any special background effects.
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.
I agree, they don't need to be ContainedWidget
, so I have removed these changes.
@property | ||
def background_color(self): | ||
xfail("Can't change the background color of Selection on this backend") |
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 PR enables background colors for quite a few other widgets which didn't support them before. Do their tests also need to be updated?
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.
Yes, the tests should be enabled. However, since background_color
is not currently correct on all platforms, so we cannot enable the background color tests on this PR, as the tests would fail on the other platforms(as the fix for those platforms is on separate PRs).
Since, this would lead to a deadlock situation. Hence, I had created #3015 to keep track of the widgets on which all background color tests are not enabled. Once, these background_color
fixing PRs are completed, I will enable all background color tests on all platforms.
…popup background color
I have moved it to #3156.
I have removed these changes.
I totally agree with you, the default background color should be transparent, as is the case on most platforms. Hence, I have made transparent the default background color. |
The only remaining currently is the clipping of the ripple effect:
I tried to use the following(on the self.native_toplevel.setClipChildren(False)
self.native_toplevel.setClipToPadding(False)
self.native_toplevel.setClipToOutline(False) However, on further testing, it turns out the problem is due to the incorrect Currently, we have: toga/android/src/toga_android/container.py Lines 59 to 63 in 8959ff1
toga/android/src/toga_android/widgets/base.py Lines 218 to 230 in 8959ff1
Which produces the clipped ripple effect: Example source:(The `Selection` widget's background is intentionally kept white)"""
My first application
"""
import toga
from toga.style import Pack
from toga.style.pack import COLUMN
class HelloWorld(toga.App):
def reset_to_native_default_background_color(self, widget, **kwargs):
for widget in self.widgets:
del widget.style.background_color
self.content.refresh()
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(
background_color="#87CEFA",
direction=COLUMN,
flex=1,
),
children=[
toga.Box(style=Pack(flex=1.5)),
toga.Button(
text="Reset to native default background color",
on_press=self.reset_to_native_default_background_color,
),
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"],
style=Pack(background_color="#ffffff"),
),
toga.Box(style=Pack(flex=1.5)),
toga.Slider(min=-5, max=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.PasswordInput(value="Jane"),
toga.Box(style=Pack(flex=1.5)),
toga.Table(
headings=["Name", "Age"],
data=[
("Arthur Dent", 42),
("Ford Prefect", 37),
("Tricia McMillan", 38),
],
),
toga.Box(style=Pack(flex=1.5)),
toga.Switch(text="Switch"),
toga.Box(style=Pack(flex=1.5)),
toga.DetailedList(
data=[
{
"icon": toga.Icon("icons/arthur"),
"title": "Arthur Dent",
"subtitle": "Where's the tea?",
},
{
"icon": toga.Icon("icons/ford"),
"title": "Ford Prefect",
"subtitle": "Do you know where my towel is?",
},
{
"icon": toga.Icon("icons/tricia"),
"title": "Tricia McMillan",
"subtitle": "What planet are you from?",
},
],
),
toga.Box(style=Pack(flex=1.5)),
toga.DateInput(),
toga.Box(style=Pack(flex=1.5)),
toga.TimeInput(),
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 HelloWorld() However, logically the correct code should be: diff --git a/android/src/toga_android/container.py b/android/src/toga_android/container.py
index 714e495ac..d46cabfe3 100644
--- a/android/src/toga_android/container.py
+++ b/android/src/toga_android/container.py
@@ -60,4 +60,4 @@ class Container(Scalable):
lp = RelativeLayout.LayoutParams(width, height)
lp.topMargin = y
lp.leftMargin = x
- widget.native_toplevel.setLayoutParams(lp)
+ widget.native.setLayoutParams(lp)
diff --git a/android/src/toga_android/widgets/base.py b/android/src/toga_android/widgets/base.py
index 2f4774fca..7cd2eb40c 100644
--- a/android/src/toga_android/widgets/base.py
+++ b/android/src/toga_android/widgets/base.py
@@ -222,10 +222,10 @@ class ContainedWidget(Widget):
self.native_toplevel = RelativeLayout(self._native_activity)
self.native_toplevel.addView(self.native)
- self.native.setLayoutParams(
+ self.native_toplevel.setLayoutParams(
RelativeLayout.LayoutParams(
- RelativeLayout.LayoutParams.MATCH_PARENT,
- RelativeLayout.LayoutParams.MATCH_PARENT,
+ RelativeLayout.LayoutParams.WRAP_CONTENT,
+ RelativeLayout.LayoutParams.WRAP_CONTENT,
)
)
# Immediately re-apply styles. Some widgets may defer style application until But this produces the wrong result: The |
Originally identified in #2484, this PR separates the changes required to fix the
background_color
fixes on Android.Originally posted in: #2484 (comment) :
As I had noted here:
toga/android/src/toga_android/widgets/base.py
Lines 197 to 212 in c9c714e
What I meant by that is that in some widgets, for example:
toga.Selection
, when their background_color is set viaset_background_simple
then the native effects are lost.So, if I have:
Then the animation effect on interaction would look like:
But now, if I set a background color on the parent like:
Then the animation effect on interaction would look like:
As we can see, the dropdown arrow is not visible, and the ripple effect is also not visible. Further, the widget doesn't set any background color and hence even setting the parent's background color also destroys the native effects. The same problem exists in other widgets also(like
toga.Switch
, etc.), where the ripple effect will not be visible:As I have previously noted in the comment, tampering with the background(
LayerDrawable
) destroys it and with it the animation effects are also destroyed. Hence, I tried to solve this withContainedWidget
class which doesn't require the existing widgets to be changed.There is also,
toga.DetailedList
:But now, if I set a background color on the parent like:
Similar, problems also exist on other widgets also(like
toga.Canvas
). Hence, for these widgets also, I have usedContainedWidget
class to solve these issues.PR Checklist: