-
-
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?
Changes from 7 commits
1e75114
1e075fa
ca50f58
6c1aeea
24abf08
a94f0de
c30aa56
6fe78dd
4a242dd
054e07b
6b1bf9e
4b5ed6f
77df2e9
6981a7a
b2ca091
1b3122c
c767a5b
9921d79
06d40e1
9254e18
e53879d
d43aba4
059578f
298d460
c6382c3
e5e3e07
ca4d5d9
8c62422
b0c8613
b6cf7d5
4b7991d
9e354f8
71f461b
b2f73a7
a0e141f
350b9d0
aad928c
9e11129
4aaedd4
a1cf579
2bb6a39
c9c714e
553ac6e
2f657db
f63859c
3e0f883
6f75e5f
a88cc13
221fd10
dbe7b42
3054df1
135741f
5991cd2
165d4b9
b2d0c47
a077b0e
d2bfbfb
544802c
db05029
f7687a8
4e4a82e
0ff7752
7d0515e
c0d8b5f
479449a
19c0d02
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 |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import itertools | ||
import warnings | ||
from math import degrees | ||
|
||
from android.graphics import ( | ||
|
@@ -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() | ||
|
||
def get_image_data(self): | ||
bitmap = Bitmap.createBitmap( | ||
self.native.getWidth(), self.native.getHeight(), Bitmap.Config.ARGB_8888 | ||
) | ||
canvas = A_Canvas(bitmap) | ||
background = self.native.getBackground() | ||
background = self._native_get_background() | ||
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 commentThe 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.draw(canvas) | ||
|
||
stream = ByteArrayOutputStream() | ||
|
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. This probably needs to be broken up into a couple of release notes - one for each underlying issue that we're resolving. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Bugs related to transparent background color are fixed. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -393,14 +393,12 @@ async def test_background_color_reset(widget, probe): | |
|
||
# Set the background color to something different | ||
widget.style.background_color = RED | ||
await probe.redraw("Widget background background color should be RED") | ||
await probe.redraw("Widget background color should be RED") | ||
assert_color(probe.background_color, named_color(RED)) | ||
|
||
# Reset the background color, and check that it has been restored to the original | ||
del widget.style.background_color | ||
await probe.redraw( | ||
message="Widget background background color should be restored to original" | ||
) | ||
await probe.redraw(message="Widget background color should be restored to original") | ||
assert_color(probe.background_color, original) | ||
|
||
|
||
|
@@ -409,10 +407,21 @@ async def test_background_color_transparent(widget, probe): | |
original = probe.background_color | ||
supports_alpha = getattr(probe, "background_supports_alpha", True) | ||
|
||
# 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 commentThe 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 commentThe 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. |
||
|
||
# Change the background color to transparent | ||
widget.style.background_color = TRANSPARENT | ||
await probe.redraw("Widget background background color should be TRANSPARENT") | ||
await probe.redraw("Widget background color should be TRANSPARENT") | ||
assert_color(probe.background_color, TRANSPARENT if supports_alpha else original) | ||
|
||
# Restore original background color | ||
del widget.style.background_color | ||
await probe.redraw("Widget background color should be restored to original") | ||
assert_color(probe.background_color, original) | ||
|
||
|
||
async def test_alignment(widget, probe, verify_vertical_alignment): | ||
"""Widget honors alignment settings.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ | |
|
||
|
||
class ProgressBar(Widget): | ||
_background_supports_alpha = False | ||
|
||
TOGA_SCALE = 1000 | ||
|
||
def create(self): | ||
|
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
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.