From b97993f8666e8dc34d9bf6aba450f13b05ce6c70 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Mon, 17 Aug 2026 21:01:06 +0200 Subject: [PATCH] fix(app): stabilize retained canvas diff rendering --- .../app/src/git/diff-document/model.test.ts | 7 ++ packages/app/src/git/diff-document/model.ts | 12 +++- .../git/diff-document/native-slabs.test.ts | 21 ++++++ .../app/src/git/diff-document/native-slabs.ts | 5 +- .../app/src/git/diff-document/paint.native.ts | 25 ++++--- .../src/git/diff-document/paint.web.test.ts | 25 +++++-- .../app/src/git/diff-document/paint.web.ts | 22 +++--- .../src/git/diff-document/surface.native.tsx | 69 ++++++++++--------- .../app/src/git/diff-document/surface.web.tsx | 29 +++++--- .../diff-document/text-measurement.test.ts | 34 +++++++++ .../src/git/diff-document/text-measurement.ts | 35 ++++++++++ .../app/src/git/diff-document/text.native.ts | 37 ++++++---- .../src/git/diff-document/viewport.test.ts | 31 +++++++++ .../app/src/git/diff-document/viewport.ts | 10 +++ 14 files changed, 274 insertions(+), 88 deletions(-) create mode 100644 packages/app/src/git/diff-document/viewport.test.ts create mode 100644 packages/app/src/git/diff-document/viewport.ts diff --git a/packages/app/src/git/diff-document/model.test.ts b/packages/app/src/git/diff-document/model.test.ts index 20f1cdc81..ab0375406 100644 --- a/packages/app/src/git/diff-document/model.test.ts +++ b/packages/app/src/git/diff-document/model.test.ts @@ -8,6 +8,7 @@ import { measureFragments, resolveScrollAnchor, resolveRelayoutScrollTop, + shouldApplyRelayoutScroll, } from "./model"; import type { BuildDiffDocumentModelInput, TextMeasurer } from "./types"; @@ -65,6 +66,12 @@ function input(overrides: Partial = {}): BuildDiffD } describe("diff document model", () => { + it("does not imperatively scroll for a no-op relayout", () => { + expect(shouldApplyRelayoutScroll(320, 320)).toBe(false); + expect(shouldApplyRelayoutScroll(320, 320.4)).toBe(false); + expect(shouldApplyRelayoutScroll(320, 321)).toBe(true); + }); + it("segments graphemes when Intl.Segmenter is unavailable", () => { const originalSegmenter = Intl.Segmenter; Object.defineProperty(Intl, "Segmenter", { configurable: true, value: undefined }); diff --git a/packages/app/src/git/diff-document/model.ts b/packages/app/src/git/diff-document/model.ts index 75cee369e..c932e7d3b 100644 --- a/packages/app/src/git/diff-document/model.ts +++ b/packages/app/src/git/diff-document/model.ts @@ -172,10 +172,9 @@ export function buildDiffDocumentModel(input: BuildDiffDocumentModelInput): Diff }; } -export function finalExpandedBodyBorderTop(model: DiffDocumentModel): number | null { +export function expandedBodyBorderTop(file: DiffFileSection): number | null { "worklet"; - const file = model.files[model.files.length - 1]; - return file && !file.isCollapsed ? file.bottom - DIFF_BODY_BORDER_HEIGHT : null; + return file.isCollapsed ? null : file.bottom - DIFF_BODY_BORDER_HEIGHT; } export function retainReusableModels( @@ -579,6 +578,13 @@ export function resolveRelayoutScrollTop( return anchor ? resolveScrollAnchor(next, anchor) : scrollTop; } +export function shouldApplyRelayoutScroll( + currentScrollTop: number, + nextScrollTop: number, +): boolean { + return Math.abs(nextScrollTop - currentScrollTop) > 0.5; +} + function sameSourceIdentity(left: DiffSourceIdentity, right: DiffSourceIdentity): boolean { return ( left.hunkIndex === right.hunkIndex && diff --git a/packages/app/src/git/diff-document/native-slabs.test.ts b/packages/app/src/git/diff-document/native-slabs.test.ts index a2821711f..630b2dc53 100644 --- a/packages/app/src/git/diff-document/native-slabs.test.ts +++ b/packages/app/src/git/diff-document/native-slabs.test.ts @@ -78,4 +78,25 @@ describe("native canvas slabs", () => { expect(nativeCanvasWindowBucket(899, 600)).toBe(2); expect(nativeCanvasWindowTop(2, 600)).toBe(600); }); + + it("keeps slab identities stable when inline review geometry moves files", () => { + const before = buildNativeCanvasSlabs( + modelWithBodies([ + { path: "first.ts", bodyTop: 30, bottom: 230 }, + { path: "second.ts", bodyTop: 260, bottom: 3460 }, + ]), + 600, + ); + const after = buildNativeCanvasSlabs( + modelWithBodies([ + { path: "first.ts", bodyTop: 30, bottom: 378 }, + { path: "second.ts", bodyTop: 408, bottom: 3608 }, + ]), + 600, + ); + + expect(after.filter((slab) => slab.path === "second.ts").map((slab) => slab.key)).toEqual( + before.filter((slab) => slab.path === "second.ts").map((slab) => slab.key), + ); + }); }); diff --git a/packages/app/src/git/diff-document/native-slabs.ts b/packages/app/src/git/diff-document/native-slabs.ts index bcbd2d0fb..2b90623bf 100644 --- a/packages/app/src/git/diff-document/native-slabs.ts +++ b/packages/app/src/git/diff-document/native-slabs.ts @@ -19,10 +19,11 @@ export function buildNativeCanvasSlabs( const slabHeight = Math.max(768, viewportHeight * 2); return model.files.flatMap((file) => { const slabs: NativeCanvasSlab[] = []; - for (let top = file.bodyTop; top < file.bottom; top += slabHeight) { + let slabIndex = 0; + for (let top = file.bodyTop; top < file.bottom; top += slabHeight, slabIndex += 1) { const height = Math.min(slabHeight, file.bottom - top); slabs.push({ - key: `${file.path}:${top}:${height}`, + key: `${file.path}:${slabIndex}`, path: file.path, fileIndex: file.fileIndex, top, diff --git a/packages/app/src/git/diff-document/paint.native.ts b/packages/app/src/git/diff-document/paint.native.ts index d80902925..afcfea7d4 100644 --- a/packages/app/src/git/diff-document/paint.native.ts +++ b/packages/app/src/git/diff-document/paint.native.ts @@ -5,7 +5,7 @@ import { type SkPaint, type SkPicture, } from "@shopify/react-native-skia"; -import { DIFF_BODY_BORDER_HEIGHT, finalExpandedBodyBorderTop, visibleRowRange } from "./model"; +import { DIFF_BODY_BORDER_HEIGHT, expandedBodyBorderTop, visibleRowRange } from "./model"; import { nativeTextRuns } from "./native-text-runs"; import { horizontalOffsetForPath, type DiffHorizontalOffsets } from "./horizontal-offsets"; import { codeLineNumberTone } from "./palette"; @@ -102,6 +102,7 @@ export function recordNativeSlabPictures(input: { textLayout: input.textLayout, paints: input.paints, layer: "fixed", + fileIndex: input.fileIndex, }); const recordContent = (contentCell: "unified" | "left" | "right") => { @@ -193,6 +194,7 @@ interface PaintNativeRangeInput { paints: NativePaints; layer: "all" | "fixed" | "content"; contentCell?: "unified" | "left" | "right"; + fileIndex?: number; } function paintNativeRange(input: PaintNativeRangeInput): void { @@ -257,15 +259,20 @@ function paintNativeRange(input: PaintNativeRangeInput): void { } }); } - const finalBorderTop = paintsFixedContent ? finalExpandedBodyBorderTop(input.model) : null; - if (finalBorderTop !== null) { + let borderFiles = input.model.files; + if (input.fileIndex !== undefined) { + const file = input.model.files[input.fileIndex]; + borderFiles = file ? [file] : []; + } + for (const file of paintsFixedContent ? borderFiles : []) { + const borderTop = expandedBodyBorderTop(file); + const isVisible = + borderTop !== null && + borderTop < input.scrollTop + input.viewportHeight && + borderTop + DIFF_BODY_BORDER_HEIGHT > input.scrollTop; + if (!isVisible) continue; input.canvas.drawRect( - Skia.XYWHRect( - 0, - finalBorderTop - input.scrollTop, - input.viewportWidth, - DIFF_BODY_BORDER_HEIGHT, - ), + Skia.XYWHRect(0, borderTop - input.scrollTop, input.viewportWidth, DIFF_BODY_BORDER_HEIGHT), input.paints.border, ); } diff --git a/packages/app/src/git/diff-document/paint.web.test.ts b/packages/app/src/git/diff-document/paint.web.test.ts index 33c81833a..7a2c0571d 100644 --- a/packages/app/src/git/diff-document/paint.web.test.ts +++ b/packages/app/src/git/diff-document/paint.web.test.ts @@ -84,7 +84,7 @@ describe("web diff text shaping", () => { expect(fills).toContainEqual({ color: "border", x: 20, y: 0, width: 1, height: 58 }); }); - it("paints the final expanded file body's bottom border", () => { + it("paints every expanded file body's bottom border", () => { const fills: Array<{ color: string; x: number; y: number; width: number; height: number }> = []; let fillStyle = ""; const context = { @@ -111,8 +111,22 @@ describe("web diff text shaping", () => { } as unknown as CanvasRenderingContext2D; const borderedModel: DiffDocumentModel = { ...model, - height: 19, - files: [{ ...model.files[0]!, bodyHeight: 19, bottom: 19 }], + height: 38, + files: [ + { ...model.files[0]!, bodyHeight: 19, bottom: 19 }, + { + ...model.files[0]!, + file: { ...model.files[0]!.file, path: "src/b.ts" }, + fileIndex: 1, + path: "src/b.ts", + top: 19, + bodyTop: 19, + bodyHeight: 19, + bottom: 38, + rowStart: 1, + rowEnd: 1, + }, + ], }; paintWebViewport({ @@ -129,7 +143,10 @@ describe("web diff text shaping", () => { devicePixelRatio: 1, }); - expect(fills).toContainEqual({ color: "border", x: 0, y: 18, width: 200, height: 1 }); + expect(fills.filter((fill) => fill.color === "border" && fill.x === 0)).toEqual([ + { color: "border", x: 0, y: 18, width: 200, height: 1 }, + { color: "border", x: 0, y: 37, width: 200, height: 1 }, + ]); }); it("clips a horizontally scrolled unified selection to the code viewport", () => { diff --git a/packages/app/src/git/diff-document/paint.web.ts b/packages/app/src/git/diff-document/paint.web.ts index 776fd8aee..d89d3699f 100644 --- a/packages/app/src/git/diff-document/paint.web.ts +++ b/packages/app/src/git/diff-document/paint.web.ts @@ -1,7 +1,7 @@ import { selectionRectangles } from "./hit-testing"; import { DIFF_BODY_BORDER_HEIGHT, - finalExpandedBodyBorderTop, + expandedBodyBorderTop, fragmentWidthForRange, visibleRowRange, } from "./model"; @@ -69,15 +69,17 @@ export function paintWebViewport(input: PaintWebViewportInput): void { }); } - const finalBorderTop = finalExpandedBodyBorderTop(input.model); - if (finalBorderTop !== null) { - context.fillStyle = input.palette.border; - context.fillRect( - 0, - finalBorderTop - input.scrollTop, - input.viewportWidth, - DIFF_BODY_BORDER_HEIGHT, - ); + context.fillStyle = input.palette.border; + const paintDocumentTop = input.scrollTop + paintTop; + const paintDocumentBottom = paintDocumentTop + paintHeight; + for (const file of input.model.files) { + const borderTop = expandedBodyBorderTop(file); + const isVisible = + borderTop !== null && + borderTop < paintDocumentBottom && + borderTop + DIFF_BODY_BORDER_HEIGHT > paintDocumentTop; + if (!isVisible) continue; + context.fillRect(0, borderTop - input.scrollTop, input.viewportWidth, DIFF_BODY_BORDER_HEIGHT); } if (input.selection) paintSelection(input, input.selection); diff --git a/packages/app/src/git/diff-document/surface.native.tsx b/packages/app/src/git/diff-document/surface.native.tsx index 3bd418157..52a773389 100644 --- a/packages/app/src/git/diff-document/surface.native.tsx +++ b/packages/app/src/git/diff-document/surface.native.tsx @@ -1,4 +1,4 @@ -import { Canvas, Picture, type SkPicture } from "@shopify/react-native-skia"; +import { Canvas, Group, Picture, type SkPicture } from "@shopify/react-native-skia"; import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { @@ -9,26 +9,34 @@ import { type LayoutChangeEvent, type ViewStyle, } from "react-native"; -import { +import Animated, { createAnimatedComponent, useAnimatedReaction, useAnimatedScrollHandler, useAnimatedStyle, + useDerivedValue, useSharedValue, type SharedValue, } from "react-native-reanimated"; import { scheduleOnRN } from "react-native-worklets"; import { InlineReviewThread } from "@/review"; +import { useKeyboardShift } from "@/hooks/keyboard-shift-context"; import { inlineUnistylesStyle } from "@/styles/unistyles-inline-style"; import { DocumentFileHeader } from "./document-file-header"; import { hitTestDiffBodyPoint } from "./native-hit-testing"; +import { retainDiffViewport } from "./viewport"; import { HorizontalScroll } from "./horizontal-scroll.native"; import { horizontalOffsetForPath, retainHorizontalOffsetsForPaths, type DiffHorizontalOffsets, } from "./horizontal-offsets"; -import { buildDiffDocumentModel, resolveRelayoutScrollTop, retainReusableModels } from "./model"; +import { + buildDiffDocumentModel, + resolveRelayoutScrollTop, + retainReusableModels, + shouldApplyRelayoutScroll, +} from "./model"; import { buildNativeCanvasSlabs, nativeCanvasSlabsForViewport, @@ -54,7 +62,6 @@ import type { } from "./types"; const AnimatedScrollView = createAnimatedComponent(ScrollView); -const AnimatedCodeLayer = createAnimatedComponent(View); const SYSTEM_MONO = "monospace"; const CODE_LEFT_PADDING = 8; @@ -69,6 +76,11 @@ export function DiffSurface(props: DiffSurfaceProps) { } | null>(null); const consumedFocusRef = useRef(null); const scrollTop = useSharedValue(0); + const { shift: keyboardShift } = useKeyboardShift(); + const keyboardSpacerStyle = useAnimatedStyle( + () => ({ height: keyboardShift.value }), + [keyboardShift], + ); const horizontalOffsets = useSharedValue({}); const family = props.displayPreferences.monoFontFamily.trim() || SYSTEM_MONO; const typography = useMemo( @@ -101,7 +113,7 @@ export function DiffSurface(props: DiffSurfaceProps) { ); const reuseFrom = canReuse ? previous?.models : undefined; const next = buildDiffDocumentModel({ - files: props.files, + files: viewport.width > 0 ? props.files : [], collapsedFilePaths: props.collapsedFilePaths, layout: props.displayPreferences.layout, wrapLines: props.displayPreferences.wrapLines, @@ -156,16 +168,16 @@ export function DiffSurface(props: DiffSurfaceProps) { const layout = useCallback((event: LayoutChangeEvent) => { const { width, height } = event.nativeEvent.layout; - setViewport((current) => - current.width === width && current.height === height ? current : { width, height }, - ); + setViewport((current) => retainDiffViewport(current, { width, height })); }, []); useLayoutEffect(() => { const previous = previousModelRef.current; if (previous && previous !== model) { const nextScrollTop = resolveRelayoutScrollTop(previous, model, scrollTop.value); - scrollTop.value = nextScrollTop; - scrollRef.current?.scrollTo({ y: nextScrollTop, animated: false }); + if (shouldApplyRelayoutScroll(scrollTop.value, nextScrollTop)) { + scrollTop.value = nextScrollTop; + scrollRef.current?.scrollTo({ y: nextScrollTop, animated: false }); + } } previousModelRef.current = model; }, [model, scrollTop]); @@ -201,7 +213,6 @@ export function DiffSurface(props: DiffSurfaceProps) { () => nativeStickyHeaderIndices(model.files.length), [model.files.length], ); - return ( - {model.files.flatMap((file, index) => [ + {model.files.flatMap((file) => [ 0 && !model.files[index - 1]?.isCollapsed} selectedPath={props.selectedPath} mode={props.mode} onToggleFile={props.onToggleFile} @@ -241,6 +251,7 @@ export function DiffSurface(props: DiffSurfaceProps) { />, ])} + ); @@ -282,6 +293,7 @@ function NativeCanvasSlabLayer({ () => nativeCanvasSlabsForViewport(descriptors, windowTop, viewportHeight), [descriptors, viewportHeight, windowTop], ); + if (model.viewportWidth <= 0 || viewportHeight <= 0) return null; return visible.map((slab) => ( {model.layout === "split" ? ( @@ -503,7 +511,6 @@ function NativeCanvasSlabView({ wrapLines={model.wrapLines} clipX={file.gutterWidth + CODE_LEFT_PADDING} clipWidth={splitClipWidth} - contentWidth={file.contentWidth} height={slab.height} /> @@ -532,7 +538,6 @@ function NativeSlabCode({ wrapLines, clipX, clipWidth, - contentWidth, height, }: { picture: SkPicture; @@ -541,14 +546,13 @@ function NativeSlabCode({ wrapLines: boolean; clipX: number; clipWidth: number; - contentWidth: number; height: number; }) { - const transform = useAnimatedStyle(() => ({ - transform: [ - { translateX: wrapLines ? 0 : -horizontalOffsetForPath(horizontalOffsets.value, path) }, - ], - })); + const pictureTransform = useDerivedValue(() => [ + { + translateX: -clipX - (wrapLines ? 0 : horizontalOffsetForPath(horizontalOffsets.value, path)), + }, + ]); const clipStyle = useMemo( () => ({ position: "absolute", @@ -560,17 +564,14 @@ function NativeSlabCode({ }), [clipWidth, clipX, height], ); - const contentStyle = useMemo( - () => ({ position: "absolute", top: 0, left: -clipX, width: contentWidth, height }), - [clipX, contentWidth, height], - ); + if (clipWidth <= 0 || height <= 0) return null; return ( - - + + - - + + ); } diff --git a/packages/app/src/git/diff-document/surface.web.tsx b/packages/app/src/git/diff-document/surface.web.tsx index a0c448d3b..54e2c4a3e 100644 --- a/packages/app/src/git/diff-document/surface.web.tsx +++ b/packages/app/src/git/diff-document/surface.web.tsx @@ -16,6 +16,7 @@ import { } from "./model"; import { paintWebViewport } from "./paint.web"; import { hasPointerDragStarted } from "./pointer-gesture"; +import { retainDiffViewport } from "./viewport"; import type { DiffHit, DiffSelection, @@ -48,6 +49,7 @@ export function DiffSurface(props: DiffSurfaceProps) { startX: number; startY: number; moved: boolean; + dismissSelectionOnClick: boolean; } | null>(null); const frameRef = useRef(null); const resizeSettleTimerRef = useRef | null>(null); @@ -223,7 +225,8 @@ export function DiffSurface(props: DiffSurfaceProps) { useLayoutEffect(() => { const root = rootRef.current; if (!root) return; - const commitPendingViewport = () => setViewport(pendingViewportRef.current); + const commitPendingViewport = () => + setViewport((current) => retainDiffViewport(current, pendingViewportRef.current)); const clearResizeSettleTimer = () => { if (resizeSettleTimerRef.current === null) return; clearTimeout(resizeSettleTimerRef.current); @@ -248,12 +251,12 @@ export function DiffSurface(props: DiffSurfaceProps) { if (!entry) return; const nextViewport = { width: entry.contentRect.width, height: entry.contentRect.height }; pendingViewportRef.current = nextViewport; - setViewport((currentViewport) => - currentViewport.width === 0 || currentViewport.height === 0 - ? nextViewport - : currentViewport, - ); if (resizePointerActiveRef.current) return; + setViewport((currentViewport) => + currentViewport.width > 0 && currentViewport.height > 0 + ? currentViewport + : retainDiffViewport(currentViewport, nextViewport), + ); clearResizeSettleTimer(); resizeSettleTimerRef.current = setTimeout(() => { resizeSettleTimerRef.current = null; @@ -403,11 +406,10 @@ export function DiffSurface(props: DiffSurfaceProps) { ) ) return; - if (selectionRef.current) { + const dismissSelectionOnClick = selectionRef.current !== null; + if (dismissSelectionOnClick) { selectionRef.current = null; - dragRef.current = null; schedulePaint(); - return; } const hit = pointHit(event); if (hit?.kind !== "cell") return; @@ -416,6 +418,7 @@ export function DiffSurface(props: DiffSurfaceProps) { startX: event.clientX, startY: event.clientY, moved: false, + dismissSelectionOnClick, }; selectionRef.current = { anchor: hit.position, focus: hit.position }; event.currentTarget.focus(); @@ -486,6 +489,11 @@ export function DiffSurface(props: DiffSurfaceProps) { alreadyDragging: drag.moved, }) : false; + if (drag && !moved && drag.dismissSelectionOnClick) { + selectionRef.current = null; + schedulePaint(); + return; + } if (drag && !moved && hit?.kind === "cell" && hit.target && reviewActions) { selectionRef.current = null; reviewActions.onStartComment(hit.target); @@ -552,11 +560,10 @@ export function DiffSurface(props: DiffSurfaceProps) { >
- {model.files.map((file, index) => ( + {model.files.map((file) => ( 0 && !model.files[index - 1]?.isCollapsed} selectedPath={props.selectedPath} mode={props.mode} onToggleFile={props.onToggleFile} diff --git a/packages/app/src/git/diff-document/text-measurement.test.ts b/packages/app/src/git/diff-document/text-measurement.test.ts index bb741f915..6fe854750 100644 --- a/packages/app/src/git/diff-document/text-measurement.test.ts +++ b/packages/app/src/git/diff-document/text-measurement.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + createCachedAsciiTextMetrics, createFallbackAwareTextMeasurer, requiresNativeParagraph, type PrimaryTextFace, @@ -40,6 +41,39 @@ describe("native paragraph retention", () => { }); }); +describe("cached ASCII text metrics", () => { + it("measures each distinct grapheme once across lines", () => { + const measured: string[] = []; + const metrics = createCachedAsciiTextMetrics({ + glyphIds: (text) => Array.from(text, () => 1), + measure(text) { + measured.push(text); + return text.length * 10; + }, + }); + + expect(metrics.measureAdvances(["a", "b", "a"])).toEqual([10, 20, 30]); + expect(metrics.measureAdvances(["b", "a"])).toEqual([10, 20]); + expect(measured).toEqual(["a", "b"]); + }); + + it("caches glyph coverage per distinct ASCII character", () => { + const checked: string[] = []; + const metrics = createCachedAsciiTextMetrics({ + glyphIds(text) { + checked.push(text); + return [text === "?" ? 0 : 1]; + }, + measure: (text) => text.length * 10, + }); + + expect(metrics.hasEveryGlyph("abba")).toBe(true); + expect(metrics.hasEveryGlyph("bad?")).toBe(false); + expect(metrics.hasEveryGlyph("cab")).toBe(true); + expect(checked).toEqual(["a", "b", "d", "?", "c"]); + }); +}); + function primaryFaceWithout(...missing: string[]): PrimaryTextFace { return { glyphIds(text) { diff --git a/packages/app/src/git/diff-document/text-measurement.ts b/packages/app/src/git/diff-document/text-measurement.ts index 4f010aa1e..a72fb29c1 100644 --- a/packages/app/src/git/diff-document/text-measurement.ts +++ b/packages/app/src/git/diff-document/text-measurement.ts @@ -5,12 +5,47 @@ export interface PrimaryTextFace { measure(text: string): number; } +export interface CachedAsciiTextMetrics { + hasEveryGlyph(text: string): boolean; + measureAdvances(graphemes: readonly string[]): number[]; +} + const CODE_LIGATURE_CANDIDATE = /(?:===?|!==?|=>|<=|>=|->|::|\+\+|--)/; export function requiresNativeParagraph(text: string): boolean { return /[^\x20-\x7e\t]/u.test(text) || CODE_LIGATURE_CANDIDATE.test(text); } +export function createCachedAsciiTextMetrics(primary: PrimaryTextFace): CachedAsciiTextMetrics { + const glyphCoverage = new Map(); + const widths = new Map(); + return { + hasEveryGlyph(text) { + for (const character of text) { + let hasGlyph = glyphCoverage.get(character); + if (hasGlyph === undefined) { + hasGlyph = primary.glyphIds(character).every((glyph) => glyph !== 0); + glyphCoverage.set(character, hasGlyph); + } + if (!hasGlyph) return false; + } + return true; + }, + measureAdvances(graphemes) { + let advance = 0; + return graphemes.map((grapheme) => { + let width = widths.get(grapheme); + if (width === undefined) { + width = primary.measure(grapheme); + widths.set(grapheme, width); + } + advance += width; + return advance; + }); + }, + }; +} + export function createFallbackAwareTextMeasurer(input: { primary: PrimaryTextFace; measureWithSystemFallback: (text: string) => number; diff --git a/packages/app/src/git/diff-document/text.native.ts b/packages/app/src/git/diff-document/text.native.ts index 7307e959a..1016f6409 100644 --- a/packages/app/src/git/diff-document/text.native.ts +++ b/packages/app/src/git/diff-document/text.native.ts @@ -7,7 +7,12 @@ import { } from "@shopify/react-native-skia"; import { fragmentTextForRange } from "./model"; import { codeTextColor } from "./palette"; -import { createFallbackAwareTextMeasurer, requiresNativeParagraph } from "./text-measurement"; +import { + createCachedAsciiTextMetrics, + createFallbackAwareTextMeasurer, + requiresNativeParagraph, + type CachedAsciiTextMetrics, +} from "./text-measurement"; import type { DiffCell, DiffDocumentModel, DiffFragment, DiffPalette, TextMeasurer } from "./types"; const PARAGRAPH_WIDTH = 100_000; @@ -19,6 +24,7 @@ export interface NativeTextLayout { export interface NativeTextLayoutStore { font: SkFont; + asciiMetrics: CachedAsciiTextMetrics; paragraphsByCell: WeakMap>; ownedParagraphs: Set; families: string[]; @@ -43,6 +49,10 @@ export function createNativeTextLayoutStore(input: { const font = primaryFont(families, input.fontSize); return { font, + asciiMetrics: createCachedAsciiTextMetrics({ + glyphIds: (text) => font.getGlyphIDs(text), + measure: (text) => font.getTextWidth(text), + }), paragraphsByCell: new WeakMap(), ownedParagraphs: new Set(), families, @@ -63,7 +73,7 @@ export function prepareNativeTextLayout( const cached = store.paragraphsByCell.get(cell); if (cached) return cached; const cellParagraphs = cell.fragments.map((fragment) => { - if (!requiresRetainedParagraph(fragment.text, store.font)) return null; + if (!requiresRetainedParagraph(fragment.text, store.asciiMetrics)) return null; const paragraph = createFragmentParagraph({ cell, fragment, @@ -88,11 +98,13 @@ export function createNativeTextMeasurer(input: { }): TextMeasurer { const families = nativeFontFamilies(input.configuredFamily); const font = primaryFont(families, input.fontSize); + const primary = { + glyphIds: (text: string) => font.getGlyphIDs(text), + measure: (text: string) => font.getTextWidth(text), + }; + const asciiMetrics = createCachedAsciiTextMetrics(primary); const fallbackAware = createFallbackAwareTextMeasurer({ - primary: { - glyphIds: (text) => font.getGlyphIDs(text), - measure: (text) => font.getTextWidth(text), - }, + primary, measureWithSystemFallback(text) { const paragraph = createParagraph({ text, @@ -110,13 +122,8 @@ export function createNativeTextMeasurer(input: { ...fallbackAware, measureAdvances(graphemes) { const text = graphemes.join(""); - if (!requiresRetainedParagraph(text, font)) { - let prefix = ""; - return graphemes.map((grapheme) => { - prefix += grapheme; - return font.getTextWidth(prefix); - }); - } + if (!requiresRetainedParagraph(text, asciiMetrics)) + return asciiMetrics.measureAdvances(graphemes); const paragraph = createParagraph({ text, families, @@ -139,8 +146,8 @@ export function createNativeTextMeasurer(input: { }; } -function requiresRetainedParagraph(text: string, font: SkFont): boolean { - return requiresNativeParagraph(text) || font.getGlyphIDs(text).some((glyph) => glyph === 0); +function requiresRetainedParagraph(text: string, asciiMetrics: CachedAsciiTextMetrics): boolean { + return requiresNativeParagraph(text) || !asciiMetrics.hasEveryGlyph(text); } function createFragmentParagraph(input: { diff --git a/packages/app/src/git/diff-document/viewport.test.ts b/packages/app/src/git/diff-document/viewport.test.ts new file mode 100644 index 000000000..97e826ae3 --- /dev/null +++ b/packages/app/src/git/diff-document/viewport.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; +import { retainDiffViewport } from "./viewport"; + +describe("retainDiffViewport", () => { + it("waits for the first positive layout", () => { + const initial = { width: 0, height: 0 }; + + expect(retainDiffViewport(initial, { width: 0, height: 700 })).toBe(initial); + expect(retainDiffViewport(initial, { width: 390, height: 0 })).toBe(initial); + expect(retainDiffViewport(initial, { width: 390, height: 700 })).toEqual({ + width: 390, + height: 700, + }); + }); + + it("preserves the completed viewport while a retained panel is hidden", () => { + const visible = { width: 390, height: 700 }; + + expect(retainDiffViewport(visible, { width: 0, height: 0 })).toBe(visible); + }); + + it("accepts a real resize and preserves identity for duplicate layouts", () => { + const current = { width: 390, height: 700 }; + + expect(retainDiffViewport(current, current)).toBe(current); + expect(retainDiffViewport(current, { width: 700, height: 390 })).toEqual({ + width: 700, + height: 390, + }); + }); +}); diff --git a/packages/app/src/git/diff-document/viewport.ts b/packages/app/src/git/diff-document/viewport.ts new file mode 100644 index 000000000..32d9b6f0b --- /dev/null +++ b/packages/app/src/git/diff-document/viewport.ts @@ -0,0 +1,10 @@ +export interface DiffViewport { + width: number; + height: number; +} + +export function retainDiffViewport(current: DiffViewport, next: DiffViewport): DiffViewport { + if (next.width <= 0 || next.height <= 0) return current; + if (current.width === next.width && current.height === next.height) return current; + return next; +}