From ae2f29ddeff8b5d3fd8f97b8b18b10dfe8ce52ed Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sun, 4 Oct 2026 20:45:37 +0300 Subject: [PATCH] fix: reopen the last text, widen the column, strengthen the highlight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects the side-by-side against the prototype turned up. **The library recorded which text was open and never read it back.** Every launch asked the reader to choose a text with one text in the list. `lastOpenedAt` has been written since M2 and nothing consumed it. Only a text that has actually been opened counts: falling back to the newest import would open a document the reader has never seen, which is a guess rather than a restore. Restored once per launch, not on every reload, because a reload follows every import and delete and would fight whatever the reader had just chosen. **The reading column was capped in points, which is the wrong unit.** 720 points gave 32 characters a line at 22pt and **15 characters at 48pt** — a column two words wide for exactly the reader who enlarged the text in order to read it more easily. The cap is now 40 characters, the top of the comfortable range for CJK, converted to points through the font size, which works because a Han glyph is one em wide. Measured on the running app: at 22pt the column goes from 713 to 876 points, and at 48pt from 713 to 1067, where it is now limited by the window rather than by the constant. **The revealed-word fill was too faint to read as a marker.** At 0.08 it composites to a 1.11:1 contrast ratio against white and 1.08:1 against the macOS dark background. Revealed is now 0.14 and selected 0.30, giving 1.20:1 and 1.49:1. I had reported that one as a dark-mode defect, and the measurement says otherwise: the two backgrounds differ by about 0.03, so one pair of values serves both and a `colorScheme` parameter would be ceremony around a rounding error. Revealed stays deliberately quiet — the reading printed above the word is the primary signal, and a page with two hundred revealed words should not become a wall of colour — while selection, which also carries a border, is the state that has to be unmistakable. Co-Authored-By: Claude Opus 5 --- Sources/HanReaderUI/Design/DesignTokens.swift | 3 -- Sources/HanReaderUI/Design/ReaderStyle.swift | 22 ++++++++++ .../HanReaderUI/Design/TokenEmphasis.swift | 33 +++++++++++---- Sources/HanReaderUI/HanReaderRootView.swift | 17 +++++++- Sources/HanReaderUI/Models/LibraryModel.swift | 12 ++++++ .../HanReaderUI/Reader/ReaderSurface.swift | 2 +- .../HanReaderUITests/LibraryModelTests.swift | 42 +++++++++++++++++++ Tests/HanReaderUITests/ReaderStyleTests.swift | 21 ++++++++++ .../TokenHighlightTests.swift | 20 +++++++++ 9 files changed, 160 insertions(+), 12 deletions(-) diff --git a/Sources/HanReaderUI/Design/DesignTokens.swift b/Sources/HanReaderUI/Design/DesignTokens.swift index 583b4e0..5a8d4c7 100644 --- a/Sources/HanReaderUI/Design/DesignTokens.swift +++ b/Sources/HanReaderUI/Design/DesignTokens.swift @@ -86,9 +86,6 @@ nonisolated enum ReaderMetrics { static let detailHeadwordWidth = 110.0 /// Width of the library sidebar on a regular-width layout. static let sidebarWidth = 260.0 - /// The reading column stops widening here. Beyond roughly this width a - /// line holds too many characters to track back to the start of the next. - static let readingColumnMaxWidth = 720.0 /// Padding around the reading column. static let readingColumnPadding = 24.0 /// Minimum hit target. Apple's guideline is 44pt, and a tapped word is diff --git a/Sources/HanReaderUI/Design/ReaderStyle.swift b/Sources/HanReaderUI/Design/ReaderStyle.swift index b7a5b9d..55934ac 100644 --- a/Sources/HanReaderUI/Design/ReaderStyle.swift +++ b/Sources/HanReaderUI/Design/ReaderStyle.swift @@ -83,6 +83,19 @@ nonisolated struct ReaderStyle: Hashable, Sendable { /// targets below the 44pt minimum. static let compactFontSizeFloor = 18.0 + /// How many characters a line may hold before the column stops widening. + /// + /// Measured in **characters**, not points, because that is what line + /// length means for reading: 30 to 40 is the comfortable range for CJK, + /// and 40 is the top of it, so the column uses as much of a wide window + /// as it can without the eye losing the start of the next line. + /// + /// A fixed point value cannot express this. The 720pt cap it replaces + /// gave 32 characters at 22pt — reasonable — and **15 characters at + /// 48pt**, which is a column two words wide for exactly the reader who + /// enlarged the text in order to read it more easily. + static let maximumCharactersPerLine = 40.0 + // MARK: - Resolved values let spacing: WordSpacing @@ -104,6 +117,15 @@ nonisolated struct ReaderStyle: Hashable, Sendable { let paragraphGap: Double let blankLineHeight: Double + /// How wide the reading column may grow. + /// + /// A Han glyph is one em wide — the square em is the point of the + /// script — so a character count converts to points by multiplying by + /// the font size. + var maximumColumnWidth: Double { + fontSize * Self.maximumCharactersPerLine + } + /// Whether a token's width grows to fit its reading. /// /// Only in `.separated` mode. Reserving the width in `.continuous` mode diff --git a/Sources/HanReaderUI/Design/TokenEmphasis.swift b/Sources/HanReaderUI/Design/TokenEmphasis.swift index b4a1be5..53b3c17 100644 --- a/Sources/HanReaderUI/Design/TokenEmphasis.swift +++ b/Sources/HanReaderUI/Design/TokenEmphasis.swift @@ -48,13 +48,32 @@ nonisolated struct TokenHighlight: Hashable, Sendable { /// word identically, which is precisely why tapping a second instance of /// an already-revealed word felt like a bug — nothing on screen changed. /// - **A dotted underline** carries reveal state when - /// `differentiateWithoutColor` is on. An 8%-opacity accent fill is the - /// *only* signal the prototype had, and it is invisible to a reader who + /// `differentiateWithoutColor` is on. A tinted fill was the *only* + /// signal the prototype had, and a tint is invisible to a reader who /// has asked the system not to rely on colour. /// - /// Increased contrast roughly doubles both fills. A 20% tint over a page - /// background is below the 3:1 non-text contrast ratio for a reader who - /// has asked for more. + /// ### The numbers, and why they are not theme-dependent + /// + /// Measured as WCAG contrast ratios between the composited fill and the + /// background, for the system accent over white and over the macOS dark + /// window background: + /// + /// | fill | on light | on dark | + /// |---|---|---| + /// | 0.08 — what shipped | 1.11 | 1.08 | + /// | 0.14 — revealed | 1.20 | 1.16 | + /// | 0.30 — selected | 1.49 | 1.43 | + /// + /// The shipped 0.08 was too faint to read as a marker at all, which is + /// the defect. It is *not* a dark-mode defect: the two columns differ by + /// about 0.03, so one pair of values serves both themes and a + /// `colorScheme` parameter would be ceremony around a rounding error. + /// + /// Revealed stays deliberately quiet. The reading printed above the word + /// is the primary signal that it is revealed; the fill only has to make + /// the set scannable, and a page where a reader has revealed two hundred + /// words should not turn into a wall of colour. Selection is the state + /// that must be unmistakable, and it also carries a border. static func resolve( _ emphasis: TokenEmphasis, increasedContrast: Bool = false, @@ -64,8 +83,8 @@ nonisolated struct TokenHighlight: Hashable, Sendable { { let fill: Double = switch emphasis { case .plain: 0 - case .revealed: increasedContrast ? 0.18 : 0.08 - case .selected: increasedContrast ? 0.32 : 0.20 + case .revealed: increasedContrast ? 0.24 : 0.14 + case .selected: increasedContrast ? 0.44 : 0.30 } return Self( fillOpacity: fill, diff --git a/Sources/HanReaderUI/HanReaderRootView.swift b/Sources/HanReaderUI/HanReaderRootView.swift index db91bfc..54c24f2 100644 --- a/Sources/HanReaderUI/HanReaderRootView.swift +++ b/Sources/HanReaderUI/HanReaderRootView.swift @@ -95,6 +95,10 @@ private struct LibraryAndReader: View { @State private var importer: TextImporter @State private var isFileImporterPresented = false @State private var duplicateNotice = false + /// Restoring happens once per launch, not on every reload. A reload + /// follows every import and delete, and re-selecting there would fight + /// whatever the reader had just chosen. + @State private var hasRestoredLastText = false /// Whichever failure is outstanding. /// @@ -153,7 +157,18 @@ private struct LibraryAndReader: View { ) } } - .task { await library.load() } + .task { + await library.load() + // The library already recorded which text was open; it just was + // not read back, so every launch asked the reader to choose a + // text again with one text in the list. + if !hasRestoredLastText { + hasRestoredLastText = true + if selection == nil { + selection = library.mostRecentlyOpened?.id + } + } + } // `.fileImporter` is identical on both platforms and handles the // security-scoped URL itself, which is why there is no `NSOpenPanel` // anywhere in this project and no platform branch here. diff --git a/Sources/HanReaderUI/Models/LibraryModel.swift b/Sources/HanReaderUI/Models/LibraryModel.swift index 9df5fc5..b662b00 100644 --- a/Sources/HanReaderUI/Models/LibraryModel.swift +++ b/Sources/HanReaderUI/Models/LibraryModel.swift @@ -57,6 +57,18 @@ final class LibraryModel { self.services = services } + /// The text the reader was last in, if there is one. + /// + /// Only a text that has actually been opened counts. Falling back to the + /// most recently *imported* one would mean a fresh library opens a + /// document the reader has never seen, which is a guess rather than a + /// restore. + var mostRecentlyOpened: LibraryListItem? { + items + .filter { $0.lastOpenedAt != nil } + .max { ($0.lastOpenedAt ?? .distantPast) < ($1.lastOpenedAt ?? .distantPast) } + } + func load() async { loadGeneration += 1 let generation = loadGeneration diff --git a/Sources/HanReaderUI/Reader/ReaderSurface.swift b/Sources/HanReaderUI/Reader/ReaderSurface.swift index e73e6f1..875f815 100644 --- a/Sources/HanReaderUI/Reader/ReaderSurface.swift +++ b/Sources/HanReaderUI/Reader/ReaderSurface.swift @@ -46,7 +46,7 @@ struct ReaderSurface: View { } } .scrollTargetLayout() - .frame(maxWidth: ReaderMetrics.readingColumnMaxWidth, alignment: .leading) + .frame(maxWidth: style.maximumColumnWidth, alignment: .leading) .padding(.horizontal, ReaderMetrics.readingColumnPadding) // Room above the first line for a revealed reading on it. // diff --git a/Tests/HanReaderUITests/LibraryModelTests.swift b/Tests/HanReaderUITests/LibraryModelTests.swift index 11cd326..392cd98 100644 --- a/Tests/HanReaderUITests/LibraryModelTests.swift +++ b/Tests/HanReaderUITests/LibraryModelTests.swift @@ -96,6 +96,48 @@ struct LibraryModelTests { #expect(item.characterCount == 4000) } + // MARK: - Reopening + + /// The library recorded which text was open and nothing read it back, so + /// every launch asked the reader to choose a text with one text in the + /// list. + @Test("The text last read is the one to reopen") + func restoresTheLastRead() async throws { + let services = try AppServices.inMemory() + let model = LibraryModel(services: services) + guard case let .imported(first) = await model.importText(title: "A", content: "一"), + case let .imported(second) = await model.importText(title: "B", content: "二") + else { + Issue.record("import failed") + return + } + + // Opened out of import order, so this cannot pass by accident: the + // newer import is the older read. + try await services.library.markOpened(second, at: .now.addingTimeInterval(-60)) + try await services.library.markOpened(first) + await model.load() + + #expect(model.mostRecentlyOpened?.id == first) + } + + /// A fresh library opens nothing. Falling back to the newest *import* + /// would open a document the reader has never seen, which is a guess + /// rather than a restore. + @Test("A library nothing has been read from reopens nothing") + func nothingToRestore() async throws { + let model = try makeModel() + _ = await model.importText(title: "A", content: "一") + #expect(model.mostRecentlyOpened == nil) + } + + @Test("An empty library reopens nothing") + func emptyRestoresNothing() async throws { + let model = try makeModel() + await model.load() + #expect(model.mostRecentlyOpened == nil) + } + // MARK: - Titles @Test("A title comes from the file name", arguments: [ diff --git a/Tests/HanReaderUITests/ReaderStyleTests.swift b/Tests/HanReaderUITests/ReaderStyleTests.swift index 8449a8c..b2c27a8 100644 --- a/Tests/HanReaderUITests/ReaderStyleTests.swift +++ b/Tests/HanReaderUITests/ReaderStyleTests.swift @@ -98,6 +98,27 @@ struct ReaderStyleTests { #expect(!ReaderStyle(fontSize: 22, spacing: .continuous).reservesRubyWidth) } + /// The regression a fixed point cap caused, as a test. 720 points gave + /// 32 characters a line at 22pt and **15 at 48pt** — a column two words + /// wide for exactly the reader who enlarged the text to read it. + @Test("The column holds the same number of characters at every size") + func columnMeasuredInCharacters() { + for size in [14.0, 22.0, 36.0, 48.0] { + let style = ReaderStyle(fontSize: size) + let characters = style.maximumColumnWidth / style.fontSize + #expect(abs(characters - ReaderStyle.maximumCharactersPerLine) < 1e-9) + } + } + + @Test("A larger reading size gets a wider column, not a narrower one") + func columnGrowsWithText() { + let small = ReaderStyle(fontSize: 14).maximumColumnWidth + let large = ReaderStyle(fontSize: 48).maximumColumnWidth + #expect(large > small) + // And the cap stays inside the range that is comfortable for CJK. + #expect((30.0 ... 40.0).contains(ReaderStyle.maximumCharactersPerLine)) + } + @Test("The ruby band is tall enough for the glyphs it holds") func rubyReservationExceedsRubyFontSize() { // Pinyin carries diacritics above and descenders below, so a diff --git a/Tests/HanReaderUITests/TokenHighlightTests.swift b/Tests/HanReaderUITests/TokenHighlightTests.swift index f0b807f..a87963b 100644 --- a/Tests/HanReaderUITests/TokenHighlightTests.swift +++ b/Tests/HanReaderUITests/TokenHighlightTests.swift @@ -39,6 +39,26 @@ struct TokenHighlightTests { #expect(increased.fillOpacity > normal.fillOpacity) } + /// The fill shipped at 0.08, which composites to a 1.11:1 contrast ratio + /// against white and 1.08:1 against the macOS dark background — too + /// faint to read as a marker. The floor here stops it drifting back + /// without anyone noticing, since nothing else in the suite would fail. + @Test("A revealed word is actually visible") + func revealedIsVisible() { + #expect(TokenHighlight.resolve(.revealed).fillOpacity >= 0.12) + #expect(TokenHighlight.resolve(.selected).fillOpacity >= 0.25) + } + + /// Quiet, though. The reading printed above the word is the primary + /// signal; a page with two hundred revealed words should not become a + /// wall of colour. + @Test("A revealed word stays quieter than the selected one") + func revealedStaysQuiet() { + let revealed = TokenHighlight.resolve(.revealed) + #expect(revealed.fillOpacity <= 0.20) + #expect(TokenHighlight.resolve(.selected).fillOpacity > revealed.fillOpacity * 1.5) + } + /// Reveal state must survive a reader who has asked the system not to /// rely on colour. An 8%-opacity accent fill was the prototype's only /// signal, and it is invisible to exactly that reader.