From d5cf089ba083db1fe8be6eba6d4565b9fcabe02f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 12:05:37 +0000 Subject: [PATCH] Bound the recent-files menu, and stop a test resolving a path two ways Two headless editor tests failed on macOS and were made Linux-only rather than diagnosed. Neither was about macOS. MenuTests.OpeningARecentFileLoadsIt clicked a recent file whose menu label was its whole path. A menu is as wide as its widest label, and a submenu that will not fit beside the menu that opened it is placed on top of that menu - which puts the item that opened it under the submenu rather than under the pointer, so ImGui takes the pointer to have left and closes the submenu on the frame after it opens. The item was therefore drawn exactly once and never again, and the click landed on a stale rectangle. macOS only picked itself out by reaching its temporary directory through a path 44 characters deeper than /tmp; the same thing happened on Linux with TMPDIR set to a macOS-shaped path, and it happened to any user who had saved a schema somewhere deep enough, on any platform, leaving Open Recent unusable for them. So the label is elided to a width that fits, the whole path stays as the item's id and in a tooltip, and forty characters is measured rather than chosen: the submenu grows about eight pixels a character, which leaves it inside the 700-pixel window the narrowest of these tests drives, where sixty does not. FileBrowserTests.SavingRecordsTheChosenPathAsRecentlyUsed was a different failure that happened to share a menu - an assertion, not the stale-item exception. It expected the name it created its scratch directory under, but macOS reaches the temporary directory through /var, a symbolic link to /private/var, and the working directory resolves the link, so the browser answered with the other spelling of the same directory. It now reads the working directory back after setting it, which reproduces and fixes under a symlinked TMPDIR on Linux too. EditorHarness.Click asked whether the probe had ever recorded a name rather than whether the item was on screen, so an item drawn once and then gone still satisfied the wait. It now asks IsOnScreen on both sides of the settle frames. That is a better message rather than a fix: with the wrong question the failure was inside the click, with the right one it is at the wait, and only the elision makes it pass. The elision is pinned frameless in Schema.Editor.Test, so the one platform-sensitive fact here is covered everywhere the frame-driven suite cannot run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0131bjn9xRwnWMQB2yJFP1cN --- CLAUDE.md | 30 ++++++++ Schema.Editor.Test/RecentFileLabelTests.cs | 68 +++++++++++++++++++ Schema.Editor/SchemaEditor.Files.cs | 49 ++++++++++++- tests/Schema.Editor.UITests/EditorHarness.cs | 13 +++- .../Schema.Editor.UITests/FileBrowserTests.cs | 6 ++ tests/Schema.Editor.UITests/MenuTests.cs | 38 +++++++++++ 6 files changed, 202 insertions(+), 2 deletions(-) create mode 100644 Schema.Editor.Test/RecentFileLabelTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 96dab05..0cb9313 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -540,6 +540,36 @@ records its tabs nor takes a selection from outside. A panel behind it - the cla diagnostics list - is tested by drawing it directly in a `WidgetHarness`, which is what the tab's own delegate does. +### What a Linux-only suite stops seeing + +The frame-driven suite runs on Linux alone, and the two tests that first failed elsewhere were +failing for reasons that had nothing to do with the operating system - which is exactly what a +one-platform suite is bad at telling you. + +`FileBrowserTests` expected the path it made its scratch directory under. macOS reaches the +temporary directory through `/var`, a symbolic link to `/private/var`, and the working directory +resolves the link, so the browser answered with the other spelling of the same directory. The test +now reads the working directory back after setting it, which is a fact about symbolic links rather +than about macOS. + +`MenuTests` clicked a recent file whose menu label was its whole path. A menu is as wide as its +widest label, and a submenu that will not fit beside the menu that opened it is placed **on top of +that menu** - which puts the item that opened it under the submenu rather than under the pointer, +so ImGui takes the pointer to have left and closes the submenu on the frame after it opens. The +temporary directory being 44 characters deeper on macOS is all that picked which platform noticed: +any schema saved somewhere deep enough did the same everywhere, and Open Recent was unusable for +whoever owned it. `SchemaEditor.ElideRecentFileLabel` bounds the label and the tooltip keeps the +path. Both halves are pinned - the mechanism in `MenuTests.ARecentFileTooDeepToLabelWholeStillOpens`, +and the elision itself in `Schema.Editor.Test/RecentFileLabelTests.cs`, which needs no frame and so +runs on every platform. That is the answer to promoting the suite: what was platform-sensitive here +was a string, and a string can be tested where the rasterizer cannot go. + +`EditorHarness.Click` asked whether the probe had **ever** recorded a name, not whether the item was +on screen now, so an item drawn once and then gone still satisfied the wait and the click failed +later on a stale rectangle. It now asks `IsOnScreen` on both sides of the settle frames. That is a +better message, not a fix: with the wrong question it failed inside the click, with the right one it +fails at the wait, and only the elision makes it pass. + ## Dependencies - **ktsu.Semantics.Strings/Paths** - Type-safe string and path wrappers diff --git a/Schema.Editor.Test/RecentFileLabelTests.cs b/Schema.Editor.Test/RecentFileLabelTests.cs new file mode 100644 index 0000000..c0e399c --- /dev/null +++ b/Schema.Editor.Test/RecentFileLabelTests.cs @@ -0,0 +1,68 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Schema.Editor.Test; + +/// +/// The label a recent file is offered under in the File menu. +/// +/// +/// +/// Offering the whole path was not a display preference that happened to be verbose. A menu is as +/// wide as its widest label, and a submenu that will not fit beside the menu that opened it is +/// placed on top of that menu instead - which puts the item that opened it underneath the submenu +/// rather than under the pointer, so ImGui takes the pointer to have left and closes the submenu +/// on the frame after it opened. Open Recent was therefore unusable for anyone whose schemas lived +/// deep enough, and how deep that was depended on the window's width. +/// +/// +/// Frameless, so it runs on every platform. That matters here more than usual: this was found by +/// two tests failing on macOS alone, for no reason macOS was responsible for - its temporary +/// directory is simply 44 characters deeper than /tmp. +/// +/// +[TestClass] +public sealed class RecentFileLabelTests +{ + [TestMethod] + public void APathThatFitsIsOfferedWhole() + { + string path = new('x', SchemaEditor.MaxRecentFileLabelLength); + + Assert.AreEqual(path, SchemaEditor.ElideRecentFileLabel(path)); + } + + [TestMethod] + public void ALongerPathIsCutToTheSameWidth() + { + string label = SchemaEditor.ElideRecentFileLabel(new string('x', SchemaEditor.MaxRecentFileLabelLength * 4)); + + Assert.AreEqual(SchemaEditor.MaxRecentFileLabelLength, label.Length, "A label past the budget is what makes the menu wider than the window."); + } + + /// + /// The end rather than the beginning: what tells two recent files apart is the file name and + /// the directory holding it, and the root they share is the part worth losing. + /// + [TestMethod] + public void WhatIsKeptIsTheEndOfThePath() + { + string path = $"/a/very/long/root/that/nobody/needs/to/read/again/and/again/and/again/{new string('d', 20)}/schema.json"; + + string label = SchemaEditor.ElideRecentFileLabel(path); + + StringAssert.EndsWith(label, $"{new string('d', 20)}/schema.json", StringComparison.Ordinal); + StringAssert.StartsWith(label, "…", StringComparison.Ordinal); + } + + /// + /// A file name longer than the whole budget still leaves a label of the right width, rather + /// than a negative slice or a label that is once again as wide as the path. + /// + [TestMethod] + public void AFileNameLongerThanTheBudgetIsCutToo() + { + string path = $"/schemas/{new string('n', SchemaEditor.MaxRecentFileLabelLength * 2)}.schema.json"; + + Assert.AreEqual(SchemaEditor.MaxRecentFileLabelLength, SchemaEditor.ElideRecentFileLabel(path).Length); + } +} diff --git a/Schema.Editor/SchemaEditor.Files.cs b/Schema.Editor/SchemaEditor.Files.cs index ef79c11..85ae8b1 100644 --- a/Schema.Editor/SchemaEditor.Files.cs +++ b/Schema.Editor/SchemaEditor.Files.cs @@ -38,6 +38,42 @@ public partial class SchemaEditor ? "Untitled schema" : Path.GetFileName(CurrentSchemaPath); + /// + /// The longest label a recent file is offered under, in characters. + /// + /// + /// + /// A menu is as wide as its widest label, and Open Recent's labels are absolute paths, so its + /// width was whatever depth the user happened to have saved at. That is not merely untidy: a + /// submenu that does not fit beside the menu that opened it is placed on top of it, and the + /// item that opened it is then underneath the submenu rather than under the pointer, which + /// ImGui reads as the pointer having left - so the submenu closes on the frame after it opens + /// and the menu cannot be used at all. + /// + /// + /// Forty is measured rather than chosen for looks: the submenu opens a little past the File + /// menu's own width and grows by about eight pixels a character, so forty leaves it inside the + /// 700-pixel window the narrowest of these tests drives, where sixty does not. The whole path + /// is still a hover away. + /// + /// + internal const int MaxRecentFileLabelLength = 40; + + /// + /// Shortens a path to for display, keeping its end. + /// + /// + /// The end rather than the beginning, because what tells two recent files apart is the file + /// name and the directory holding it. What is dropped is the root they are most likely to + /// share. + /// + /// The path to label. + /// The path itself if it fits, or its last characters behind an ellipsis. + internal static string ElideRecentFileLabel(string path) => + path.Length <= MaxRecentFileLabelLength + ? path + : $"…{path[^(MaxRecentFileLabelLength - 1)..]}"; + private void ShowRecentFilesMenu() { IReadOnlyList recent = [.. Options.RecentFiles]; @@ -58,12 +94,23 @@ private void ShowRecentFilesMenu() } anyShown = true; - bool clicked = ImGui.MenuItem(path); + + // The whole path is the id and the elided path is the label, so two files with the + // same name stay separate items however alike they read. + ImGui.PushID(path); + bool clicked = ImGui.MenuItem(ElideRecentFileLabel(path)); // Recorded under the file name rather than the whole path, which a probe name would // otherwise read as a chain of scopes because both are separated by slashes. ImGuiProbes.MarkItem("recent", Path.GetFileName(path)); + if (ImGui.IsItemHovered()) + { + ImGui.SetTooltip(path); + } + + ImGui.PopID(); + if (clicked) { AbsoluteFilePath captured = path; diff --git a/tests/Schema.Editor.UITests/EditorHarness.cs b/tests/Schema.Editor.UITests/EditorHarness.cs index 670c990..f40d808 100644 --- a/tests/Schema.Editor.UITests/EditorHarness.cs +++ b/tests/Schema.Editor.UITests/EditorHarness.cs @@ -100,16 +100,27 @@ internal void StepUntil(Func condition, string description, int maxFrames /// Waits for a marked item to be drawn, then clicks it. /// /// + /// /// The frames between the item first appearing and the click are not padding. A modal sizes /// itself from its contents on the frame it appears and is centred on the next, so the /// rectangle recorded for a control on its first frame is not where that control ends up; /// clicking there hits the background instead. + /// + /// + /// Both waits ask rather than whether the probe has ever + /// recorded the name, and the second one is why there are two: an item can be drawn once and + /// then go away again while those frames run, and the name it left behind is enough to satisfy + /// a wait that only asks whether the probe has heard of it. Asked this way, a test that clicks + /// something no longer there says so, instead of failing inside the click on a rectangle that + /// has since been taken by whatever moved into it. + /// /// /// A marked name, or the trailing part of one. internal void Click(string item) { - StepUntil(() => App.Probe.Matches(item).Count > 0, $"'{item}' appearing"); + StepUntil(() => IsOnScreen(item), $"'{item}' appearing"); App.Step(3); + StepUntil(() => IsOnScreen(item), $"'{item}' still being drawn once it had settled"); App.Click(item); App.Step(2); } diff --git a/tests/Schema.Editor.UITests/FileBrowserTests.cs b/tests/Schema.Editor.UITests/FileBrowserTests.cs index 3aad4fd..12cb4da 100644 --- a/tests/Schema.Editor.UITests/FileBrowserTests.cs +++ b/tests/Schema.Editor.UITests/FileBrowserTests.cs @@ -39,6 +39,12 @@ public void StartEditor() previousWorkingDirectory = Directory.GetCurrentDirectory(); Directory.SetCurrentDirectory(scratchDirectory); + // Read the directory back rather than keeping the name it was created under. On macOS the + // temporary directory is reached through /var, which is a symbolic link to /private/var, + // and the working directory resolves the link - so the browser, which opens on the working + // directory, answers with the spelling that the name it was created under is not. + scratchDirectory = Directory.GetCurrentDirectory().As(); + harness = EditorHarness.Start(); } diff --git a/tests/Schema.Editor.UITests/MenuTests.cs b/tests/Schema.Editor.UITests/MenuTests.cs index 5a56212..e3415c2 100644 --- a/tests/Schema.Editor.UITests/MenuTests.cs +++ b/tests/Schema.Editor.UITests/MenuTests.cs @@ -225,6 +225,44 @@ public void OpeningARecentFileLoadsIt() Assert.AreEqual("Recalled", harness.Editor.CurrentClass?.Name.ToString()); } + /// + /// A schema saved somewhere deep is still offered and still opens. + /// + /// + /// Labelled with its whole path, such a file made Open Recent wider than the room beside the + /// File menu, and a submenu that does not fit beside the menu that opened it is placed on top + /// of it - so the item that opened it sat under the submenu rather than under the pointer, + /// ImGui took the pointer to have left, and the submenu closed on the frame after it opened. + /// This is the case that found it, in the only form that says what it was: the two tests it + /// broke failed on macOS alone, whose temporary directory is 44 characters deeper than + /// /tmp and which had nothing else to do with it. + /// + [TestMethod] + public void ARecentFileTooDeepToLabelWholeStillOpens() + { + AbsoluteDirectoryPath deep = scratchDirectory; + for (int level = 0; level < 3; level++) + { + deep /= $"a-directory-named-at-length-{level}".As(); + } + + Directory.CreateDirectory(deep); + + Schema source = new(); + source.AddClass("Deep".As()); + AbsoluteFilePath path = deep / "deep.schema.json".As(); + File.WriteAllText(path, SchemaSerializer.Serialize(source)); + harness.Editor.Options.RecordRecentFile(path); + + Assert.IsGreaterThan(SchemaEditor.MaxRecentFileLabelLength, path.ToString().Length, "The path was not long enough to be the case this pins."); + + harness.ChooseMenuItem("File", "Open Recent"); + harness.Click("recent/deep.schema.json"); + + Assert.AreEqual(path, harness.Editor.CurrentSchemaPath); + Assert.AreEqual("Deep", harness.Editor.CurrentClass?.Name.ToString()); + } + [TestMethod] public void UndoFromTheEditMenuRevertsTheLastEdit() {