fix(android): fix setting font#16188
Open
ermshiperete wants to merge 3 commits into
Open
Conversation
keymanapp-test-bot
Bot
added
the
user-test-missing
User Test ResultsTest specification and instructions
Results Template |
keymanapp-test-bot
Bot
added
has-user-test
user-test-required
ermshiperete
force-pushed
the
fix/android/16187_font
branch
2 times, most recently
from
92edddc to
a26d081
Compare
PR #16146 introduced a problem with selecting a different font as display font so that we always ended up with not setting the font. This was caused by the font filenames now being a URL (which is necessary because they get processed by the web engine). However, the Android code checks for the existence of the font in order to create the typeface, which only works for local paths. This PR modifies and simplifies `KMKeyboard.getFontFilename` to return the full path, renames `KMKeyboard.txtFont` and `KMKeyboard.oskFont` to make it clearer that they contain a path and not a URL. Also initialize `KMKeyboard.oskFontPath` with empty string instead of `null`. This makes it consistent with` txtFontPath` and with the documented behavior of `KMManager.getKeyboardOskFontFilename()` (which returns `KMKeyboard.oskFontPath`). Follows: #16146 Fixes: #16187 Build-bot: release:android
ermshiperete
force-pushed
the
fix/android/16187_font
branch
from
a26d081 to
1191e43
Compare
mcdurdin
reviewed
Jul 3, 2026
| /** | ||
| * Returns the filename without path of the display font of the current keyboard. | ||
| */ | ||
| private String getKeyboardTextFontFilename() { |
Member
There was a problem hiding this comment.
Should we call this getKeyboardTextFontFilenameOnly() because of the legacy of KMManager.getKeyboardTextFontFilename() returning a full path?
Comment on lines
+933
to
+935
| // REVIEW: Do we have to do anything if font is a JSONObject? | ||
| // See makeFontObj. | ||
|
|
Member
There was a problem hiding this comment.
I don't really understand this question
Contributor
Author
There was a problem hiding this comment.
In makeFontObj we have this questionable block of code that deals with a font as JSONObject. If that is still needed, then we'll likely have to do something here with a JSONObject as well.
Any suggestion for a better review or todo comment?
Comment on lines
+1069
to
+1071
| // REVIEW: Why do we need the complicated code below? Can this still | ||
| // happen, or can we remove it? (see also getFontFilename) | ||
| KMLog.LogInfo(TAG, "Got font without font extension: " + font); |
Member
There was a problem hiding this comment.
Let's open an issue on this to follow up in the future once we have some logging. (Note that LogInfo doesn't go to Sentry though? Can we capture a Sentry event?)
Contributor
Author
There was a problem hiding this comment.
LogInfo() calls Sentry.captureMessage, so I think it should end up on Sentry.
| if (fontFilename.startsWith("http://") || fontFilename.startsWith("https://") | ||
| || fontFilename.startsWith("file://")) { | ||
| // Font file is not local, so cannot load Typeface | ||
| KMLog.LogError(TAG, "Font file is not local: " + fontFilename); |
Contributor
Author
There was a problem hiding this comment.
Yes, also has a Sentry.captureMessage call.
Co-authored-by: Marc Durdin <marc@durdin.net> Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>
This addresses a code review comment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #16146 introduced a problem with selecting a different font as display font so that we always ended up with not setting the font. This was caused by the font filenames now being a URL (which is necessary because they get processed by the web engine). However, the Android code checks for the existence of the font in order to create the typeface, which only works for local paths.
This PR modifies and simplifies
KMKeyboard.getFontFilenameto return the full path, renamesKMKeyboard.txtFontandKMKeyboard.oskFontto make it clearer that they contain a path and not a URL.Also initialize
KMKeyboard.oskFontPathwith empty string instead ofnull. This makes it consistent withtxtFontPathand with the documented behavior ofKMManager.getKeyboardOskFontFilename()(which returnsKMKeyboard.oskFontPath).Follows: #16146
Replaces: #16178
Fixes: #16187
Build-bot: release:android
User Testing
TEST_KEYBOARDHARNESS:
install the KeyboardHarness apk from this PR
Switch to the
longpress '"|5% +keyboardverify that the OSK uses the Code2001 font (compare with screenshot below)
type gbqJ and verify that the Code2001 font is used (compare with screenshot)
TEST_KEYMAN:
TEST_FV: