initial font library implementation#19
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR transforms font management from a single-pack system to a multi-family architecture. FontHelper now stores per-family directories under ChangesMulti-Family Font Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/feature/custom-font.patch (1)
23-30: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename the new stock overload to
inu_getTypeface.This adds a fresh overload on a stock class without the required
inu_prefix, which will make future rebases harder than necessary. Rename the overload and its new call sites while the surface is still small.As per coding guidelines, "If you must add fields/methods to stock classes, prefix them with inu_ (including overloads, e.g., inu_addTab)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/feature/custom-font.patch` around lines 23 - 30, The new public overload getTypeface(String assetPath, boolean inu_forPaint) must be renamed to inu_getTypeface to follow the inu_ prefix rule; update the implementation and all new call sites accordingly (change the method declaration name and any calls that pass inu_forPaint), keep the same parameter names and behavior around typefaceCache, the inu_override lookup via desu.inugram.helpers.font.FontHelper.onGetTypeface, and the early return logic intact so only the identifier changes; ensure the original getTypeface(String) continues to delegate to inu_getTypeface(assetPath, false).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kotlin/helpers/font/FontHelper.kt`:
- Around line 367-429: importFromUris mutates shared collections (families,
rosterTokens, hiddenTokens) off the globalQueue and must instead build new state
locally and publish it under synchronization or on the main thread; change
importFromUris to collect files, create targetDir/faces, and assemble a local
map/list of changes (e.g. newFamilies: Map<id,Family>, updatedFamilyDirs,
addedRosterTokens: List<String>, addedFaces count) without touching the shared
families/rosterTokens/hiddenTokens, then after all file I/O complete apply those
changes inside a synchronized block or by posting a single update to the main
thread (e.g. runOnUiThread or a single lock around families) where you call
readFamily(targetDir) and assign families[targetId] = it, mutate
rosterTokens/hiddenTokens, call saveRoster() and any
rosterTokens.add("font:$targetId") only from that synchronized/main-thread
publish step; reference functions/vars: importFromUris, families, rosterTokens,
hiddenTokens, readFamily, writeManifest, saveRoster, newId so the code that
renames/copies files remains the same but shared collections are only mutated in
one atomic, synchronized publish.
In `@src/kotlin/InuConfig.kt`:
- Around line 505-507: ACTIVE_FONT_ID is currently created as an exportable
StringItem causing backups to carry a local font-family id; change it to be
non-exportable and add a normalization step when loading font settings: update
the ACTIVE_FONT_ID declaration (StringItem("active_font_id", "")) to use the API
to mark it non-exportable (e.g., pass/export flag or call setExportable(false)
on ACTIVE_FONT_ID) and in the font-loading path that respects FONT_MODE (the
code that reads/uses ACTIVE_FONT_ID when FONT_MODE == 2) validate the stored id
against available font families and fallback to "" (or force FONT_MODE != 2) if
the id is missing.
In `@src/kotlin/ui/settings/FontsSettingsActivity.kt`:
- Around line 144-149: The option added in FontsSettingsActivity via opts.add
currently appears for every roster token but only works correctly for imported
families; update the code that adds the app-font target (the opts.add block that
sets InuConfig.FONT_MODE, InuConfig.ACTIVE_FONT_ID, calls
listView.adapter.update(true) and showRestartBulletin()) to only be created for
tokens the main picker can represent — e.g., check the token value or type
before adding (only allow tokens that start with "font:" or pass whatever
isFontToken(token) predicate your model uses); alternatively, if you prefer
keeping the option for built-ins, extend showAppFontPicker()/its model to
include built-in families so the picker can display/select them consistently.
---
Outside diff comments:
In `@patches/feature/custom-font.patch`:
- Around line 23-30: The new public overload getTypeface(String assetPath,
boolean inu_forPaint) must be renamed to inu_getTypeface to follow the inu_
prefix rule; update the implementation and all new call sites accordingly
(change the method declaration name and any calls that pass inu_forPaint), keep
the same parameter names and behavior around typefaceCache, the inu_override
lookup via desu.inugram.helpers.font.FontHelper.onGetTypeface, and the early
return logic intact so only the identifier changes; ensure the original
getTypeface(String) continues to delegate to inu_getTypeface(assetPath, false).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb20b5cd-9c17-4324-9763-75663d07fbc6
📒 Files selected for processing (8)
FEATURES.mdpatches/feature/custom-font.patchsrc/kotlin/InuConfig.ktsrc/kotlin/SearchRegistry.ktsrc/kotlin/helpers/font/FontHelper.ktsrc/kotlin/ui/settings/AppearanceSettingsActivity.ktsrc/kotlin/ui/settings/FontsSettingsActivity.ktsrc/res/values/strings_inu.xml
teidesu
force-pushed
the
editor-custom-fonts
branch
from
b07fe89 to
b8814ad
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/kotlin/helpers/font/FontHelper.kt (1)
136-154: ⚖️ Poor tradeoffHardcoded user-facing face labels should go through
LocaleController.
faceLabelreturns user-visible strings ("Variable", "Thin"…"Black", "Italic", "$w Italic") shown in the tap menu. These bypass the project's string-resource path.As per coding guidelines, "Access strings via LocaleController.getString(R.string.InuXxx) in Kotlin/Java code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kotlin/helpers/font/FontHelper.kt` around lines 136 - 154, Replace hardcoded user-facing labels in faceLabel(Face) with localized strings via LocaleController.getString(...) using appropriate R.string entries (e.g., R.string.font_variable, R.string.font_thin, font_extralight, font_light, font_regular, font_medium, font_semibold, font_bold, font_extrabold, font_black, font_italic, font_italic_combined). Update the logic in faceLabel to map the conditions on f.variable, f.weight, and f.italic to those resource IDs and return LocaleController.getString(resourceId) (or a formatted localized string for combined weight+italic) instead of returning literal strings like "Variable", "Thin", "$w Italic".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/kotlin/helpers/font/FontHelper.kt`:
- Around line 136-154: Replace hardcoded user-facing labels in faceLabel(Face)
with localized strings via LocaleController.getString(...) using appropriate
R.string entries (e.g., R.string.font_variable, R.string.font_thin,
font_extralight, font_light, font_regular, font_medium, font_semibold,
font_bold, font_extrabold, font_black, font_italic, font_italic_combined).
Update the logic in faceLabel to map the conditions on f.variable, f.weight, and
f.italic to those resource IDs and return LocaleController.getString(resourceId)
(or a formatted localized string for combined weight+italic) instead of
returning literal strings like "Variable", "Thin", "$w Italic".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a11c7c61-423c-4c3e-b871-437f15518ce3
📒 Files selected for processing (8)
FEATURES.mdpatches/feature/custom-font.patchsrc/kotlin/InuConfig.ktsrc/kotlin/SearchRegistry.ktsrc/kotlin/helpers/font/FontHelper.ktsrc/kotlin/ui/settings/AppearanceSettingsActivity.ktsrc/kotlin/ui/settings/FontsSettingsActivity.ktsrc/res/values/strings_inu.xml
✅ Files skipped from review due to trivial changes (2)
- src/res/values/strings_inu.xml
- FEATURES.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/kotlin/InuConfig.kt
- src/kotlin/SearchRegistry.kt
- src/kotlin/ui/settings/AppearanceSettingsActivity.kt
- src/kotlin/ui/settings/FontsSettingsActivity.kt
polina4096
force-pushed
the
editor-custom-fonts
branch
from
b8814ad to
f7cba01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patches/feature/custom-font.patch`:
- Line 161: The static field inu_loadGeneration needs to be declared as volatile
to ensure thread-safe visibility across the UI thread and theme queue thread.
Currently, inu_loadGeneration is accessed from multiple threads (read on the
theme queue thread at line 168, written by inu_invalidate() on the UI thread,
and read again on the UI thread at line 180) without synchronization, which
could lead to stale values being used. Add the volatile keyword to the
inu_loadGeneration field declaration to guarantee that all threads see the most
up-to-date value and prevent stale async builds from committing after the
generation has been incremented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63152aad-da7d-4146-aeae-96735c132150
📒 Files selected for processing (13)
FEATURES.mdpatches/feature/custom-font.patchsrc/kotlin/InuConfig.ktsrc/kotlin/InuHooks.ktsrc/kotlin/SearchRegistry.ktsrc/kotlin/helpers/NotificationsHelper.ktsrc/kotlin/helpers/font/FontHelper.ktsrc/kotlin/ui/settings/AppearanceSettingsActivity.ktsrc/kotlin/ui/settings/FontsSettingsActivity.ktsrc/res/values-ja/strings_inu.xmlsrc/res/values-ru/strings_inu.xmlsrc/res/values-zh-rCN/strings_inu.xmlsrc/res/values/strings_inu.xml
✅ Files skipped from review due to trivial changes (4)
- src/res/values-zh-rCN/strings_inu.xml
- src/kotlin/helpers/NotificationsHelper.kt
- src/kotlin/SearchRegistry.kt
- FEATURES.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/kotlin/InuConfig.kt
- src/res/values/strings_inu.xml
- src/kotlin/ui/settings/AppearanceSettingsActivity.kt
- src/kotlin/helpers/font/FontHelper.kt
|
|
||
| private static List<PaintTypeface> typefaces; | ||
| public static boolean loadingTypefaces; | ||
| + private static int inu_loadGeneration = 0; |
There was a problem hiding this comment.
Make inu_loadGeneration volatile for cross-thread visibility.
inu_loadGeneration is read on the theme queue thread (line 168), written on the UI thread (inu_invalidate()), and read again on the UI thread for comparison (line 180). Without volatile or synchronization, visibility is not guaranteed, which could allow a stale async build to commit after inu_invalidate() has incremented the generation.
🔒 Proposed fix
- private static int inu_loadGeneration = 0;
+ private static volatile int inu_loadGeneration = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| + private static int inu_loadGeneration = 0; | |
| private static volatile int inu_loadGeneration = 0; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patches/feature/custom-font.patch` at line 161, The static field
inu_loadGeneration needs to be declared as volatile to ensure thread-safe
visibility across the UI thread and theme queue thread. Currently,
inu_loadGeneration is accessed from multiple threads (read on the theme queue
thread at line 168, written by inu_invalidate() on the UI thread, and read again
on the UI thread at line 180) without synchronization, which could lead to stale
values being used. Add the volatile keyword to the inu_loadGeneration field
declaration to guarantee that all threads see the most up-to-date value and
prevent stale async builds from committing after the generation has been
incremented.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/feature/custom-font.patch (1)
145-152:⚠️ Potential issue | 🔴 CriticalVerify that
getName()handlesnameKey=nullfor imported fonts.The new constructor (lines 98–106) sets
nameKey = nullwhilegetName()at line 149 returnsLocaleController.getString(nameKey)without checking for null. This means callinggetName()on imported fonts created with this constructor will passnulltoLocaleController.getString(), likely causing a null-pointer exception or returning null/empty string. Add a null check or fallback to thenamefield.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/feature/custom-font.patch` around lines 145 - 152, The getName() method does not check if nameKey is null before calling LocaleController.getString(nameKey), which will fail when importing fonts using the new PaintTypeface constructor (that sets nameKey to null). Update the getName() method to first check if nameKey is null, and if so, return the name field directly as a fallback; otherwise, proceed with the existing LocaleController.getString(nameKey) call to support both localized and imported font names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patches/feature/custom-font.patch`:
- Around line 145-152: The getName() method does not check if nameKey is null
before calling LocaleController.getString(nameKey), which will fail when
importing fonts using the new PaintTypeface constructor (that sets nameKey to
null). Update the getName() method to first check if nameKey is null, and if so,
return the name field directly as a fallback; otherwise, proceed with the
existing LocaleController.getString(nameKey) call to support both localized and
imported font names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 268656f6-8137-4a68-a52f-12cfbfb31eaa
📒 Files selected for processing (2)
patches/feature/custom-font.patchscripts/config.ts
polina4096
force-pushed
the
editor-custom-fonts
branch
2 times, most recently
from
578e592 to
7a0067f
Compare
teidesu
force-pushed
the
editor-custom-fonts
branch
from
7a0067f to
33d5129
Compare
teidesu
force-pushed
the
editor-custom-fonts
branch
from
33d5129 to
cab985b
Compare
teidesu
pushed a commit
that referenced
this pull request
Jun 20, 2026
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
2 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.
Summary by CodeRabbit