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

Selection.set_font() for Android and Windows #1758

Closed
wants to merge 5 commits into from

Conversation

t-arn
Copy link
Contributor

@t-arn t-arn commented Jan 30, 2023

This PR adds Selection.set_font() for Android and Windows.
It requires the PR beeware/briefcase-android-gradle-template#61

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 Jan 30, 2023

@freakboy3742 Please review this PR.
The Android testbed failed because this PR requires the PR beeware/briefcase-android-gradle-template#61

I would love to avoid the template change, but according to @mhsmith, Chaquopy cannot extend classes that don't have a zero-argument constructor.

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.

Thanks for the contribution - I can tell you've put a lot of work into this, but I'm unlikely to merge this PR as it's currently proposed.

As I've indicated several times when this subject has come up - adding stubs to the app template isn't something that I'd consider a viable path forward.

Yes, this is due to a current limitation in Chaquopy (historically, that limitation was in Rubicon Java, but Chaquopy has similar limitations for similar reasons). We need to address that limitation, not to patch around it.

Yes, Canvas uses a stub in the template; however, that stub is (a) a fairly generic stub, and (b) not a case that I'd consider a precedent for other widgets. I'd like to see the Canvas stub removed, not another stub added.

I might entertain this as a workaround if this was needed to support some core piece of functionality (e.g., in the case of Canvas where it's needed to support the widget at all), or maybe if it was a bare-bones stub - however, in this case, it's a non-trivial block of Java code, to support Toga's usage of set_font() on a widget - an API that I'd consider a very low overall priority.

Overriding the style (color, font, size etc) of individual widgets is, in my opinion, a bad idea in the general case. Platforms provide a platform style, and Toga endeavours to honour those platform style preferences to the extent possible. There are legitimate reasons to support custom fonts, and for that reason set_font() is an API that needs to exist - but "I prefer the look of this other font" is, in my opinion, a really bad reason to change a font, but is the primary reason that the API gets used. Apps that override those default preferences should have a really good reason to do so.

So - from my perspective, this PR is effectively "introduce an undesirable hack to work around a problem in an API that probably shouldn't be used in the first place".

I'm sorry I couldn't provide a more positive review for this. I thought I'd been clear about avoiding purpose-specific stubs in the app template; so if I wasn't successful in communicating that, I apologise. You've clearly worked out the mechanics for how to change fonts on the Selection widget - we just need to do is work out how to accomodate that logic in a way that doesn't require modifying the app template.

@t-arn
Copy link
Contributor Author

t-arn commented Jan 31, 2023

I agree that extending the template is not the way to go and that we should rather enhance Chaquopy to support class extension for classes with no zero-argument constructor. Is this on the roadmap? When could this be implemented?

Regarding the set_font() method: The reason I need this is not for setting some queer font. I need it for setting the font size. In my Android apps, I allow setting a custom font size because for many (elderly) people the standard size is too small to read. And it really doesn't look good when everything has the size 18 and the selection has 14.

@freakboy3742
Copy link
Member

I agree that extending the template is not the way to go and that we should rather enhance Chaquopy to support class extension for classes with no zero-argument constructor. Is this on the roadmap? When could this be implemented?

It's definitely on the wishlist, but it's not on the short term roadmap. I wouldn't expect to see it in 2023Q1 unless someone submits a PR for it.

Regarding the set_font() method: The reason I need this is not for setting some queer font. I need it for setting the font size. In my Android apps, I allow setting a custom font size because for many (elderly) people the standard size is too small to read. And it really doesn't look good when everything has the size 18 and the selection has 14.

That's totally understandable as a use case - but completely the wrong way to fix it. Every operating system has controls for the default font size, specifically to accomodate accessibility issues like the one you describe. It should not require the developer to manually set font sizes for every widget in their app.

If Toga isn't honoring the system default font size - that's definitely a bug, and one that I'd consider a much higher priority than Java subclassing (and, from a quick look, this appears that it may be the case - font size has been hardcoded as 14pt, and doesn't appear to take system defaults or device scaling into account).

@freakboy3742 freakboy3742 added not quite right The idea or PR has been reviewed, but more work is needed. android The issue relates to Android mobile support. labels May 3, 2023
@mhsmith
Copy link
Member

mhsmith commented Sep 11, 2023

Instead of subclassing ArrayAdapter, we could just subclass SpinnerAdapter, which is possible because it's an interface. Then we can forward all of its methods to a separate ArrayAdapter, and modify the Views it returns to apply font, color and alignment.

@mhsmith mhsmith mentioned this pull request Sep 11, 2023
11 tasks
@t-arn
Copy link
Contributor Author

t-arn commented Sep 20, 2023

@mhsmith That's an interesting idea! I will try to implement this.

@t-arn
Copy link
Contributor Author

t-arn commented Sep 23, 2023

@mhsmith I succeeded in creating a class TogaArrayAdapter by extending the interface SpinnerAdapter :-)
The font setting is not working correctly yet, but the beginning is promising.

@t-arn
Copy link
Contributor Author

t-arn commented Sep 24, 2023

@mhsmith It works now :-)
I just submitted PR #2130 which replaces this PR here.

@freakboy3742
Copy link
Member

Closing on the basis that it has been superceded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android The issue relates to Android mobile support. not quite right The idea or PR has been reviewed, but more work is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants