-
-
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
background_color
Transparency Fixes
#2484
base: main
Are you sure you want to change the base?
Conversation
Regarding the new test, I have added the missing canvas image data test, but since the internal method to patch for testing are platform specific, hence the test is on the probe (Also, on android,
which you had previously told me to avoid. But, I think that using |
background_color
Transparency Fixes
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.
Your proposed interpretation of background color matches what I would expect.
Your proposal to change the default background color also largely makes sense; it's been proposed as a fix for at least 2 issues; see #767 and #2425. However, it will likely have a widespread impact, so we need to be very careful making this change.
As noted inline, the mock-based approach is a non starter; I suspect once that has been removed, the issue about the new test becomes a non-issue.
@@ -236,14 +237,20 @@ def _text_paint(self, font): | |||
paint.setTextSize(self.scale_out(font.size())) | |||
return paint | |||
|
|||
# This has been separated out, so that it can be mocked during testing. | |||
def _native_get_background(self): | |||
return self.native.getBackground() |
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 an acceptable approach. Mocking is a last resort for testing. It's used in a handful of places related to permissions and hardware APIs because using those APIs requires passing control to a separate process, at which point the test suite loses the ability to execute. Background color is an entirely internal mechanism, and isn't encumbered by this restriction.
It may be difficult to interrogate - but that doesn't mean we can mock it, because we're trying to verify the actual behavior.
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.
During the test, coverage reported missing coverage for:
toga/android/src/toga_android/widgets/canvas.py
Lines 244 to 247 in dad68bb
background = self.native.getBackground() | |
if background: | |
background.draw(canvas) | |
self.native.draw(canvas) |
I/python.stdout: Name Stmts Miss Branch BrPart Cover Missing
I/python.stdout: -------------------------------------------------------------------------------------------------------------------------------------------------------
I/python.stdout: data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/requirements/toga_android/widgets/canvas.py 153 0 28 1 99.4% 245->247
I/python.stdout: -------------------------------------------------------------------------------------------------------------------------------------------------------
I/python.stdout: TOTAL 2190 0 324 1 99.9%
So, 245->247 means it is reporting missing coverage for the missing else branch. The else branch will be triggered when self.native.getBackground()
returns None. Since, it is an internal platform method, I cannot make it return None without mocking it. The other way was to ignore the missing else branch with #pragma: no branch
, which you have told me to avoid. So, is there any other way to test the missing else branch or am I missing something?
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.
NVM, I just realized there was a much simpler way to test this. Thanks :) So, the question remains, should I move the test to testbed or keep it in the probe, since it is implementation specific?
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.
There might be a specific edge case that is needed to exercise that test, but if the other backends don't have an equivalent "if background" branch, then they will run the code without branching, giving double coverage of one specific code path. That's not a problem - it's OK to test something more than once on iOS (et al) if it means we get 100% coverage on Android as well; if nothing else, it's an indication that there are two different test configurations, and we need to test both, even if the "default" implementation works for both cases on iOS.
However, I'm confused because that line did have coverage prior to this change, and it has coverage on every other platform. This, to me, indicates that there's a bigger problem here. Either this branch of code was covering over an edge case that no longer exists, or there's a case that isn't being tested.
testbed/tests/widgets/properties.py
Outdated
# Set the background color to something different | ||
widget.style.background_color = RED | ||
await probe.redraw("Widget background color should be RED") | ||
assert_color(probe.background_color, named_color(RED)) |
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.
What is this test for? We already have a background color test - why do we need to test a normal background color in the transparent background color test?
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 think, this is a debugging artifact, that I had put on for visual inspection of widgets with transparent as default background color. I'll remove it. Thanks.
While implementing this PR, I discovered a bug in the WinForms Divider widget. I have corrected the WinForms Divider implementation to use a The previous implementation did not produce any visual results when background color was set. This is because it was producing the Divider widget by squeezing the borders of the The reason why it was passing tests was because it was setting the background color of the |
Also, discovered another bug in Label widget transparent background_color on WinForms.
But it only works if the label is inside/above a PictureBox and not for other cases. So, the best bet for transparency is to make label background color same as the container inside which the widget is present. The other way would be to mark Label as |
Unless I'm misunderstanding, this is what #2425 is describing.
If
Is it setting the background to white, or "default background"? We need to be very careful that we're differentiating between those two cases, especially given that background has been getting closer and closer to white over time. I think the solution may be actually halfway between the two answers you've suggested. My read of #767 and #2425 is that the current In the case of widgets like Button, we also need to be careful about the use of background - are we describing the button background color, or are we describing the color of the padding around the button. |
Apologies for the delayed response. So, I researched more and did more tests, and found that WinForms doesn't actually support true transparency. The important bit for setting up transparency is:
WinForms actually just copies the BackColor of the widget's parent to the widget. Which means, every time the widget's parent changes, we need to reapply background color to copy the new parent's BackColor to the widget. However, if we use the above code, then the behavior becomes highly inconsistent, and even glitches out when the parent is changed. Most of the time transparency works like a fluke. I also tried to use a I have found that manually setting a widget's BackColor same as that of the parent widget works most stably, and that is what I have opted to implement. With it, I have fixed #2425(the bug were non transparent Label and Box widgets). Another bug I have noticed is that when a button's background_color is manually set to another color, like in the above image, then a white border appears. Maybe it is not clearly visible in a white background, but if we change the background to a contrasting color, then it becomes apparent. The only way to remove it seems to be to change the button's FlatStyle to Flat:
But then, it doesn't look like a native button anymore. As for the new failing canvas tests:
They are somehow connected to the new |
What happens if I:
My read of this code is that the child will end up with a red background. Don't we also need to propagate any background color to all children? Also - what about alpha blending? The approach you've described will only work for a true TRANSPARENT color. If I set my background to "50% gray, 50% alpha", and my parent is red, then my background shouldn't be red - it should be a 50% blend of gray and red. This last case is definitely more complex to handle (although, interestingly, a blend with TRANSPARENT should be a redundant case of blending with any parent color...). Unless you can find an easy way to implement it, I'd be OK with noting this as an edge case in the docs.
A red background doesn't look very native either :-) There's an argument to be made that the border is desirable specifically because it ensures the button edge is visually distinct. If I have a red background and a red button, how do I know the extent of the button's pressable surface?
Well sure - you're messing around with transpareny, so the tests of transparency are breaking. My guess will be either the expected results are incorrect once you correctly handle transparency, or the canvas probe isn't correctly introspecting transparency. |
Apologies for the delayed response - I had gotten busy with my university assignments. I've addressed background color and transparency issues across all widgets, eliminating the need of Additionally, I've ensured proper handling of
The WebView HTML file can be found https://proneon267.github.io/proneon267-tests/transparency-test.html. Please try out the sample app and let me know if you encounter any inconsistencies. Based on this, it's evident that the expected reference image of the canvas is incorrect and requires updating. I haven't made |
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.
The core of the Winforms transparency changes make sense; I've flagged a couple of issues inline.
It doesn't surprise me that the reference image needs to be changed. When the test fails locally, it will save a copy of the "actual" image. The log of the test failure should provide the image that's been generated; the image generated in CI is also uploaded as an artefact attached to the CI run.
The biggest issue at this point is the new android "warning" branch, and the test that you've added to get coverage - the approach you've taken seems to be the "make the test be quiet" approach, rather than actually attempting to understand what is going on, and why that branch isn't being covered (and, more importantly - why it was covered before).
There's also an issue with the way you've implemented the background color probe; details inline.
else: | ||
self.native.BackColor = native_color(color) | ||
|
||
if isinstance(self.interface, toga.Box): |
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 shouldn't be an explicit type check for Box. self.interface.children is None
is a better check for widgets that cannot have children; if children
is not None, then there are children to work with.
winforms/src/toga_winforms/colors.py
Outdated
) | ||
CACHE[c] = color | ||
CACHE[toga_color] = color |
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.
toga_color
is also a method, so there's an inherent ambiguity here.
winforms/src/toga_winforms/colors.py
Outdated
return rgba(native_color.R, native_color.G, native_color.B, native_color.A / 255) | ||
|
||
|
||
def alpha_blending_over_operation(child_color, parent_color): |
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.
The use of child
and parent
nomenclature here presupposes the usage. Alpha blending is a generic task; back
and front
(or similar) would be better terminology.
I'd also be inclined to add this as a utility in core (or possibly even in Travertino). Although we're not actually using this anywhere other than Winforms, the generic ability to do alpha blending isn't a bad thing to have, and it would allow us to explicitly test the blending API with core tests, rather than implicitly testing it through usage in Winforms.
winforms/src/toga_winforms/colors.py
Outdated
|
||
|
||
def alpha_blending_over_operation(child_color, parent_color): | ||
# The blending operation I have implemented here is the "over" operation and |
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.
Code comments generally shouldn't refer to the author. We're describing the current state of the code, which isn't dependent on any one author.
) | ||
self.native.BackColor = native_color(blended_color) | ||
else: | ||
self.native.BackColor = native_color(color) |
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 branch is dealing with setting the color on the root element (i.e, parent is None); what happens if color has an alpha value (i.e., set the background color of the root element to 0.5 opacity red)? Do we need to do an alpha blend with SystemColors.Control
?
return TRANSPARENT | ||
else: | ||
return toga_color(self.native.BackColor) | ||
parent_color = toga_color(self.widget.parent._impl.native.BackColor) |
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.
Again - this requires that parent exists.
parent_color = toga_color(self.widget.parent._impl.native.BackColor) | ||
blended_color = alpha_blending_over_operation( | ||
self.widget.style.background_color, parent_color | ||
) |
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.
As above - the value of background_color
shouldn't matter; we're trying to establish ground truth.
@@ -244,6 +245,8 @@ def get_image_data(self): | |||
background = self.native.getBackground() | |||
if background: | |||
background.draw(canvas) | |||
else: | |||
warnings.warn("Failed to get canvas background") |
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 doesn't make any sense to me. Why couldn't the background be obtained?
self.native.setBackground(None) | ||
with pytest.warns(match="Failed to get canvas background"): | ||
self.impl.get_image_data() | ||
self.native.setBackground(original_background) |
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.
As with the comment on the implementation - why is this occurring? Why is the test suite passing with 100% branch coverage prior to this change?
changes/2484.bugfix.rst
Outdated
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 probably needs to be broken up into a couple of release notes - one for each underlying issue that we're resolving.
I've identified the cause of the missing coverage on the Android canvas: The missing coverage surfaced after I had made the following change: toga/android/src/toga_android/widgets/base.py Lines 138 to 142 in a9f9716
to: toga/android/src/toga_android/widgets/base.py Lines 138 to 144 in c767a5b
The missing coverage was reported for the lines 245->247: toga/android/src/toga_android/widgets/canvas.py Lines 244 to 247 in a9f9716
It seemed that a test was setting the widget's background to The test that is covering the case of direct jump to toga/testbed/tests/widgets/test_canvas.py Lines 283 to 303 in a9f9716
At the end of this test, we do: toga/testbed/tests/widgets/test_canvas.py Line 303 in a9f9716
And in assert_reference we do:toga/testbed/tests/widgets/test_canvas.py Lines 241 to 244 in a9f9716
So, for a canvas widget, it calls get_image() =>get_image_data() and in Canvas.get_image_date() :toga/android/src/toga_android/widgets/canvas.py Lines 244 to 247 in a9f9716
Here, background = self.native.getBackground() returned None and it directly jumped to line 247
I have also identified that, in toga/android/src/toga_android/widgets/base.py Line 139 in a9f9716
Here, self.native.getBackground() returned None since there was no background previously set on the widget.
So, to account for the missing coverage of the background color being set to |
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
…into transparency_fix
Quick question, if I move the fixes, I have made to the Android backend to a separate PR(like "Android background_color fixes") and change the description of this PR, then can we merge this PR? Also, a meta question, often times when working on a PR, I encounter several related bugs that I can fix. But by doing so, I increase the size of the PR. Moreover, if I decide to breakdown each of those bugfixes into new PRs, then I just start to pile up PRs. What should I do in such situations? Do I log an issue about each of those bugs & not fix the bugs, and leave it at that? |
I still need to do a thorough review; but reducing the scope of a PR will always make it more likely it will be merged. Without doing that full review, it's difficult to say whether "this PR minus Android" would be mergable.
The PRs only "pile up" when they're not presented in a mergable state. The smaller they are, the more likely they will be mergable - or, at least, the easier it will be to review them, or modify them so they are mergable. "One PR == one issue" is a good general rule to work by. If, in the process of fixing one bug, you find a second issue - submit a second PR. You don't necessarily need to open a second issue - you can if you want to, but we're happy to have a PR with no attached issue, as long as the PR contains as much detail as the issue would have had (i.e, how do I reproduce this problem). The only real reason that a single PR would close multiple issues is if fixing the second issue is almost accidental - that there's some fundamental conceptual fix, and one fix corrects 2 (seemly unrelated) things. However, if the fix for the second issue can be completely isolated from fix for the first issue - make it a second PR. The same is true of features - unless 2 features are absolutely co-mingled, present them as 2 PRs, not 1. The reason this PR is taking so long to land? It's not one issue. Based purely on the change notes, it's at least 7 issues. Every time it's come back for review, it's acquired a couple more issues, and more complexity. That's why I've previously advised - twice - that this PR needs to be broken up. This complexity also impacts on when I'm able to provide a review. If I were to provide a full review of this PR right now, I'd need to set aside half a day. My final review will involve require running an example of almost every widget on every platform, to verify that there hasn't been a change to background color rendering (or, if there is a change, that it's both intentional and desired). That's a big commitment - and I need to balance scheduling that review against my other priorities - which impacts on the feedback cycle that you get. And, to be completely frank, based on past PRs you've submitted, I'd honestly expect that I'd need to do another half day review in a day/week/month when you've been able to respond to the feedback that has been provided. Compare that to a small PR for a clearly described issue that touches a handful of files? I can slip a review of that in at the end of the day when I've got 20 minutes to spare. If I need to review that same PR twice or three times in a week, that's not especially onerous, as long as progress is being made. And, from your perspective, it means you can take lessons learned landing one PR into the next one, on a much faster feedback cycle. You could easily end up landing 7 PRs in 7 weeks, each of which received multiple passes of review - because you've made the reviewing process easier. To be clear - this isn't just a rule I'm making up solely for you, either. It's the sort of thing I have to do as well. Consider #2244 - that PR turned into a prototyping monster; I'm currently in the process of converting it into ~10 PRs, each of which gets one step closer to the final result. This has been done entirely because of the overhead of code review. |
I understand the importance of keeping my PRs small and focused, and I appreciate your patience and guidance. Regarding the current PR, I'll take your advice and break it down into smaller, more manageable and reviewable PRs. To avoid piling up unmerged PRs, I'll also focus on ensuring each submission is in a mergeable state before moving on to the next. I understand that large PRs demand substantial review time and can complicate the merging process. I'll start by addressing the current PR, breaking it down as discussed - starting with the smallest bugfix in this PR. Thank you again for your guidance and support. I really appreciate it. |
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. 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. |
Thanks for confirming. I'll investigate further and report back. |
Can you take a look at the iOS testbed setup failure: https://github.com/beeware/toga/actions/runs/12103501801/job/33745775230?pr=2484. It is failing to build Pillow for iOS. |
@proneon267 It shouldn't be trying to compile Pillow at all - it should be using a pre-compiled wheel. From the look of it, the issue was a timeout on the BeeWare wheel repo; I've restarted the build, and that seems to have corrected the problem. |
Thanks for restarting it. |
Fixes #767, Fixes #2425, Fixes #2430
With reference to the issues identified in #2478, I have implemented the related fixes separately in this PR. Reposting from #2478 for reference:
The reason for why only the divider widget triggered the transparency bug on Android and not the existing widgets is that the divider widget is explicitly setting a background color on initialization:
toga/android/src/toga_android/widgets/divider.py
Lines 13 to 14 in 1b96ca1
Hence, its default background is also not transparent whereas for other widgets, the default background was TRANSPARENT.
I have also fixed a bug on winforms progressbar, which was encountered when I enabled
test_background_color_transparent
intest_progressbar
.On the backends, there are 3 different possible values for
set_background_color()/set_background_color_simple()
- None, TRANSPARENT, color(Actual color). I want to propose the following interpretation for them(for the internal backend calls):Also, as per: https://developer.mozilla.org/en-US/docs/Web/CSS/background-color#formal_definition, the initial value for background_color is transparent, so I also propose to make TRANSPARENT the initial value for background_color.
Let me know what you think and whether I have left out any widgets where the transparency test should be enabled.