From 12e91aa4fe0cdd87bb9dd402c411e4448fe418d0 Mon Sep 17 00:00:00 2001 From: atani Date: Fri, 6 Mar 2026 23:16:15 +0900 Subject: [PATCH] fix: address review feedback for CJK font fallback - Split Unicode ranges by language to avoid mapping Hangul to Hiragino Sans or Kana to Apple SD Gothic Neo. Shared CJK ranges (ideographs, symbols, fullwidth forms) use the first CJK language's font, while script-specific ranges (Kana, Hangul) only map to their own font. - Use UUID-based temp file path to prevent race conditions on concurrent launches. - Move fallback injection after ghostty_config_load_recursive_files so that config-file includes are already loaded when checking for existing font-codepoint-map entries. - Follow config-file directives when scanning for existing font-codepoint-map entries. - Extract test helper withTempConfig to reduce duplication. - Add tests for multi-language mappings and config-file includes. - Replace placeholder issue URL with actual PR link. --- Sources/GhosttyTerminalView.swift | 121 +++++++++++++++------- cmuxTests/GhosttyConfigTests.swift | 158 ++++++++++++++++------------- 2 files changed, 173 insertions(+), 106 deletions(-) diff --git a/Sources/GhosttyTerminalView.swift b/Sources/GhosttyTerminalView.swift index 945b0e40..b1ceee1f 100644 --- a/Sources/GhosttyTerminalView.swift +++ b/Sources/GhosttyTerminalView.swift @@ -999,8 +999,8 @@ class GhosttyApp { ghostty_config_load_default_files(config) loadReleaseAppSupportGhosttyConfigIfNeeded(config) loadLegacyGhosttyConfigIfNeeded(config) - loadCJKFontFallbackIfNeeded(config) ghostty_config_load_recursive_files(config) + loadCJKFontFallbackIfNeeded(config) ghostty_config_finalize(config) } @@ -1009,23 +1009,24 @@ class GhosttyApp { /// a decorative calligraphic font) for CJK characters. This injects a /// sensible default based on the system's preferred languages. /// - /// See: https://github.com/manaflow-ai/cmux/issues/XXX + /// See: https://github.com/manaflow-ai/cmux/pull/1017 private func loadCJKFontFallbackIfNeeded(_ config: ghostty_config_t) { if Self.userConfigContainsCJKCodepointMap() { return } - guard let fontFamily = Self.preferredCJKFontFamily() else { return } + guard let mappings = Self.cjkFontMappings() else { return } - let lines = Self.cjkUnicodeRanges.map { range in - "font-codepoint-map = \(range)=\(fontFamily)" + let lines = mappings.map { range, font in + "font-codepoint-map = \(range)=\(font)" }.joined(separator: "\n") - let tmpPath = NSTemporaryDirectory() + "cmux-cjk-font-fallback.conf" + let tmpURL = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-cjk-font-fallback-\(UUID().uuidString).conf") do { - try lines.write(toFile: tmpPath, atomically: true, encoding: .utf8) - tmpPath.withCString { path in + try lines.write(to: tmpURL, atomically: true, encoding: .utf8) + defer { try? FileManager.default.removeItem(at: tmpURL) } + tmpURL.path.withCString { path in ghostty_config_load_file(config, path) } - try? FileManager.default.removeItem(atPath: tmpPath) } catch { #if DEBUG Self.initLog("failed to write CJK font fallback config: \(error)") @@ -1033,40 +1034,69 @@ class GhosttyApp { } } - /// Unicode ranges that cover CJK characters, kana, and fullwidth forms. - private static let cjkUnicodeRanges = [ + /// Unicode ranges shared by all CJK languages (Han ideographs, symbols, fullwidth forms). + private static let sharedCJKRanges = [ "U+3000-U+303F", // CJK Symbols and Punctuation - "U+3040-U+309F", // Hiragana - "U+30A0-U+30FF", // Katakana "U+4E00-U+9FFF", // CJK Unified Ideographs "U+F900-U+FAFF", // CJK Compatibility Ideographs "U+FF00-U+FFEF", // Halfwidth and Fullwidth Forms - "U+AC00-U+D7AF", // Hangul Syllables - "U+1100-U+11FF", // Hangul Jamo "U+3400-U+4DBF", // CJK Unified Ideographs Extension A ] - /// Returns a suitable CJK font family name based on the user's preferred - /// system language, or nil if no CJK language is detected. - static func preferredCJKFontFamily( + /// Unicode ranges specific to Japanese (kana). + private static let japaneseRanges = [ + "U+3040-U+309F", // Hiragana + "U+30A0-U+30FF", // Katakana + ] + + /// Unicode ranges specific to Korean (Hangul). + private static let koreanRanges = [ + "U+AC00-U+D7AF", // Hangul Syllables + "U+1100-U+11FF", // Hangul Jamo + ] + + /// Returns (range, font) pairs for CJK font fallback based on the system's + /// preferred languages, or nil if no CJK language is detected. Each language + /// only maps its own script ranges to avoid assigning glyphs to a font that + /// lacks coverage (e.g. Hangul to Hiragino Sans). + static func cjkFontMappings( preferredLanguages: [String] = Locale.preferredLanguages - ) -> String? { + ) -> [(String, String)]? { + var mappings: [(String, String)] = [] + var coveredShared = false + for lang in preferredLanguages { let lower = lang.lowercased() + let font: String + var langRanges: [String] = [] + if lower.hasPrefix("ja") { - return "Hiragino Sans" + font = "Hiragino Sans" + langRanges = japaneseRanges + } else if lower.hasPrefix("ko") { + font = "Apple SD Gothic Neo" + langRanges = koreanRanges + } else if lower.hasPrefix("zh-hant") || lower.hasPrefix("zh-tw") || lower.hasPrefix("zh-hk") { + font = "PingFang TC" + } else if lower.hasPrefix("zh") { + font = "PingFang SC" + } else { + continue } - if lower.hasPrefix("ko") { - return "Apple SD Gothic Neo" + + if !coveredShared { + for range in sharedCJKRanges { + mappings.append((range, font)) + } + coveredShared = true } - if lower.hasPrefix("zh-hant") || lower.hasPrefix("zh-tw") || lower.hasPrefix("zh-hk") { - return "PingFang TC" - } - if lower.hasPrefix("zh") { - return "PingFang SC" + + for range in langRanges { + mappings.append((range, font)) } } - return nil + + return mappings.isEmpty ? nil : mappings } /// Checks whether the user's Ghostty config files already contain @@ -1081,12 +1111,35 @@ class GhosttyApp { ) -> Bool { for rawPath in configPaths { let path = NSString(string: rawPath).expandingTildeInPath - guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { continue } - for line in contents.components(separatedBy: .newlines) { - let trimmed = line.trimmingCharacters(in: .whitespaces) - if trimmed.hasPrefix("#") { continue } - if trimmed.hasPrefix("font-codepoint-map") { - return true + if Self.configFileContainsCodepointMap(atPath: path) { + return true + } + } + return false + } + + /// Scans a single config file (and any files it includes) for + /// `font-codepoint-map` entries. + private static func configFileContainsCodepointMap(atPath path: String) -> Bool { + guard let contents = try? String(contentsOfFile: path, encoding: .utf8) else { + return false + } + for line in contents.components(separatedBy: .newlines) { + let trimmed = line.trimmingCharacters(in: .whitespaces) + if trimmed.hasPrefix("#") { continue } + if trimmed.hasPrefix("font-codepoint-map") { + return true + } + if trimmed.hasPrefix("config-file") { + let parts = trimmed.split(separator: "=", maxSplits: 1) + if parts.count == 2 { + let includePath = parts[1] + .trimmingCharacters(in: .whitespaces) + .trimmingCharacters(in: CharacterSet(charactersIn: "\"")) + let resolved = NSString(string: includePath).expandingTildeInPath + if configFileContainsCodepointMap(atPath: resolved) { + return true + } } } } diff --git a/cmuxTests/GhosttyConfigTests.swift b/cmuxTests/GhosttyConfigTests.swift index dc8f1196..6d45da72 100644 --- a/cmuxTests/GhosttyConfigTests.swift +++ b/cmuxTests/GhosttyConfigTests.swift @@ -1342,95 +1342,92 @@ final class GhosttyMouseFocusTests: XCTestCase { // MARK: - CJK Font Fallback - func testPreferredCJKFontFamilyReturnsHiraginoForJapanese() { - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["ja-JP", "en-US"]), - "Hiragino Sans" - ) - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["ja"]), - "Hiragino Sans" - ) + private func withTempConfig( + _ contents: String, + body: (String) -> Void + ) throws { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-test-cjk-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let file = dir.appendingPathComponent("config") + try contents.write(to: file, atomically: true, encoding: .utf8) + body(file.path) } - func testPreferredCJKFontFamilyReturnsAppleSDGothicNeoForKorean() { - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["ko-KR", "en-US"]), - "Apple SD Gothic Neo" - ) + // MARK: cjkFontMappings + + func testCJKFontMappingsReturnsHiraginoWithKanaForJapanese() { + let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "en-US"])! + let fonts = Set(mappings.map(\.1)) + let ranges = mappings.map(\.0) + + XCTAssertTrue(fonts.contains("Hiragino Sans")) + XCTAssertTrue(ranges.contains("U+3040-U+309F"), "Should include Hiragana") + XCTAssertTrue(ranges.contains("U+30A0-U+30FF"), "Should include Katakana") + XCTAssertTrue(ranges.contains("U+4E00-U+9FFF"), "Should include CJK Ideographs") + XCTAssertFalse(ranges.contains("U+AC00-U+D7AF"), "Should NOT include Hangul") } - func testPreferredCJKFontFamilyReturnsPingFangForChinese() { - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["zh-Hant-TW"]), - "PingFang TC" - ) - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["zh-Hans-CN"]), - "PingFang SC" - ) - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["zh-HK"]), - "PingFang TC" - ) + func testCJKFontMappingsReturnsAppleSDGothicNeoWithHangulForKorean() { + let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ko-KR"])! + let fonts = Set(mappings.map(\.1)) + let ranges = mappings.map(\.0) + + XCTAssertTrue(fonts.contains("Apple SD Gothic Neo")) + XCTAssertTrue(ranges.contains("U+AC00-U+D7AF"), "Should include Hangul Syllables") + XCTAssertTrue(ranges.contains("U+1100-U+11FF"), "Should include Hangul Jamo") + XCTAssertTrue(ranges.contains("U+4E00-U+9FFF"), "Should include CJK Ideographs") + XCTAssertFalse(ranges.contains("U+3040-U+309F"), "Should NOT include Hiragana") } - func testPreferredCJKFontFamilyReturnsNilForNonCJKLanguages() { - XCTAssertNil( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["en-US", "fr-FR"]) - ) - XCTAssertNil( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: []) - ) + func testCJKFontMappingsReturnsPingFangForChinese() { + let mappingsTW = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hant-TW"])! + XCTAssertTrue(mappingsTW.contains { $0.1 == "PingFang TC" }) + + let mappingsCN = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-Hans-CN"])! + XCTAssertTrue(mappingsCN.contains { $0.1 == "PingFang SC" }) + + let mappingsHK = GhosttyApp.cjkFontMappings(preferredLanguages: ["zh-HK"])! + XCTAssertTrue(mappingsHK.contains { $0.1 == "PingFang TC" }) } - func testPreferredCJKFontFamilyUsesFirstCJKLanguageInList() { - XCTAssertEqual( - GhosttyApp.preferredCJKFontFamily(preferredLanguages: ["en-US", "ko-KR", "ja-JP"]), - "Apple SD Gothic Neo" - ) + func testCJKFontMappingsReturnsNilForNonCJKLanguages() { + XCTAssertNil(GhosttyApp.cjkFontMappings(preferredLanguages: ["en-US", "fr-FR"])) + XCTAssertNil(GhosttyApp.cjkFontMappings(preferredLanguages: [])) } + func testCJKFontMappingsMultiLanguageMapsScriptSpecificRanges() { + let mappings = GhosttyApp.cjkFontMappings(preferredLanguages: ["ja-JP", "ko-KR"])! + + let hiraginoRanges = mappings.filter { $0.1 == "Hiragino Sans" }.map(\.0) + let sdGothicRanges = mappings.filter { $0.1 == "Apple SD Gothic Neo" }.map(\.0) + + XCTAssertTrue(hiraginoRanges.contains("U+3040-U+309F"), "Hiragana → Hiragino") + XCTAssertTrue(hiraginoRanges.contains("U+4E00-U+9FFF"), "Shared CJK → first lang font") + XCTAssertTrue(sdGothicRanges.contains("U+AC00-U+D7AF"), "Hangul → Apple SD Gothic Neo") + XCTAssertFalse(hiraginoRanges.contains("U+AC00-U+D7AF"), "Hangul NOT in Hiragino") + } + + // MARK: userConfigContainsCJKCodepointMap + func testUserConfigContainsCJKCodepointMapDetectsPresence() throws { - let tmpDir = NSTemporaryDirectory() + "cmux-test-cjk-\(UUID().uuidString)/" - try FileManager.default.createDirectory(atPath: tmpDir, withIntermediateDirectories: true) - defer { try? FileManager.default.removeItem(atPath: tmpDir) } - - let configWithMap = tmpDir + "config-with-map" - try "font-family = Menlo\nfont-codepoint-map = U+3000-U+9FFF=Hiragino Sans\n" - .write(toFile: configWithMap, atomically: true, encoding: .utf8) - - XCTAssertTrue( - GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [configWithMap]) - ) + try withTempConfig("font-family = Menlo\nfont-codepoint-map = U+3000-U+9FFF=Hiragino Sans\n") { path in + XCTAssertTrue(GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [path])) + } } func testUserConfigContainsCJKCodepointMapReturnsFalseWhenAbsent() throws { - let tmpDir = NSTemporaryDirectory() + "cmux-test-cjk-\(UUID().uuidString)/" - try FileManager.default.createDirectory(atPath: tmpDir, withIntermediateDirectories: true) - defer { try? FileManager.default.removeItem(atPath: tmpDir) } - - let configWithoutMap = tmpDir + "config-no-map" - try "font-family = Menlo\nfont-size = 14\n" - .write(toFile: configWithoutMap, atomically: true, encoding: .utf8) - - XCTAssertFalse( - GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [configWithoutMap]) - ) + try withTempConfig("font-family = Menlo\nfont-size = 14\n") { path in + XCTAssertFalse(GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [path])) + } } func testUserConfigContainsCJKCodepointMapIgnoresComments() throws { - let tmpDir = NSTemporaryDirectory() + "cmux-test-cjk-\(UUID().uuidString)/" - try FileManager.default.createDirectory(atPath: tmpDir, withIntermediateDirectories: true) - defer { try? FileManager.default.removeItem(atPath: tmpDir) } - - let configCommented = tmpDir + "config-commented" - try "# font-codepoint-map = U+3000-U+9FFF=Hiragino Sans\n" - .write(toFile: configCommented, atomically: true, encoding: .utf8) - - XCTAssertFalse( - GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [configCommented]) - ) + try withTempConfig("# font-codepoint-map = U+3000-U+9FFF=Hiragino Sans\n") { path in + XCTAssertFalse(GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [path])) + } } func testUserConfigContainsCJKCodepointMapReturnsFalseForMissingFiles() { @@ -1438,4 +1435,21 @@ final class GhosttyMouseFocusTests: XCTestCase { GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: ["/nonexistent/path/config"]) ) } + + func testUserConfigContainsCJKCodepointMapFollowsConfigFileIncludes() throws { + let dir = FileManager.default.temporaryDirectory + .appendingPathComponent("cmux-test-cjk-include-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: dir) } + + let included = dir.appendingPathComponent("fonts.conf") + try "font-codepoint-map = U+3000-U+9FFF=Hiragino Sans\n" + .write(to: included, atomically: true, encoding: .utf8) + + let main = dir.appendingPathComponent("config") + try "font-family = Menlo\nconfig-file = \(included.path)\n" + .write(to: main, atomically: true, encoding: .utf8) + + XCTAssertTrue(GhosttyApp.userConfigContainsCJKCodepointMap(configPaths: [main.path])) + } }