Skip to content
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

Implemented Selection.set_font() for Android #2130

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

t-arn
Copy link
Contributor

@t-arn t-arn commented Sep 24, 2023

This PR implements Selection.set_font() for Android.
It replaces the PR #1758

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@t-arn
Copy link
Contributor Author

t-arn commented Sep 24, 2023

@mhsmith Please review this PR

@freakboy3742
Copy link
Member

@mhsmith Please review this PR

A review would be premature, given that there's a merge conflict with main (and thus CI doesn't run).

However - the first review comment will be that this doesn't include any tests. With the widget audit nearing completion, we're in a position to enforce test coverage as a PR requirement. In this case, the Android testbed will need to be modified to include a probe for font properties on the Selection widget.

@t-arn
Copy link
Contributor Author

t-arn commented Sep 26, 2023

@freakboy3742
I now merged the main branch and the tests were all successful

 include a probe for font properties on the Selection widget

OK, will add that (by the way: this is missing for Winforms even though this platform already supports setting the font)

And where do I add the testbed code that sets the font and tests the result?

@freakboy3742
Copy link
Member

include a probe for font properties on the Selection widget

OK, will add that (by the way: this is missing for Winforms even though this platform already supports setting the font)

And where do I add the testbed code that sets the font and tests the result?

The test already exists. What you need to implement is the probe - the mechanism that tells the test "this is how you can find out the font of the widget on Android.

The idea behind the testbed is that we write a single set of tests that exercise everything that the backend should be able to do, using an abstract interface that can ask implementation specific questions.

So - we write a single test that tries to set the background color on a button. The test asks the backend probe "what is the current background color of the button", and gets a color, which the test can then assert. Rinse and repeat for every interesting property of every widget.

In your case, we have a test of Selection that checks the font (it's actually a generic test, because "change the font on a widget" is a capability common to most widgets; it's brought into selection specifically here).

This test asks the probe to assert the current font family, size, etc. The Android backend needs to break that check down into parts, so the assertion then defers to widget-specific properties. In Selection, these properties raise an xfail, which is the testbed's way of saying "This isn't implemented yet". You need to provide that implementation.

Compare and contrast with Winforms - the selection Widget doesn't define any font methods... but it inherits from SimpleProbe, which inherits from FontMixin, which defines the assertions. So - Winforms both implements and tests this feature.

@mhsmith
Copy link
Member

mhsmith commented Sep 27, 2023

# Conflicts:
#	android/src/toga_android/libs/android/widget.py
#	android/src/toga_android/widgets/selection.py
…rect values on font change after widget creation;

Removed implementation specific stuff from example app
@t-arn
Copy link
Contributor Author

t-arn commented Oct 3, 2023

@freakboy3742 Please review this PR

I found out why the Android testbed fails: The testbed assumes (hard-coded?) that the default font size on Android is always 14SP, but the default font size of the Selection view is 16SP as you can see in following screenshot:
image

I/python.stdout: =================================== FAILURES ===================================
I/python.stdout: __________________________________ test_font ___________________________________
I/python.stdout: Traceback (most recent call last):
I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/app/tests/widgets/properties.py", line 293, in test_font
I/python.stdout:     probe.assert_font_size(SYSTEM_DEFAULT_FONT_SIZE)
I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/app/tests_backend/fonts.py", line 80, in assert_font_size
I/python.stdout:     assert round(self.text_size) == round(
I/python.stdout: AssertionError: assert 42 == 37
I/python.stdout:  +  where 42 = round(42.0)
I/python.stdout:  +    where 42.0 = <tests_backend.widgets.selection.SelectionProbe object at 0x7f1a63a12bc0>.text_size
I/python.stdout:  +  and   37 = round(36.75)
I/python.stdout:  +    where 36.75 = <JavaMethod static float android.util.TypedValue.applyDimension(int, float, android.util.DisplayMetrics)>(2, 14, <android.util.DisplayMetrics 'DisplayMetrics{density=2.625, width=1080, height=2094, scaledDensity=2.625, xdpi=420.0, ydpi=420.0}'>)
I/python.stdout:  +      where <JavaMethod static float android.util.TypedValue.applyDimension(int, float, android.util.DisplayMetrics)> = TypedValue.applyDimension
I/python.stdout:  +      and   2 = TypedValue.COMPLEX_UNIT_SP
I/python.stdout:  +      and   <android.util.DisplayMetrics 'DisplayMetrics{density=2.625, width=1080, height=2094, scaledDensity=2.625, xdpi=420.0, ydpi=420.0}'> = <cyfunction __get__.<locals>.<lambda> at 0x7f1a69d25be0>()
I/python.stdout:  +        where <cyfunction __get__.<locals>.<lambda> at 0x7f1a69d25be0> = <android.content.res.Resources@7e6782>.getDisplayMetrics
I/python.stdout:  +          where <android.content.res.Resources@7e6782> = <cyfunction __get__.<locals>.<lambda> at 0x7f1a69d25630>()
I/python.stdout:  +            where <cyfunction __get__.<locals>.<lambda> at 0x7f1a69d25630> = <android.widget.Spinner{3f4628e VFED..CL. ........ 0,0-242,63}>.getResources
I/python.stdout:  +              where <android.widget.Spinner{3f4628e VFED..CL. ........ 0,0-242,63}> = <tests_backend.widgets.selection.SelectionProbe object at 0x7f1a63a12bc0>.native

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes inline. I've also fixed the issue with iOS on CI.

As for the remaining CI failure - the probe has a mechanism to define the default font size: see how the font size assertion is implemented: https://github.com/beeware/toga/blob/main/android/tests_backend/fonts.py#L79

If Selection has a different default font size, you can add a default_font_size declaration - see the probe for TextInput on Android as an example.

self.adapter.apply_font(tv)
self.interface.refresh()

def get_textsize(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this method only exists to support the probe, the detail should be captured there. It's OK for the probe to access internal implementation details of the widget - the probe is, by definition, probing internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, removed get_textsize() and get_typeface() from impl

@@ -0,0 +1 @@
The set_font() method has been implemented for Android
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's specific to selection, not "whole of android".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed the text accordingly

self.styled_selection.style.font_size = SYSTEM_DEFAULT_FONT_SIZE
self.styled_selection.style.font_style = NORMAL

def print_font_attributes(self, widget):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this "print" option is adding much - it's not really helpful as a diagnostic, as it's not doing anything but reflecting the values that were set in change_font from the same data source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, removed the button and code behind from the Selection example again

Fixed the probe, so it uses default_font_size=16;
Corrected the news fragment;
Removed the "Print font attrs" button from the example
@t-arn
Copy link
Contributor Author

t-arn commented Oct 5, 2023

@freakboy3742 I made all the requested changes and the font size check now seems to work. But I get a new fail when checking the orig_height (and then probably also orig_width):

2023-10-05T06:57:07.6636200Z I/python.stdout: __________________________________ test_font ___________________________________
2023-10-05T06:57:07.6650430Z I/python.stdout: Traceback (most recent call last):
2023-10-05T06:57:07.6665440Z I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/app/tests/widgets/properties.py", line 335, in test_font
2023-10-05T06:57:07.6678140Z I/python.stdout:     assert probe.height == orig_height
2023-10-05T06:57:07.6691350Z I/python.stdout: AssertionError: assert 41 == 24

How can I fix this?

@freakboy3742
Copy link
Member

How can I fix this?

I'm not sure what to say here. You fix it by finding the cause of the problem, and then adjusting the code so that the test passes.

One of 2 things must be true:

  1. The test has identified a problem with your code, in which case you need to fix your implementation; or
  2. You've revealed that the current test isn't rich enough to encompass the result that the Android backend is now generating, in which case you need to refactor the test so that we can verify the same fundamental property of the widget across all platforms, but allowing for the discrepancy that Android is exhibiting.

I haven't looked into the problem to know which of these two is happening here - but that's the task of developing a test for a new feature PR.

If you're stuck working out what is going on, or how to adjust the test, then we're happy to elaborate on details - but we need to know what you've investigated and what your analysis has revealed.

@t-arn
Copy link
Contributor Author

t-arn commented Oct 6, 2023

I don't know what is going wrong here.

The testbed gets the height with probe.height which is Selection._impl.native.getHeight()

I added a debug button to the Selection example app which prints out self.styled_selection._impl.native.getHeight()

I then printed the original height, changed the font, printed the new height, changed the font back and printed the height again.
Result: the height in the end matched the original height.

So, I don't know why the testbed check fails

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you'll have to look further into how and when the height is changing. I tried adding some print statements around the places where apply_font is called, and I got this:

I/python.stdout: ----------------------------- Captured stdout call -----------------------------
I/python.stdout: FIXME set_font system 30pt
I/python.stdout: FIXME apply tv=470679232816, self.interface=<Font: 30pt system>
I/python.stdout: FIXME set_font finished
I/python.stdout: FIXME getView
I/python.stdout: FIXME apply tv=470679344896, self.interface=<Font: 30pt system>
I/python.stdout: FIXME set_font fantasy 30pt
I/python.stdout: FIXME apply tv=470679232816, self.interface=<Font: 30pt fantasy>
I/python.stdout: FIXME set_font finished
I/python.stdout: FIXME getView
I/python.stdout: FIXME apply tv=470679345472, self.interface=<Font: 30pt fantasy>
I/python.stdout: FIXME set_font system 30pt
I/python.stdout: FIXME apply tv=470679232816, self.interface=<Font: 30pt system>
I/python.stdout: FIXME set_font finished
I/python.stdout: FIXME getView
I/python.stdout: FIXME apply tv=470679346672, self.interface=<Font: 30pt system>
I/python.stdout: FIXME set_font system default size
I/python.stdout: FIXME apply tv=470679232816, self.interface=<Font: system default size system>
I/python.stdout: FIXME set_font finished
I/python.stdout: --------------------------- Captured stdout teardown ---------------------------
I/python.stdout: FIXME getView
I/python.stdout: FIXME apply tv=470679357760, self.interface=<Font: system default size system>

This shows that every time the font changes, it's actually calling apply_font twice: once from set_font and once from getView. But on the final change, the call to getView doesn't happen until after the test is over.

I'm not sure why getView is being called at all here, but it might be reacting to the Selection's size changing as a result of Toga's own layout algorithm. So the obvious way to proceed would be to add more print statements around that area, such as in set_bounds and rehint.

A few other comments:

Comment on lines 25 to 30
def typeface(self):
xfail("Can't change the font of Selection on this backend")
return self.impl.adapter._typeface

@property
def text_size(self):
xfail("Can't change the font of Selection on this backend")
return self.impl.adapter._textsize
Copy link
Member

@mhsmith mhsmith Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever possible, the probe should be based only on the native widget, otherwise we're not verifying what the user can actually see. Then the _typeface and _textsize variables wouldn't even be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I eliminated the attributes _typeface and _textsize and query the native control directly

Comment on lines 37 to 38
if self.impl._font_impl and tv:
self.impl._font_impl.apply(
Copy link
Member

@mhsmith mhsmith Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although GitHub isn't showing a merge conflict, there actually is one, because the apply method doesn't exist anymore on the main branch. It's been replaced by set_textview_font in label.py. To make sure we don't merge the PR before dealing with this, I'll set it to draft status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I now use label.set_textview_font

tv = self.native.getSelectedView()
if tv:
self.adapter.apply_font(tv)
self.interface.refresh()
Copy link
Member

@mhsmith mhsmith Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refresh should be unnecessary, as the style system should automatically do a refresh whenever any style changes.

Copy link
Contributor Author

@t-arn t-arn Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I removed the refresh

@mhsmith mhsmith marked this pull request as draft October 7, 2023 09:21
@t-arn
Copy link
Contributor Author

t-arn commented Oct 11, 2023

I found out what caused the testbed problems on Android:
del widget.style.font_size and del widget.style.font_family both trigger an immediate widget change on Android, and await probe.redraw probably didn't wait to catch both updates.

I now reset the attributes one by one and now the testbed seems to work :-)

@t-arn
Copy link
Contributor Author

t-arn commented Oct 11, 2023

@freakboy3742 All checks now pass :-)
Please re-review this PR

@t-arn t-arn marked this pull request as ready for review October 12, 2023 10:09
@t-arn
Copy link
Contributor Author

t-arn commented Oct 12, 2023

@mhsmith I removed the draft status from this PR, because I merged main into it and now use Label.set_textview_font()

self.adapter.setDropDownViewResource(R.layout.simple_spinner_dropdown_item)

def apply_font(self, tv):
if self.impl._font_impl and tv:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How/when does this method get invoked when tv is None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the creation of the widget, this can happen.

def set_font(self, font):
self._font_impl = font._impl
tv = self.native.getSelectedView()
if tv:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above - how/when is this branch not triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the creation of the widget, I saw that set_font() is called and tv was null

probe.assert_font_size(SYSTEM_DEFAULT_FONT_SIZE)

# Reset to original family
del widget.style.font_family
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clearly makes the test pass, but I think it is masking the underlying problem.

Running the test in --slow mode can be illustrative, as you can see the changes that are happening at a speed that is observable.

In this case, using --slow highlights that there's a problem. If you revert your change to this test, you can see that after deleting the two font properties, the widget is still tall - that is, the test failure is reporting a problem correctly.

If you change the order of the deletions - deleting the family, then the size - the widget height is correct, and the test passes.

So - introducing the extra probe redraw here actually isn't the fix. It's the fact that you've changed the order of the deletions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made all kinds of changes to test_font() and although, it visually looked ok (with --slow), the test failed when checking the resetted widget height. Doing the same kind of changes in the selection example works, though. There, the widget height does not only look ok, Spinner.getHeight() in fact returns its original value.

So, if you've found a better way to make the testbed work, please apply these changes to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to your own personal setup - but the CI setup reveals the problem, and I can reproduce the problem locally, so this isn't an oddity of the testbed. There's a problem here that needs to be resolved.

What you're looking for is the amout of space above the text in the widget. The text has the right sized font, but there's a border above the widget (and presumably below as well), which effectively represents the old widget size, with the smaller text vertically centered in that space.

As for a fix... you're the person proposing the change. It's up to you to fix problems that are found. I'm telling you by way of code review that the change you've made to the test suite isn't acceptable. Maybe @mhsmith has some more ideas about places to look for a fix, but we both have other development priorities (not the least of which is finally getting the next Toga release out the door), so we're not in a position to drop everything to hunt a bug that isn't a high priority for us personally.

Changed example to reveal problem in widget height
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants