-
-
Notifications
You must be signed in to change notification settings - Fork 671
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 1 commit
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
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,16 +1,64 @@ | ||
from System.Drawing import Color | ||
from travertino.colors import TRANSPARENT | ||
from travertino.colors import TRANSPARENT, rgb, rgba | ||
|
||
CACHE = {TRANSPARENT: Color.Transparent} | ||
|
||
|
||
def native_color(c): | ||
def native_color(toga_color): | ||
try: | ||
color = CACHE[c] | ||
color = CACHE[toga_color] | ||
except KeyError: | ||
color = Color.FromArgb( | ||
int(c.rgba.a * 255), int(c.rgba.r), int(c.rgba.g), int(c.rgba.b) | ||
int(toga_color.rgba.a * 255), | ||
int(toga_color.rgba.r), | ||
int(toga_color.rgba.g), | ||
int(toga_color.rgba.b), | ||
) | ||
CACHE[c] = color | ||
CACHE[toga_color] = color | ||
|
||
return color | ||
|
||
|
||
def toga_color(native_color): | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. The use of 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. |
||
# 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 commentThe 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. |
||
# replicates CSS's rgba mechanism. For the formula used here, see: | ||
# https://en.wikipedia.org/wiki/Alpha_compositing#Description | ||
|
||
blended_alpha = child_color.a + ((1 - child_color.a) * parent_color.a) | ||
|
||
# Check if the blended alpha is zero, indicating no blending | ||
if blended_alpha == 0: | ||
# If both child and parent alphas are 0, no blending occurs, so return child color. | ||
return child_color | ||
|
||
blended_color = rgb( | ||
# Red Component | ||
( | ||
( | ||
(child_color.r * child_color.a) | ||
+ (parent_color.r * parent_color.a * (1 - child_color.a)) | ||
) | ||
/ blended_alpha | ||
), | ||
# Green Component | ||
( | ||
( | ||
(child_color.g * child_color.a) | ||
+ (parent_color.g * parent_color.a * (1 - child_color.a)) | ||
) | ||
/ blended_alpha | ||
), | ||
# Blue Component | ||
( | ||
( | ||
(child_color.b * child_color.a) | ||
+ (parent_color.b * parent_color.a * (1 - child_color.a)) | ||
) | ||
/ blended_alpha | ||
), | ||
) | ||
return blended_color |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,15 @@ | |
from decimal import ROUND_HALF_EVEN, Decimal | ||
|
||
from System.Drawing import ( | ||
Color, | ||
Point, | ||
Size, | ||
SystemColors, | ||
) | ||
from travertino.size import at_least | ||
|
||
from toga.colors import TRANSPARENT | ||
from toga_winforms.colors import native_color | ||
import toga | ||
from toga.colors import TRANSPARENT, rgba | ||
from toga_winforms.colors import alpha_blending_over_operation, native_color, toga_color | ||
|
||
|
||
class Scalable: | ||
|
@@ -37,10 +37,6 @@ def scale_round(self, value, rounding): | |
|
||
|
||
class Widget(ABC, Scalable): | ||
# In some widgets, attempting to set a background color with any alpha value other | ||
# than 1 raises "System.ArgumentException: Control does not support transparent | ||
# background colors". Those widgets should set this attribute to False. | ||
_background_supports_alpha = True | ||
|
||
def __init__(self, interface): | ||
self.interface = interface | ||
|
@@ -119,17 +115,27 @@ def set_color(self, color): | |
self.native.ForeColor = native_color(color) | ||
|
||
def set_background_color(self, color): | ||
if not hasattr(self, "_default_background"): | ||
self._default_background = self.native.BackColor | ||
if color is None or ( | ||
color == TRANSPARENT and not self._background_supports_alpha | ||
): | ||
self.native.BackColor = self._default_background | ||
if color is None or (color == TRANSPARENT and (not self.interface.parent)): | ||
self.native.BackColor = SystemColors.Control | ||
else: | ||
win_color = native_color(color) | ||
if not self._background_supports_alpha: | ||
win_color = Color.FromArgb(255, win_color.R, win_color.G, win_color.B) | ||
self.native.BackColor = win_color | ||
if self.interface.parent: | ||
parent_color = toga_color( | ||
self.interface.parent._impl.native.BackColor | ||
).rgba | ||
if color is TRANSPARENT: | ||
requested_color = rgba(0, 0, 0, 0) | ||
else: | ||
requested_color = color.rgba | ||
blended_color = alpha_blending_over_operation( | ||
requested_color, parent_color | ||
) | ||
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 commentThe 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 |
||
|
||
if isinstance(self.interface, toga.Box): | ||
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 shouldn't be an explicit type check for Box. |
||
for child in self.interface.children: | ||
child._impl.set_background_color(child.style.background_color) | ||
|
||
# INTERFACE | ||
|
||
|
@@ -151,8 +157,7 @@ def refresh(self): | |
# doesn't actually support transparency. It just copies the parent's | ||
# BackColor to the widget. So, if a widget's parent changes then we need | ||
# to reapply background_color to copy the new parent's BackColor. | ||
if self.interface.style.background_color in {None, TRANSPARENT}: | ||
self.set_background_color(self.interface.style.background_color) | ||
self.set_background_color(self.interface.style.background_color) | ||
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 can see that re-application will be necessary, but it's not clear to me why this is happening in refresh. Refresh is a layout calculation mechanism; it shouldn't be interacting with color choices. 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'm not sure where should I move the re-application call to. I had thought refresh was to be used for reapplying changes related to both layout changes and style changes. 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. No - refresh is an internal trigger when something affecting widget layout has occurred - something that could change the size of the widget.
What is the sequence of events that requires a call in refresh? How do you manufacture a problem that is resolved by this line of code? 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. The background color needs to be reapplied when the widget's parent changes. Since refresh should be used for layout, I have moved the call to reapply background color, to the widget's |
||
|
||
def rehint(self): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,8 @@ | ||
import System.Windows.Forms as WinForms | ||
|
||
from toga.colors import TRANSPARENT | ||
|
||
from .base import Widget | ||
|
||
|
||
class Box(Widget): | ||
def create(self): | ||
self.native = WinForms.Panel() | ||
|
||
def set_background_color(self, color): | ||
if color in {None, TRANSPARENT}: | ||
if self.interface.parent: | ||
self.native.BackColor = self.interface.parent._impl.native.BackColor | ||
else: | ||
super().set_background_color(color) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ | |
|
||
|
||
class ProgressBar(Widget): | ||
_background_supports_alpha = False | ||
|
||
TOGA_SCALE = 1000 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,16 @@ | ||
import pytest | ||
from pytest import approx | ||
from System import EventArgs, Object | ||
from System.Drawing import Color, SystemColors | ||
from System.Drawing import SystemColors | ||
from System.Windows.Forms import MouseButtons, MouseEventArgs | ||
|
||
from toga.colors import TRANSPARENT | ||
from toga.style.pack import JUSTIFY, LEFT | ||
from toga_winforms.colors import alpha_blending_over_operation | ||
|
||
from ..fonts import FontMixin | ||
from ..probe import BaseProbe | ||
from .properties import toga_color | ||
from .properties import reverse_alpha_blending_over_operation, toga_color | ||
|
||
|
||
class SimpleProbe(BaseProbe, FontMixin): | ||
|
@@ -56,10 +57,30 @@ def color(self): | |
|
||
@property | ||
def background_color(self): | ||
if self.native.BackColor == Color.Transparent: | ||
if ( | ||
self.widget.style.background_color is None | ||
and self.native.BackColor.ToArgb() == SystemColors.Control.ToArgb() | ||
): | ||
return None | ||
|
||
if self.widget.style.background_color is TRANSPARENT and ( | ||
self.native.BackColor.ToArgb() | ||
== self.widget.parent._impl.native.BackColor.ToArgb() | ||
): | ||
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 seem quite right. The purpose of the probe is to return an "independent observer" value for things being tested - in this case, color. If we're testing how color and transparency works, we can't really include On top of that, the second if will fail if invoked on the root widget in a tree (because it won't have a parent). 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 am not sure if there is an easy way out of this. To the native winforms, the blended colors all have an alpha value of 1. This is important because including an alpha component in the blended color whose value is not 1, will make winforms perform additional calculation and produce wrong results, since we are handling the alpha blending ourselves. For example, if a child requests a background color of (0,0,255,0) and the parent has a background color of (255,0,0,1). Then the blended color would be (255,0,0,1). So, if we were to query to the native winforms about the color of the child, then it would report it as (255,0,0,1). So, now we would have no idea if the child background color was (0,0,255,0) or TRANSPARENT, because in both cases the blended color will be (255,0,0,1). Though it may seem that (0,0,255,0) and TRANSPARENT are same, but the as the value of alpha increases between 0 to 1, the results would be vastly different. And the original requested color is something, we need to preserve in order to enable the user to make minor changes(like increasing or decreasing the alpha value) to the background color later on. It is also evident from the reverse_alpha_blending_over_operation on the probe, that the original requested alpha value cannot be recovered from the blended color and the parent alone; We need the original requested color along with its alpha value. 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. The key idea that you need to keep in mind is what is the purpose of this test? We're not just trying to make the coverage check stop complaining - we're trying to validate correct behavior. In this case, that means getting the most independent possible validation that the color on a specific widget is correct. So how do we fix this? Well, the underlying problem is that on most platforms, we can compare the literal Conor; but on Winforms, we need to differentiate the transparent cases from the non-transparent cases. So... do that. Differentiate the test cases. Have different tests, with different test assertions for transparent colors. This might require modifying Alternatively - make It might even turn out that a combination of these approaches yields the best result - I haven't sat down and worked out all the details. What I'm pointing out is that when you're faced with a novel problem, sometimes you need to change the test, rather than trying to cram the novel problem into the existing 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 agree with you, modifications in either the tests or the probe or even both might be needed. I'll do more research and tests and report back to you. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Again - this requires that parent exists. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. As above - the value of |
||
if blended_color == parent_color: | ||
return self.widget.style.background_color | ||
else: | ||
return reverse_alpha_blending_over_operation( | ||
blended_color, | ||
parent_color, | ||
child_alpha=self.widget.style.background_color.rgba.a, | ||
) | ||
|
||
@property | ||
def font(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,7 @@ | ||
import System.Windows.Forms | ||
|
||
from toga.colors import TRANSPARENT | ||
|
||
from .base import SimpleProbe | ||
from .properties import toga_color | ||
|
||
|
||
class BoxProbe(SimpleProbe): | ||
native_class = System.Windows.Forms.Panel | ||
|
||
@property | ||
def background_color(self): | ||
if self.native.BackColor == self.widget.parent._impl.native.BackColor: | ||
return TRANSPARENT | ||
else: | ||
return toga_color(self.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.
toga_color
is also a method, so there's an inherent ambiguity here.