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

Font improvements #4884

Open
wants to merge 3 commits into
base: 14-dev
Choose a base branch
from
Open

Conversation

tgex0
Copy link
Contributor

@tgex0 tgex0 commented Oct 10, 2024

Description

I went deep down a rabbit hole investigating a bug I reported (#4854) and I found several other small related problems.
I hope you will consider this PR which fixes the problems I found and updates the internal Inter font and the Google Fonts list.
Please let me know your thoughts. Thank you.

This PR addresses the following bugs (from a user's point of view):

  1. There are two Inter fonts (with exactly the same name) listed side by side in the font list.
  2. Selecting either one of the Inter font radio buttons selects them both.
  3. Both Inter fonts are Google Fonts (neither is the internal Inter font).
  4. The additional Inter font lists only five variants (the Inter font supports 18 variants).
  5. Two of the five variants listed for the additional Inter font are italic but neither of them actually display as italic.
  6. On the first visit to the font list, no radio button is selected (even though there are two Inter fonts listed and the default font is Inter).
  7. The variant label 'regular' is not displayed alongside the family for the default font on the General settings page (inconsistent as the variant label 'medium' is displayed for the default font and 'regular' is displayed for all other fonts).

This PR implements the following (from a dev's point of view):

1. Updated the internal Inter font from v3.12 to v4.0 (v4.0 uses the same license as v3.2).
2. Ensured that the default font labels (on the General settings page) display the selected variant as well as the family. Previously "Regular" was missing (for default font only.. it was present for all other fonts).
3. Renamed the internal Inter font from "Inter" to "Inter v4.0" "Inter v3" (just via labelling... no font files have been modified from their original source versions) to differentiate it from the Google Fonts version. Renaming the default font labels (on the General settings page) and the font itself in the font list ensures that on the first visit to the font list, a radio button is selected. Previously, on the first visit to the font list, no radio button was selected.
4. Removed the duplicate Google Fonts Inter font (with italic variants which did not work) from the font list to prevent duplicate font names being listed and to prevent two radio buttons from being selected at the same time.
5. Added the internal Inter font (with all 18 supported the 4 available variants) to the font list (after system fonts and before Google Fonts in the list) as it was previously unavailable. This givers users a way to reinstate the default font after selecting a different font.
6. Updated the Google Fonts list (generated directly using the Google Fonts API on 8 October 2024). The font family count has increased from 1646 to 1726 (80 new fonts).
7. Allowed AssetFont and ResourceFont font types to be included in the font list. Previously, they could not be selected via their corresponding radio buttons on the first try. The radio button would look to be selected but on moving away from the font list, it would be evident that the font had not been applied and, going back to the font list, the radio button would no longer be selected. The font would need to be selected for a second time before it worked as expected. This needed to be fixed as the internal Inter font is added to the font list as a ResourceFont. (Fix also implemented for AssetFont for potential future use.)
8. Used translatable strings for internal Inter font variant labels (same as Google Font variant labels).

Fixes #4854

Type of change

❌ General change (non-breaking change that doesn't fit the below categories like copyediting)
✅ Bug fix (non-breaking change which fixes an issue)
❌ New feature (non-breaking change which adds functionality)
❌ Breaking change (fix or feature that would cause existing functionality to not work as expected)

@validcube
Copy link
Contributor

I've tried to bump the Inter font from v3.12 to v4.00 before but in my opinion, the original Inter 3.16 font the Inter font v4.x made the app label of the truncated app too cramped (sees Authenticat... label)

I think the original font should be kept and the user can update to Inter v4.00 by switching the font to Inter (from Google Fonts)

image

image

@tgex0
Copy link
Contributor Author

tgex0 commented Oct 10, 2024

Thanks for your input. It's most appreciated.
It looks like the last v3 version is v3.19.
Would you like me to adjust the PR to include v3.19 instead or do you think v3.12 (the current version) would still be better?
Your post above mentions v3.16. Do you think this might be a contender?

@tgex0
Copy link
Contributor Author

tgex0 commented Oct 10, 2024

I'll set this PR to draft until the font version is decided upon.

@tgex0 tgex0 marked this pull request as draft October 10, 2024 20:24
@tgex0
Copy link
Contributor Author

tgex0 commented Oct 11, 2024

I have reverted the internal Inter font to v3.12 with the original variants only (bold, medium, regular and semi bold).
This PR no longer changes any internal Inter font files.
I've updated the OP to reflect the current PR changes. Check strikethroughs for changes.

@tgex0 tgex0 marked this pull request as ready for review October 11, 2024 16:44
@tgex0 tgex0 marked this pull request as draft October 12, 2024 09:43
@tgex0
Copy link
Contributor Author

tgex0 commented Oct 12, 2024

Additional commit to use translatable strings for internal Inter font variant labels (same as Google Font variant labels).

@tgex0 tgex0 marked this pull request as ready for review October 12, 2024 10:40
@validcube
Copy link
Contributor

validcube commented Oct 12, 2024

Thanks for your input. It's most appreciated. It looks like the last v3 version is v3.19. Would you like me to adjust the PR to include v3.19 instead or do you think v3.12 (the current version) would still be better? Your post above mentions v3.16. Do you think this might be a contender?

👋 Hello!

I haven't tested Inter v3.19 yet but I'm sure it won't be much of a difference other than bug fixes and new glyph.

Also unfortunately at the time of writing that message it was about like 3 AM for me so my English might be very broken so sorry about that.

So here's my revised message if you're interested

In my opinion, the Inter font v4.x made the app label of the truncated app too cramped for me (sees the image specifically the Authenticat... label)

I think the original font should be kept until there's a better way to improve readability and the user can change to the latest Inter font by switching the default to one provided by the Google Fonts' listing

Also sorry again for the slow response, my availability is very sparse at the moment.

@tgex0
Copy link
Contributor Author

tgex0 commented Oct 12, 2024

Thanks for your reply.

I think we should probably leave the internal Inter font exactly as it is at the moment (v3.12) and just allow this PR to fix the bugs.
The font can perhaps be updated sometime in the future in a separate PR.

Being that this PR addresses several different issues, I just thought it was a good opportunity to update the internal Inter font at the same time but, in hindsight (and with your information and screenshots), this does not sound like such a good idea after all.

I see no harm in updating the Google Fonts list just now, though. This would allow users to select Inter v4.0 if desired. The current Google Fonts list uses Inter v3.19 but the new Google Fonts list (in this PR) uses Inter v4.0 (labelled by Google as v18).

Cheers!

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.

[BUG] Duplicate font listed ("Inter")
2 participants