From d7b1efeba096c565f3252bfd7705620ddb01df91 Mon Sep 17 00:00:00 2001 From: Storm Dragon Date: Thu, 22 Oct 2026 16:39:44 -0400 Subject: [PATCH] Some hopefully final flat review touch ups. --- docs/testing/flat-review-tables.md | 10 +++++++- internal/atspi/review_cache_native_test.go | 1 + internal/atspi/text_bounds_test.go | 29 ++++++++++++++++++++++ internal/atspi/text_native.go | 20 ++++++++++++--- tests/review_cache_fixture.c | 15 +++++++++++ 5 files changed, 71 insertions(+), 4 deletions(-) diff --git a/docs/testing/flat-review-tables.md b/docs/testing/flat-review-tables.md index b47c589..119c0ed 100644 --- a/docs/testing/flat-review-tables.md +++ b/docs/testing/flat-review-tables.md @@ -16,6 +16,12 @@ Their rectangles are combined across every chunk, including interior bidi runs. For partially clipped lines, offscreen chunks are rejected before querying individual character rectangles. This preserves native offsets and visible text without measuring thousands of characters in one synchronous request. +Exact range rectangles are reused only within one object's synchronous text +capture, including zero-area rectangles. This avoids querying the same 128-character +blocks once to combine line bounds and again to clip the line. The map is discarded +when that object's text capture returns; a changed viewport or later command does +not reuse it. Failed requests are not stored, and cancellation is checked before +using a saved rectangle. Individual glyph checks for clipped text remain intact. In the live FastSMRW investigation, measuring an entire 3,424-character line took about 0.9 seconds, exceeding the adapter's 0.8-second provider timeout. A later @@ -78,7 +84,9 @@ dependencies during the test. The long-range regression uses a horizontally clipped 3,424-character GTK label. Its provider deliberately stalls when a range query exceeds 128 characters. -Capture must retain visible text and never trigger that large request. Unit +Capture must retain visible text and never trigger that large request. The fixture +also counts repeated native range requests: the original implementation repeats +36 requests, while capture-local reuse makes none. Unit tests also cover Unicode offsets, offscreen query counts, interior bidi bounds, zero-size chunks, cancellation and provider errors. diff --git a/internal/atspi/review_cache_native_test.go b/internal/atspi/review_cache_native_test.go index b483116..9291a6c 100644 --- a/internal/atspi/review_cache_native_test.go +++ b/internal/atspi/review_cache_native_test.go @@ -203,6 +203,7 @@ func test_native_review_provider(t *testing.T, mode string) { } if mode == "bounds" { command("largest", "largest 128") + command("repeated", "repeated 0") } if mode == "lines" && text.Ranges[0].Start <= maxReviewCharacters { t.Fatalf("line viewport did not seek scrolled native offsets: %d", text.Ranges[0].Start) diff --git a/internal/atspi/text_bounds_test.go b/internal/atspi/text_bounds_test.go index 80b6877..02591c9 100644 --- a/internal/atspi/text_bounds_test.go +++ b/internal/atspi/text_bounds_test.go @@ -88,3 +88,32 @@ func TestBoundedReviewBoundsStopsOnFailureOrCancellation(t *testing.T) { t.Fatalf("cancellation: %v queries=%d", err, queries) } } + +// Even a block entirely inside the viewport can contain zero-area glyphs. +// A horizontally clipped line must retain its existing per-glyph exclusions. +type zeroGlyphReviewText struct{ longReviewText } + +func (source *zeroGlyphReviewText) bounds(ctx context.Context, start, end int) (Rectangle, error) { + if start == 5 && end == 6 { + return Rectangle{}, ctx.Err() + } + return source.longReviewText.bounds(ctx, start, end) +} + +func TestCaptureVisibleTextClippedInteriorBlockExcludesZeroAreaGlyph(t *testing.T) { + source := &zeroGlyphReviewText{longReviewText: longReviewText{runes: []rune(strings.Repeat("x", 400))}} + got, err := captureVisibleText(context.Background(), source, Rectangle{Width: 2000, Height: 20}) + if err != nil || !got.Known || got.Limited { + t.Fatalf("capture: %+v, %v", got, err) + } + characters := 0 + for _, span := range got.Ranges { + if span.Start <= 5 && span.End > 5 { + t.Fatalf("zero-area glyph included: %+v", span) + } + characters += span.End - span.Start + } + if characters != 199 { + t.Fatalf("visible characters=%d, want 199", characters) + } +} diff --git a/internal/atspi/text_native.go b/internal/atspi/text_native.go index 8ffc16c..153fbd3 100644 --- a/internal/atspi/text_native.go +++ b/internal/atspi/text_native.go @@ -129,6 +129,9 @@ func expandNativeEmbeddedChild(source *C.AtspiAccessible, offset, depth int, bud type nativeReviewText struct { text *C.AtspiText terminal bool + // Exact range rectangles belong to one synchronous text capture. Reuse + // block measurements during clipping without retaining state across commands. + boundsCache map[[2]int]Rectangle } func (source nativeReviewText) count(ctx context.Context) (int, error) { @@ -250,6 +253,10 @@ func (source nativeReviewText) range_bounds(ctx context.Context, start, end int) if err := ctx.Err(); err != nil { return Rectangle{}, err } + key := [2]int{start, end} + if bounds, known := source.boundsCache[key]; known { + return bounds, nil + } var nativeErr *C.GError bounds := C.atspi_text_get_range_extents(source.text, C.gint(start), C.gint(end), C.ATSPI_COORD_TYPE_SCREEN, &nativeErr) if bounds != nil { @@ -264,7 +271,13 @@ func (source nativeReviewText) range_bounds(ctx context.Context, start, end int) return Rectangle{}, errors.New("missing review range bounds") } rect := Rectangle{int32(bounds.x), int32(bounds.y), int32(bounds.width), int32(bounds.height)} - return rect, ctx.Err() + if err := ctx.Err(); err != nil { + return Rectangle{}, err + } + if source.boundsCache != nil { + source.boundsCache[key] = rect + } + return rect, nil } // Only called from captureNativeTree on the adapter's GLib thread. Unsupported, @@ -281,7 +294,8 @@ func textFromNative(ctx context.Context, accessible *C.AtspiAccessible, geometry return VisibleText{}, ctx.Err() } defer C.g_object_unref(C.gpointer(text)) - result, err := captureVisibleText(ctx, nativeReviewText{text: text, terminal: snapshot.Role == "terminal"}, clip) + source := nativeReviewText{text: text, terminal: snapshot.Role == "terminal", boundsCache: make(map[[2]int]Rectangle)} + result, err := captureVisibleText(ctx, source, clip) if err != nil { if ctx.Err() != nil { return VisibleText{}, ctx.Err() @@ -292,7 +306,7 @@ func textFromNative(ctx context.Context, accessible *C.AtspiAccessible, geometry // Some providers advertise Text on component-only controls and // containers. Verify their bounded content before allowing the // established accessible-name/child-content path to handle them. - if recovered, ok := recover_review_component(ctx, nativeReviewText{text: text}, snapshot, geometry, clip); ok { + if recovered, ok := recover_review_component(ctx, source, snapshot, geometry, clip); ok { result = recovered } } diff --git a/tests/review_cache_fixture.c b/tests/review_cache_fixture.c index c4bdc24..f70c451 100644 --- a/tests/review_cache_fixture.c +++ b/tests/review_cache_fixture.c @@ -10,6 +10,8 @@ static gboolean armed; static unsigned childQueries; static gboolean slowRanges; static gint largestRange; +static GHashTable *measuredRanges; +static guint repeatedRanges; static gboolean lineGeometryOnly; static gboolean missingGlyph; static GtkWidget *reviewScroller; @@ -28,6 +30,13 @@ static void range_extents(AtkText *text, gint start, gint end, AtkCoordType coor } if (slowRanges) { largestRange = MAX(largestRange, end - start); + guint64 key = ((guint64)(guint32)start << 32) | (guint32)end; + if (g_hash_table_contains(measuredRanges, &key)) repeatedRanges++; + else { + guint64 *stored = g_new(guint64, 1); + *stored = key; + g_hash_table_add(measuredRanges, stored); + } if (end - start > 128) g_usleep(1200000); } originalRangeExtents(text, start, end, coordinates, rectangle); @@ -64,7 +73,11 @@ static gboolean command_ready(GIOChannel *channel, GIOCondition condition, gpoin printf("count %u\n", childQueries); } else if (g_str_has_prefix(line, "measure")) { slowRanges = TRUE; + g_hash_table_remove_all(measuredRanges); + repeatedRanges = 0; puts("measuring"); + } else if (g_str_has_prefix(line, "repeated")) { + printf("repeated %u\n", repeatedRanges); } else if (g_str_has_prefix(line, "largest")) { printf("largest %d\n", largestRange); } else if (g_str_has_prefix(line, "scroll")) { @@ -92,6 +105,7 @@ int main(int argc, char **argv) { lineGeometryOnly = linesMode; missingGlyph = glyphsMode; gtk_init(&argc, &argv); + measuredRanges = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, NULL); GtkWidget *window = gtk_window_new(GTK_WINDOW_TOPLEVEL); gtk_window_set_title(GTK_WINDOW(window), "Review cache probe"); gtk_window_set_default_size(GTK_WINDOW(window), 300, 100); @@ -143,5 +157,6 @@ int main(int argc, char **argv) { gtk_widget_show_all(window); gtk_main(); g_io_channel_unref(input); + g_hash_table_unref(measuredRanges); return 0; }