Fix font loading normalization and improve font screen UX - #26
Closed
relativemodder wants to merge 5 commits into
Closed
Fix font loading normalization and improve font screen UX#26relativemodder wants to merge 5 commits into
relativemodder wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughImported fonts now normalize inflated vertical metrics and show progress placeholders during import. The navigation drawer gains proxy controls with state synchronization, and proxy list cells no longer override MD3 section backgrounds. ChangesFont import pipeline
Proxy drawer integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FontsSettingsActivity
participant FontLibrary
participant FontImportSkeletonRow
User->>FontsSettingsActivity: pick font URIs
FontsSettingsActivity->>FontImportSkeletonRow: show import placeholders
FontsSettingsActivity->>FontLibrary: import fonts in background
FontLibrary-->>FontsSettingsActivity: return imported faces
FontsSettingsActivity->>FontsSettingsActivity: refresh font roster
sequenceDiagram
participant User
participant DrawerProxyCell
participant DrawerHelper
participant ConnectionsManager
participant NotificationCenter
User->>DrawerProxyCell: toggle proxy switch
DrawerProxyCell->>DrawerHelper: report enabled state
DrawerHelper->>ConnectionsManager: apply proxy configuration
DrawerHelper->>NotificationCenter: post proxySettingsChanged
NotificationCenter-->>DrawerHelper: refresh drawer state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kotlin/ui/settings/fonts/FontsSettingsActivity.kt (1)
280-305: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlways clear import progress when background processing fails.
If
FontLibrary.importFromUristhrows, the UI callback never runs;importPlaceholderCountremains nonzero and Add stays disabled for the process lifetime. Wrap the import intry/finallyor return a failure result, then clear placeholders and show the failure bulletin on the UI thread.🤖 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/ui/settings/fonts/FontsSettingsActivity.kt` around lines 280 - 305, Ensure the import flow around FontLibrary.importFromUris always clears import progress when background processing throws. Catch failures or use finally, then dispatch setImportPlaceholderCount(0, notifyOthers = true) and the existing InuFontImportFailed bulletin on the UI thread, while preserving the current success and rejection handling.
🤖 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 `@FEATURES.md`:
- Line 221: Update the FEATURES.md claim to refer specifically to standalone
TTF/OTF fonts rather than imported fonts generally. Keep the statement focused
on inflated vertical metrics not adding bogus top/bottom padding, and do not
imply TTC collection support.
In `@src/kotlin/helpers/font/FontLibrary.kt`:
- Around line 371-380: Replace the two debug logging calls in
normalizeVerticalMetrics with android.util.Log.d, including the normalized and
unknown-magic messages. Leave the existing FileLog.e error logging unchanged.
- Around line 428-448: The normalization logic around shouldNormalizeHeadBox and
shouldNormalizeWinBox must account for actual glyph extents before reducing font
bounds. Calculate or preserve the font’s contour extrema, clamp targetAsc and
targetDesc so the head table contains every glyph, and ensure
usWinAscent/usWinDescent remain large enough to avoid clipping before writing
the fields.
- Around line 367-369: TTC containers are currently skipped by the font
normalization logic in FontLibrary.kt around the readInt magic check; either
implement traversal of each TTC face so affected table checksums are updated, or
explicitly exclude/reject TTC files. If TTC support is not added, update
FEATURES.md at line 221 to claim normalization only for standalone TTF/OTF
files; both listed sites require corresponding changes.
- Around line 477-495: Update tableChecksum to seek to offset once, then process
the padded table data through a reusable buffered byte array with sequential
reads instead of seeking for each four-byte word. Preserve the existing
big-endian checksum calculation, zero-padding of the final partial word, and
32-bit masking behavior.
In `@src/kotlin/ui/drawer/DrawerProxyCell.kt`:
- Around line 46-48: Update the child layout parameters in DrawerProxyCell to
respect the existing isRTL flag: swap LEFT/RIGHT gravities and exchange
corresponding left/right margins for imageView, textView, and checkBox. Preserve
the current dimensions, vertical positioning, and spacing in LTR while ensuring
mirrored placement and non-overlapping views in RTL.
---
Outside diff comments:
In `@src/kotlin/ui/settings/fonts/FontsSettingsActivity.kt`:
- Around line 280-305: Ensure the import flow around FontLibrary.importFromUris
always clears import progress when background processing throws. Catch failures
or use finally, then dispatch setImportPlaceholderCount(0, notifyOthers = true)
and the existing InuFontImportFailed bulletin on the UI thread, while preserving
the current success and rejection handling.
🪄 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: dca46331-96a7-4d63-96be-d25b7879dcb9
📒 Files selected for processing (12)
FEATURES.mdpatches/bugfix/proxy-list-md3-sections.patchseriessrc/kotlin/helpers/dialogs/DrawerHelper.ktsrc/kotlin/helpers/font/FontLibrary.ktsrc/kotlin/ui/drawer/DrawerLayoutAdapter.ktsrc/kotlin/ui/drawer/DrawerProxyCell.ktsrc/kotlin/ui/settings/fonts/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
Comment on lines
+367
to
+369
| when (val magic = raf.readInt()) { | ||
| 0x74746366 -> Unit // TTC checkSumAdjustment is per-face; skip rather than risk corrupting it. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Either normalize TTC faces or narrow the feature scope.
The implementation explicitly skips TTC containers while the feature documentation covers imported fonts generally.
src/kotlin/helpers/font/FontLibrary.kt#L367-L369: traverse each TTC font directory and update affected table checksums, or explicitly reject/exclude TTC normalization.FEATURES.md#L221-L221: limit the claim to standalone TTF/OTF files until TTC support exists.
📍 Affects 2 files
src/kotlin/helpers/font/FontLibrary.kt#L367-L369(this comment)FEATURES.md#L221-L221
🤖 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/FontLibrary.kt` around lines 367 - 369, TTC
containers are currently skipped by the font normalization logic in
FontLibrary.kt around the readInt magic check; either implement traversal of
each TTC face so affected table checksums are updated, or explicitly
exclude/reject TTC files. If TTC support is not added, update FEATURES.md at
line 221 to claim normalization only for standalone TTF/OTF files; both listed
sites require corresponding changes.
Source: Coding guidelines
Owner
|
merged in 39ef155, thanks! |
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
New Features
Bug Fixes