feat: Add custom font support with iTerm2-style font picker (#103)

[kshivang]

Summary

• Add fontName setting to TerminalSettings for custom system fonts
• Create FontUtils.kt with font listing and loading utilities using Skia FontMgr
• Add SettingsSectionedDropdown component with iTerm2-style grouped font selection
• Fonts organized into: Bundled, Fixed Pitch, Variable Pitch sections
• Variable pitch fonts work with fixed cell width rendering (same approach as iTerm2)

Changes

• FontUtils.kt (new): Font listing, categorization, and loading
• SettingsComponents.kt: Added SettingsSectionedDropdown component
• VisualSettingsSection.kt: Updated to use sectioned font dropdown
• TerminalSettings.kt: Added fontName setting
• EmbeddableTerminal.kt/TabbedTerminal.kt: Refactored font loading
• README.md: Added fontName configuration example

Test plan

• Open Settings → Visual → Font section
• Verify font dropdown shows Bundled, Fixed Pitch, Variable Pitch sections
• Select a monospace font (e.g., SF Mono, Menlo) and verify terminal updates
• Select a variable pitch font and verify it renders with fixed cell width
• Verify bundled font option works correctly

Closes #103

🤖 Generated with Claude Code

[claude]

Code Review - PR #112: Custom Font Support

Great work on adding custom font support! This is a well-designed feature with a clean architecture. Here's my detailed review:

✅ Strengths

1. Excellent Architecture

   • Clean separation of concerns with FontUtils.kt handling all font logic
   • Smart refactoring to eliminate duplicate loadTerminalFont() implementations
   • Proper use of Skia's FontMgr API for cross-platform font access
   • Good use of Compose's remember with proper keys for font caching

2. User Experience

   • iTerm2-style categorized font picker is intuitive and professional
   • Fallback handling ensures robustness (bundled font → FontFamily.Monospace)
   • Proper documentation in README with clear examples

3. Code Quality

   • Clear, self-documenting function names and comments
   • Proper error handling with informative error messages
   • Deprecated old APIs with migration path

🔍 Issues & Concerns

1. Performance: Font Scanning on Every Settings Open (Critical)

Location: VisualSettingsSection.kt:47

val categorizedFonts = remember { getCategorizedFonts() }

Problem: getCategorizedFonts() scans ALL system fonts, loads each typeface, and measures glyph widths. On macOS this could be 200+ fonts, taking 500ms-2s to complete. This runs every time the settings panel is composed.

Impact:

• UI lag when opening settings
• Wasted CPU on repeated scans
• Multiple settings opens = multiple full scans

Solution: Cache at application level, not composition level:

// In FontUtils.kt
private val categorizedFontsCache: Map<String, List<String>> by lazy {
    getCategorizedFontsInternal()
}

fun getCategorizedFonts(): Map<String, List<String>> = categorizedFontsCache

private fun getCategorizedFontsInternal(): Map<String, List<String>> {
    // Current implementation here
}

Then VisualSettingsSection.kt becomes:

val categorizedFonts = remember { getCategorizedFonts() } // Now instant

2. Unsafe Non-Null Assertions (Medium)

Location: FontUtils.kt:72, 74-76

categorized[FONT_SECTION_BUNDLED]!! + categorized[FONT_SECTION_FIXED_PITCH]!!

Problem: Using !! assumes the map always has these keys. While true today, it's fragile and will crash with NPE if getCategorizedFonts() changes.

Solution:

categorized[FONT_SECTION_BUNDLED].orEmpty() + categorized[FONT_SECTION_FIXED_PITCH].orEmpty()

Or better, use getValue() which documents the requirement:

categorized.getValue(FONT_SECTION_BUNDLED) + categorized.getValue(FONT_SECTION_FIXED_PITCH)

3. Deprecated Functions Should Not Have Active Callers (Low)

Location: FontUtils.kt:69-85

The @deprecated functions (getAvailableFonts, getAvailableMonospaceFonts) still delegate to getCategorizedFonts(), adding unnecessary code paths. Since there are no callers in this PR, consider removing them entirely or adding clear TODOs if they're for external API compatibility.

Recommendation: Remove deprecated functions in this PR or document why they're needed for backward compatibility.

🎯 Suggestions

4. Font Detection Tolerance Could Miss Some Monospace Fonts

Location: FontUtils.kt:45

if (kotlin.math.abs(widthW - widthI) < 0.1f) {

Consideration: Some fonts marketed as monospace may have slightly unequal widths due to kerning or hinting. iTerm2 likely uses a different heuristic. Consider testing with fonts like "Input Mono" or "Iosevka" which have subtle width variations.

Alternative Approach: Check if Skia's Typeface has an isFixedPitch() method (common in font APIs), which would be more reliable than width measurement.

5. Error Handling Silently Skips Fonts

Location: FontUtils.kt:51-53

} catch (e: Exception) {
    // Skip fonts that fail to load
}

Consideration: While skipping is correct behavior, debugging font issues would be easier with optional logging. Consider:

} catch (e: Exception) {
    if (System.getProperty("bossterm.debug.fonts") != null) {
        System.err.println("Skipping font '$familyName': ${e.message}")
    }
}

6. Font Reloading on Settings Change

Location: EmbeddableTerminal.kt:101, TabbedTerminal.kt:69

val sharedFont = remember(settings.fontName) { loadTerminalFont(settings.fontName) }

Positive: ✅ Correctly uses remember key to trigger reload when font changes.

Question: Does this properly trigger terminal recomposition? Verify that changing the font in settings causes all visible terminals to re-render with the new font.

🧪 Testing Recommendations

1. Performance Test: Time settings panel open with 200+ fonts on macOS
2. Edge Cases:
   • Select a font, delete it from system, restart app (should fallback)
   • Set fontName: "NonExistentFont" in settings.json manually
   • Empty font list (VM with minimal fonts)
3. Visual Test: Verify variable pitch fonts render correctly with fixed-width cells
4. Compatibility: Test on Linux/Windows to ensure FontMgr.default works consistently

📝 Documentation

• README examples are clear and helpful ✅
• Consider adding a note about the performance of variable pitch fonts in terminals (they work but may look awkward)
• Settings tooltip in UI should mention powerline/nerd font symbols are only available with bundled font

🏁 Verdict

Overall Assessment: This is a solid PR with good engineering practices. The critical performance issue (#1) should be fixed before merge, but the rest are minor improvements.

Recommendation:

• Must Fix: Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1 (font caching)
• Should Fix: Issue 🔍 Implement text search/find functionality (Ctrl+F) #2 (non-null assertions)
• Nice to Have: Issues 🔗 Implement hyperlink detection and click handling #3-5

After addressing #1 and #2, this is ready to merge! 🚀

---

Reviewed against BossTerm conventions from CLAUDE.md

[claude]

Pull Request Review: Custom Font Support (#103)

Summary

This PR adds custom font support with an iTerm2-style font picker. The implementation is well-structured with clean separation of concerns and proper error handling. Overall, this is high-quality work with a few minor suggestions for improvement.

---

Strengths

1. Excellent Code Organization - Clear separation with FontUtils.kt for font logic, SettingsSectionedDropdown for UI. DRY principle applied by extracting duplicate loadTerminalFont() into shared utility.

2. Robust Error Handling - Graceful fallbacks at multiple levels (system font → bundled font → FontFamily.Monospace) with exception handling.

3. Performance Optimization - Font loading cached with remember(settings.fontName) and categorized fonts computed once.

4. Good Documentation - Clear KDoc comments, updated README, appropriate @deprecated annotations.

---

Issues & Suggestions

1. Performance: Font Categorization on UI Thread (Medium Priority)

Location: FontUtils.kt:25-60, VisualSettingsSection.kt:46

Problem: getCategorizedFonts() iterates over all system fonts (potentially 100+) and measures glyph widths for each on UI thread.

Suggestion: Use coroutines for async computation or add module-level caching with lazy initialization.

2. Logic Issue: Non-Nullable Map Access (Medium Priority)

Location: FontUtils.kt:72, 74-76

Problem: Force-unwrapping !! on map lookups could crash if sections are empty.

Suggestion: Use safe access with elvis operator ?: to provide fallback values.

3. Monospace Detection Robustness (Low Priority)

Location: FontUtils.kt:40-49

Problem: Only compares W and i characters. Some fonts might pass but have variable width.

Suggestion: Test additional character pairs (M, l, 1, 0) for better accuracy.

4. UI Consistency: Empty Sections (Low Priority)

Location: SettingsComponents.kt:543-576

Problem: Empty sections still show headers with no items.

Suggestion: Filter out empty sections before rendering.

5. Documentation: Outdated Comment (Trivial)

Location: EmbeddableTerminal.kt:37-39

Problem: Comment mentions fontPath but setting is fontName.

Fix: Update comment to reference fontName.

---

Security Review

No security concerns identified. No user input in file paths, uses Skia safe API, proper temp file cleanup.

Test Coverage

Missing automated tests. Suggest adding unit tests for font utilities and integration tests for UI.

---

Recommendations

Before merge:

1. Fix non-nullable map access (issue 🔍 Implement text search/find functionality (Ctrl+F) #2)
2. Update documentation comment (issue 🌏 Implement IME (Input Method Editor) support for CJK languages #5)

Future improvements:

1. Add async font loading
2. Enhance monospace detection
3. Filter empty sections in dropdown
4. Add unit tests

---

Verdict

APPROVED with minor suggestions. Solid implementation following BossTerm patterns. Issues are non-critical and can be addressed in follow-up PRs.

Great work on the iTerm2-style font categorization!

---

Generated with Claude Code