-
-
Notifications
You must be signed in to change notification settings - Fork 687
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
[widget audit] toga.Button #1761
Conversation
As of cddb9e4 platform coverage is at:
Most of the coverage omissions appear to be related to the button press. I suspect coverage on Windows is a misreport; it's not reporting coverage on any of the method bodies, even though the tests should be running. This might be a path issue in my virtualised Windows setup. |
The Windows issue was caused by the same thing as nedbat/coveragepy#686: the main loop runs on a thread that's created through the Windows API, not the Python Fixed by manually installing the trace hook at the start of the thread. |
(50, 128, 200, 0.0), | ||
(50, 128, 200, 0.5), | ||
(50, 128, 200, 0.9), | ||
(50, 128, 200, 1.0), |
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've reduced the multiplicity of colours here as it didn't seem to be adding anything to test rigor, but it was difficult to see the "meaningful" colors appear in the test series.
As of 065bca3, the GTK colour tests pass, but only if you have UI animations turned of. At the command line, run:
You can reset the setting with:
This is an OS-wide setting. |
227e187
to
4a1ce8a
Compare
On Debian Buster, after disabling animations, I was able to reproduce some similar failures to the CI, plus a new one involving fonts and UTF-8: Log
|
Most of those errors look like the Pygobject issue - The font bug, and the failures where there's a B and A channel value, but no R and G value (e.g., test_color). Are you sure you're running with the bugfix fork of pygobject? However, some of the other failures do give me an idea - I wonder if the issue is color resets, and the tests not correctly identifying the background color (which is essentially platform dependent, but will be reliably reproducible on any given machine, and appears to be |
Nope. It was Wayland. Of course it was. |
The good news is that the global |
OK, after pulling the current version of this PR, and running |
df8737e
to
f7a1006
Compare
test_implementation doesn't allow widgets to implement methods by inheriting them from a base class. This is often the most readable and maintainable way of doing it, but will inevitably be different from platform to platform. Also, it's fairly arbitrary which methods it's checking for: e.g. TextInput has a set_font method, but Label doesn't. Now that we have the testbed, I think test_implementation is more trouble than it's worth, and can just be removed. |
I've added a The exception is Windows, where the default font size is 8 points. So Toga widgets with fixed font sizes currently appear proportionately larger on Windows than on the other platforms. I don't think there's anything we can do about that now, but in the future we could deal with it by implementing the CSS font size keywords |
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've done a full review pass; this mostly looks good, but I've flagged a couple of minor issues. Most of them are likely fixed with a docstring explaining reasoning; the only "big" item is the dependency inversion on font family resolution in the test probe (and even that is that big).
# will give relative sizes that are consistent with desktop platforms. It also | ||
# means font sizes will all change proportionately if the user adjusts the | ||
# text size in the system settings. | ||
tv.setTextSize(TypedValue.COMPLEX_UNIT_SP, self.interface.size) |
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 is definitely a good approximation; I only wonder if it might be worth introducing a fudge factor so that "12pt == 14sp" (i.e., "size in toga points" * 1.1666 == size in dp).
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.
(for the record - this is on the "strong opinion, weakly held" list; I won't give much pushback on a different fudge factor, or no fudge factor at all).
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.
Windows defaults to 8 points (see above), and according to the fonts.py files in the respective backends, Cocoa defaults to 12 points and iOS 17. So I don't see any strong reason to fix 12 as the default. Android is already within the range of the other platforms, and making 1 Toga pt = 1sp brings it closer to iOS, which is probably the one it's most useful to be consistent with.
In the long run I think the best solution to this issue is the CSS font size keywords, as mentioned 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.
Sure - that makes sense.
|
||
def get_typeface(self): | ||
cache_key = (self.interface, default_typeface) |
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.
Why is the default typeface part of the cache key?
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.
Because it may affect the resulting font when font_family is SYSTEM
. The default font on most widgets is sans-serif
, but on buttons it's sans-serif-medium
, which is intermediate between normal and bold.
) | ||
|
||
if font is None: |
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 you certain this error handling is no longer required? Core and system fonts will have all the weights/variants; but if a user-supplied font doesn't have a particular variant/weight, will this still succeed?
(And if it will - a comment explaining how/why would be worthwhile)
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.
It's no longer possible for font
to be None here, because:
- When a custom font can't be found, I've added a line above to fall back on the system font. Previously in this case, the size, style and weight settings would all be ignored.
- According to the
convertFont
documentation if it fails to find a font with the given traits, it returns the original font unchanged. I'll add a comment explaining this.
Also, note I've changed the indentation here so that the SYSTEM font uses the same bold/italic logic as everything else. Previously the italic setting would be ignored with a SYSTEM font.
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.
if a user-supplied font doesn't have a particular variant/weight
It would be good to have a test for this, but that would require actually supplying such a font, and it looks like loading fonts from a file is currently broken on Cocoa. But this won't be a very common thing for an app to do, so I'm happy to defer it until we come to test the Font
API directly.
testbed/tests/assertions.py
Outdated
".AppleSystemUIFont", | ||
"sans-serif-medium", # Android buttons | ||
], | ||
} |
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's the reasoning behind having this table of "generic" fonts in the testbed assertions, rather than being something that is normalised out by the test backend? Previously, toga_font
on each backend would normalise ".AppleSystemUIFont" into SYSTEM, but do so in the Cocoa backend.
Having this table here seems like an odd inversion - we've got platform-specific details in the generic test definitions.
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 problem I was trying to solve was the many-to-one mapping between Toga font names and actual fonts. For example, SANS_SERIF and SYSTEM will often give you the same visible result, as will explicitly specifying the font name. But whether these 3 options can be distinguished by the probe varies from platform to platform.
But you're right, it would be better for platform-specific differences to be handled by the test backend, so I'll do that.
".AppleSystemUIFont": SYSTEM, | ||
"Papyrus": FANTASY, | ||
}.get(str(font.familyName), str(font.familyName)), | ||
family=str(font.familyName), |
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.
See Q below about this inversion, removing the ".AppleSystemUIFont" normalisation from the platform backend.
if font is None: | ||
print(f"Unable to load font: {self.interface.size}pt {full_name}") | ||
else: | ||
_FONT_CACHE[self.interface] = font |
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 Cocoa; we either need to report the missing font issue, or docstring the reason why it isn't an issue.
await probe.redraw() | ||
assert_font_family(probe.font.family, family) | ||
assert probe.font.weight == weight | ||
assert probe.font.style == style |
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 makes me wonder whether what is needed here is a set of comprehensive "font management" tests operating on a single widget (say, Label), and a basic "can a font be set" test on each widget. If basic font setting works on a widget, we don't have any particular reason to believe that weight/variants will be problematic.
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.
That's true if we assume that the widgets are all using a common font implementation, and using it correctly, but that's something that could easily be broken by accident in the future. So I think it's worth being comprehensive as long as it only adds a few seconds to the tests.
assert 10 <= probe.width <= 150, f"Width ({probe.width}) not in range (10, 150)" | ||
assert 10 <= probe.height <= 50, f"Height ({probe.height}) not in range (10, 50)" |
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 discussed, these messages should be generated automatically by pytest's "assertion rewriting", but that isn't currently working on Android. But since this PR is blocking all the other widget testing, let's fix that separately.
…play a single space to prevent the widget from collapsing
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.
From the user's point of view, banning empty text on buttons seems like an arbitrary restriction. I see the problem was that it caused the button height to collapse on Winforms, so I've worked around that by making it display a single space instead, which is visually identical, and only requires a small special case in the tests.
I saw a similar issue with empty labels on iOS last week, so I'll probably end up doing something similar when I review the Label PR tomorrow. However, in the case of a label with a background color, the difference between a space and an empty string may be visually significant, so maybe there's a Unicode zero-width character that would be better.
Notes | ||
----- | ||
* Any "truthy" object (i.e, object that evaluates as ``True``) can be provided | ||
as the text for the button; if the object is not a string, it will be | ||
automatically converted to a string using ``str()``. | ||
|
||
* The button text cannot contain newlines. Any content after the first newline | ||
will be ignored. |
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've moved the property documentation into the docstring so it'll appear next to the property itself, rather than in a separate section.
testbed/tests/widgets/properties.py
Outdated
async def test_text_empty(widget, probe): | ||
"The text displayed on a widget can be empty" | ||
# Set the text to the empty string | ||
widget.text = "" |
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.
Empty strings are now covered by test_text
above, and conversion of non-string objects is covered by test_button.py in the core.
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.
A couple more notes and tweaks, hopefully not too controversial.
if expected == SYSTEM: | ||
assert actual == ( | ||
"sans-serif-medium" | ||
if isinstance(self.widget, toga.Button) |
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.
We can use subclassing here, rather than an isinstance check.
core/src/toga/widgets/button.py
Outdated
|
||
self._impl.set_text(value) | ||
self._impl.set_text(self._text) |
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'm generally against the introduction of interface state/shadow variables; in this case, the shadow variable is only required if we have problems round-tripping the original value. In this implementation we're not round tripping anyway (because we're losing None
and multiline input); to support the winforms edge case, I'm entirely comfortable with a normalizing a user-supplied "\u0200"
(zero width space) to ""
). It's highly unlikely to ever be user supplied in that form, and it's functionally indistinguishable from "".
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'm entirely comfortable with a normalizing a user-supplied
"\u0200"
(zero width space) to""
).
Makes sense, but \u0200 is LATIN CAPITAL LETTER A WITH DOUBLE GRAVE. I've replaced that with \u200B.
@@ -29,14 +29,8 @@ A button has a text label. A handler can be associated with button press events. | |||
|
|||
button = toga.Button('Click me', on_press=my_callback) | |||
|
|||
Notes |
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 we should keep this as a "Notes" section. While the notes at present are all style related, we're very likely going to come across platform specific eccentricities that don't fit the description being "style", and don't map to a specific attribute like the text handling of None
et al.
A testing and documentation audit of the
toga.Button
widget.Includes the content from #1794.
Audit checklist