From ea712886602d6fbd9a6159df4ea277415652e581 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:59:59 +0100 Subject: [PATCH 1/9] Stop autohiding scrollbars (#464) (#465) Co-authored-by: Claude Opus 5 --- src/PlanViewer.App/App.axaml | 21 ++++++ .../ScrollBarVisibilityTests.cs | 70 +++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 tests/PlanViewer.Core.Tests/ScrollBarVisibilityTests.cs diff --git a/src/PlanViewer.App/App.axaml b/src/PlanViewer.App/App.axaml index 8a8c17df..3dc52470 100644 --- a/src/PlanViewer.App/App.axaml +++ b/src/PlanViewer.App/App.axaml @@ -14,6 +14,27 @@ + + + + + + + + diff --git a/tests/PlanViewer.Core.Tests/ScrollBarVisibilityTests.cs b/tests/PlanViewer.Core.Tests/ScrollBarVisibilityTests.cs new file mode 100644 index 00000000..9d54d670 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ScrollBarVisibilityTests.cs @@ -0,0 +1,70 @@ +using System.Linq; +using Avalonia.Controls; +using Avalonia.Controls.Primitives; +using Avalonia.VisualTree; + +namespace PlanViewer.Core.Tests; + +/// +/// #464: every scrollbar in the app collapsed to a sliver until hovered, because Fluent defaults +/// AllowAutoHide to true and nothing ever set it otherwise. The reporter's complaint was +/// about a horizontal bar, which is where it hurts most — the thing you need to grab is a couple +/// of pixels tall until after you have found it. +/// +/// Two selectors are needed, not one, and that is the whole reason this test exists. A +/// rule alone looks like it covers the app and does not: DataGrid does +/// not scroll through a ScrollViewer, its template hosts PART_HorizontalScrollbar and +/// PART_VerticalScrollbar as bare s. A style that compiles is not a style +/// that matches, so both are asserted against controls that have actually been styled rather than +/// against the App.axaml text. +/// +public class ScrollBarVisibilityTests +{ + [Fact] + public void AScrollViewerDoesNotAutoHideItsBars() + { + HeadlessUi.Run(() => + { + var scrollViewer = new ScrollViewer { Content = new TextBlock { Text = "x" } }; + Show(scrollViewer); + + Assert.False( + scrollViewer.AllowAutoHide, + "the app-wide ScrollViewer style should have turned auto-hide off"); + }); + } + + [Fact] + public void ADataGridsOwnScrollBarsDoNotAutoHideEither() + { + HeadlessUi.Run(() => + { + /* The grid needs columns and rows before its template puts scrollbars in the tree, + so this is a real grid rather than an empty one. */ + var grid = new DataGrid + { + ItemsSource = Enumerable.Range(0, 50).Select(i => new { Value = i }).ToList() + }; + Show(grid); + + var bars = grid.GetVisualDescendants().OfType().ToList(); + + Assert.NotEmpty(bars); + Assert.All(bars, bar => Assert.False( + bar.AllowAutoHide, + "a DataGrid's scrollbars are bare ScrollBars and need their own rule")); + }); + } + + /// + /// Puts a control in a window and forces a layout pass, so styles are applied and templated + /// children exist. Nothing is asserted before this runs — an unstyled control reports the + /// Fluent default and would pass for the wrong reason. + /// + private static void Show(Control content) + { + var window = new Window { Content = content, Width = 400, Height = 300 }; + window.Show(); + window.UpdateLayout(); + } +} From 21a6e10639b3c37323743ef0ea735dd065733e1b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:44:41 +0100 Subject: [PATCH 2/9] Restore query tabs on restart, and let Copy Path see them (#463) (#468) Plan tabs came back after a restart. Query tabs did not, even when the query had been opened from a file and the path was sitting on the session control. GetTabFilePath knew exactly one tab shape: a DockPanel with a PlanViewerControl inside it. A query tab is a QuerySessionControl with no wrapper, and it has had a SourceFilePath of its own since #459. It was simply never asked, so SaveOpenPlans wrote nothing down and RestoreOpenPlans had nothing to bring back. The same blind spot hid Copy Path on the tab context menu, which is shown only when GetTabFilePath answers. It had never appeared on a query tab. With both kinds of file in one saved list, restore routes on extension through the existing OpenFileByExtension instead of assuming a plan. Handing a .sql file to LoadPlanFile produced an "XML is not valid" box where the user's query should have been. The setting is renamed open_plans -> open_tabs now that it holds both. A file written by the previous version is still read: the old key deserializes into a migration-only property that is merged into OpenTabs on load and then nulled, so it drops out of the file on the next save rather than taking a user's restored tabs with it. Scope: this restores query tabs that came from a file. A scratch tab that was never saved has no path and still does not come back; persisting unsaved buffers is #462. Claude-Session: https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx Co-authored-by: Claude Fable 5.1 --- .../Dialogs/SettingsWindow.axaml.cs | 2 +- src/PlanViewer.App/MainWindow.FileOps.cs | 38 ++- src/PlanViewer.App/MainWindow.Tabs.cs | 14 + .../Services/AppSettingsService.cs | 36 ++- .../RestoreQueryTabsTests.cs | 240 ++++++++++++++++++ 5 files changed, 319 insertions(+), 11 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs diff --git a/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs b/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs index 9aaa24be..420746fc 100644 --- a/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs +++ b/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs @@ -639,7 +639,7 @@ private void ResetAll_Click(object? sender, RoutedEventArgs e) var fresh = new AppSettings { RecentPlans = _settings.RecentPlans, - OpenPlans = _settings.OpenPlans, + OpenTabs = _settings.OpenTabs, AccuracyRatioDivergenceLimit = _settings.AccuracyRatioDivergenceLimit }; _settings = fresh; diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 99c56d6e..05b30bba 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -384,11 +384,16 @@ private bool ValidatePlanXml(string xml, string label) } /// - /// Saves the file paths of all currently open file-based plan tabs. + /// The file behind every open tab that has one, in tab order. Plans and queries both, + /// since answers for either shape. /// - private void SaveOpenPlans() + /// + /// Separate from so a test can assert what would be persisted + /// without writing over the user's real settings file. + /// + internal List CollectOpenTabPaths() { - _appSettings.OpenPlans.Clear(); + var paths = new List(); foreach (var item in MainTabControl.Items) { @@ -396,31 +401,46 @@ private void SaveOpenPlans() var path = GetTabFilePath(tab); if (!string.IsNullOrEmpty(path)) - _appSettings.OpenPlans.Add(path); + paths.Add(path); } + return paths; + } + + /// + /// Saves the file paths of all currently open file-based tabs, plans and queries alike. + /// + private void SaveOpenPlans() + { + _appSettings.OpenTabs.Clear(); + _appSettings.OpenTabs.AddRange(CollectOpenTabPaths()); + AppSettingsService.Save(_appSettings); } /// - /// Restores plan tabs from the previous session. Skips files that no longer exist. + /// Restores the tabs from the previous session. Skips files that no longer exist. /// Falls back to a new query tab if nothing was restored. + /// + /// The saved list holds queries as well as plans, so it routes on extension the + /// same way an ordinary file open does. Sending a .sql file to LoadPlanFile would greet + /// the user with "the XML is not valid" where their query used to be. /// private void RestoreOpenPlans() { var restored = false; - foreach (var path in _appSettings.OpenPlans) + foreach (var path in _appSettings.OpenTabs) { if (File.Exists(path)) { - LoadPlanFile(path); + OpenFileByExtension(path); restored = true; } } - // Clear the open plans list now that we've restored - _appSettings.OpenPlans.Clear(); + // Clear the restored list now that its tabs are back on screen + _appSettings.OpenTabs.Clear(); AppSettingsService.Save(_appSettings); if (!restored) diff --git a/src/PlanViewer.App/MainWindow.Tabs.cs b/src/PlanViewer.App/MainWindow.Tabs.cs index 029e8549..d47477d1 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -187,6 +187,15 @@ private static void SetTabLabel(TabItem tab, string label) text.Text = label; } + /// + /// The file a tab was opened from, or null when nothing on disk is behind it + /// (a pasted plan, a scratch query, a Query Store tab). + /// + /// Every tab shape that can carry a path has to be recognised here, because this one + /// method answers two questions: what writes down for the next + /// session, and whether Copy Path appears on the tab's context menu. A shape it does + /// not know about loses both without saying anything. + /// private static string? GetTabFilePath(TabItem tab) { // Plans opened from file are wrapped in a DockPanel with the viewer as the last child @@ -198,6 +207,11 @@ private static void SetTabLabel(TabItem tab, string label) return v.SourceFilePath; } } + + // Queries are the session control itself, with no wrapper around it + if (tab.Content is QuerySessionControl session) + return session.SourceFilePath; + return null; } diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index f62ae717..56dcd678 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -69,6 +69,11 @@ public static AppSettings Load() settings = JsonSerializer.Deserialize(json, JsonOptions) ?? new AppSettings(); } + // Settings written before the open-tab list held queries use the old key. + // Ahead of MigrateFormatSettings, which can Save mid-load and would otherwise + // write the old key straight back out. + MigrateOpenTabs(settings); + // Migrate legacy format settings file into unified settings MigrateFormatSettings(settings); @@ -117,6 +122,23 @@ public static void Save(AppSettings settings) } } + /// + /// Moves an "open_plans" list written by an older version onto , + /// so upgrading does not cost the user the tabs they had open. Only fills an empty OpenTabs: + /// if both keys are somehow present, the current one wins. + /// + /// + /// Nothing is written here. The old key disappears from disk on the next ordinary save, which + /// happens on the first restore, so a downgrade before that point still finds its list. + /// + internal static void MigrateOpenTabs(AppSettings settings) + { + if (settings.LegacyOpenPlans is { Count: > 0 } legacy && settings.OpenTabs.Count == 0) + settings.OpenTabs = legacy; + + settings.LegacyOpenPlans = null; + } + /// /// If the old perfstudio_format_settings.json exists, migrate it into AppSettings /// (when FormatOptions is not yet set) and delete the old file unconditionally. @@ -214,8 +236,20 @@ internal sealed class AppSettings [JsonPropertyName("recent_plans")] public List RecentPlans { get; set; } = new(); + /// + /// Paths of the tabs that were open when the app last closed, reopened on the next start. + /// Holds queries as well as plans, which is why it is no longer named for plans. + /// + [JsonPropertyName("open_tabs")] + public List OpenTabs { get; set; } = new(); + + /// + /// What was called before it held queries too. Read on load so an + /// upgrade does not throw away the tabs the previous version wrote down, then nulled — + /// nulls are not serialized, so the old key drops out of the file on the next save. + /// [JsonPropertyName("open_plans")] - public List OpenPlans { get; set; } = new(); + public List? LegacyOpenPlans { get; set; } /// /// Divergence limit for accuracy ratio coloring on plan links. Default 10. diff --git a/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs new file mode 100644 index 00000000..366001b9 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs @@ -0,0 +1,240 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using System.Threading.Tasks; +using Avalonia.Controls; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #463: plan tabs came back after a restart and query tabs did not, even when the query had been +/// opened from a file and its path was sitting right there on the session. The saved-tab list was +/// built from GetTabFilePath, which knew how to look inside a plan tab and nothing else, so a query +/// tab was invisible to it — nothing was written down, so nothing could be restored. +/// +/// The same blind spot hid Copy Path on the tab context menu, which is shown only when +/// GetTabFilePath answers, and so had never once appeared on a query tab. Both halves are covered +/// here because they are one defect, and the menu half is the easier one to fix by accident and +/// never actually check. +/// +/// These drive LoadSqlFile and the MainWindow constructor rather than the menu handlers, for the +/// same reason does: a file picker cannot be answered headlessly. +/// +public class RestoreQueryTabsTests +{ + [Fact] + public void AQueryOpenedFromAFileIsWrittenDownForTheNextSession() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS restored;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + Assert.Contains(path, window.CollectOpenTabPaths()); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// The deliberate edge of the fix. A never-saved scratch buffer has no path, so there is + /// nothing to write down and it does not come back — persisting unsaved text is #462's job, + /// not this one's. + /// + [Fact] + public void AScratchQueryHasNoFileAndIsNotWrittenDown() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var session = Sessions(window).Last(); + Assert.Null(session.SourceFilePath); + Assert.Empty(window.CollectOpenTabPaths()); + }); + } + + [Fact] + public void AQueryFileComesBackAsAQueryTabOnTheNextStart() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS came_back;"); + try + { + Seed(path); + + var window = new MainWindow(); + + var session = Sessions(window).SingleOrDefault(s => s.SourceFilePath == path); + Assert.NotNull(session); + Assert.Equal("SELECT 1 AS came_back;", session!.QueryEditor.Text); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// The saved list now holds both kinds of file, so restore routes on extension. This is the + /// half that would break if that routing sent everything to LoadSqlFile instead. + /// + [Fact] + public void APlanFileStillComesBackAsAPlanTab() + { + HeadlessUi.Run(() => + { + var path = Path.Combine(System.AppContext.BaseDirectory, "Plans", "row_goal_plan.sqlplan"); + Seed(path); + + var window = new MainWindow(); + + Assert.Contains(path, window.CollectOpenTabPaths()); + Assert.Contains(Viewers(window), v => v.SourceFilePath == path); + }); + } + + [Fact] + public void CopyPathIsOfferedOnAQueryTabAndCopiesTheFile() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS copied;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = window.MainTabControl.Items + .OfType() + .Last(t => t.Content is QuerySessionControl); + + var copyPath = ContextMenuItem(tab, "Copy Path"); + Assert.True(copyPath.IsVisible, + "the query came from a file, so there is a path to copy"); + + copyPath.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + + Assert.Equal(path, Pump(ClipboardHelper.TryGetTextAsync(window))); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// A scratch tab has no path, so the menu item stays hidden — the gate still gates. + /// + [Fact] + public void CopyPathStaysHiddenOnAQueryTabWithNoFile() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var tab = window.MainTabControl.Items + .OfType() + .Last(t => t.Content is QuerySessionControl); + + Assert.False(ContextMenuItem(tab, "Copy Path").IsVisible); + }); + } + + /// + /// The list outgrew the name "open_plans" once it started holding queries. Renaming the key + /// is free for a new install and expensive for an existing one, so the old key is still read. + /// + [Fact] + public void TabsRecordedUnderTheOldSettingsKeyAreStillRestored() + { + var settings = JsonSerializer.Deserialize( + """{"open_plans":["/tmp/one.sqlplan","/tmp/two.sql"]}""")!; + + AppSettingsService.MigrateOpenTabs(settings); + + Assert.Equal(new[] { "/tmp/one.sqlplan", "/tmp/two.sql" }, settings.OpenTabs); + Assert.Null(settings.LegacyOpenPlans); + } + + /// + /// Both keys present means a downgrade wrote the old one after the new one already existed. + /// The current key is the one that reflects the last session. + /// + [Fact] + public void TheCurrentSettingsKeyWinsOverTheOldOne() + { + var settings = JsonSerializer.Deserialize( + """{"open_plans":["/tmp/stale.sqlplan"],"open_tabs":["/tmp/current.sql"]}""")!; + + AppSettingsService.MigrateOpenTabs(settings); + + Assert.Equal(new[] { "/tmp/current.sql" }, settings.OpenTabs); + Assert.Null(settings.LegacyOpenPlans); + } + + /// + /// Puts one path where the next MainWindow will look for the previous session's tabs. + /// Load returns the process-wide cached instance, which is the same object the window reads, + /// and restore clears it again on the way out — so this does not leak into other tests. + /// + private static void Seed(string path) + { + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + settings.OpenTabs.Add(path); + } + + private static MenuItem ContextMenuItem(TabItem tab, string header) => + ((StackPanel)tab.Header!).ContextMenu!.Items + .OfType() + .Single(i => (i.Header as string) == header); + + /// + /// Drains the UI queue until the clipboard call finishes. Copy Path starts its write and + /// does not await it, so the read that follows can be a dispatcher turn early. Fails rather + /// than hangs if the headless clipboard never answers. + /// + private static T Pump(Task task) + { + for (var i = 0; i < 100 && !task.IsCompleted; i++) + Dispatcher.UIThread.RunJobs(); + + Assert.True(task.IsCompleted, "the clipboard call never completed"); + return task.GetAwaiter().GetResult(); + } + + private static string TempSql(string text) + { + var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sql"); + File.WriteAllText(path, text); + return path; + } + + private static IEnumerable Sessions(MainWindow window) => + window.MainTabControl.Items.OfType().Select(t => t.Content).OfType(); + + private static IEnumerable Viewers(MainWindow window) => + window.MainTabControl.Items.OfType() + .Select(t => t.Content) + .OfType() + .SelectMany(d => d.Children) + .OfType(); +} From 17e4240138f13dc7be031ae08ab9aa14557133dd Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:45:00 +0100 Subject: [PATCH 3/9] Warn about unsaved query changes, and mark modified tabs (#462) (#469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A query session tracked where it came from but never what was in it, so nothing in the app could tell an edited tab from an untouched one. Closing either lost the same amount of work: all of it, silently. The dirty flag is the feature; the close prompt and the modified marker are both consumers of it. QuerySessionControl now keeps the text as of the last load or save and compares against it, so typing something back to how it started leaves the session clean rather than latching modified on the first keystroke. Close now asks, on every path that discards a tab — the x, middle-click, the context menu, Ctrl+W — and on the window itself, which walks every tab rather than the one in front. Cancel cancels. A never-saved scratch tab answering Save routes through Save As, because saving it in place would write over nothing. The tab's x becomes a filled dot while there are unsaved changes and reverts to the x under the pointer, so a marked tab is still a closable one. Both glyphs are \u escapes, matching the "✕" they replaced. Claude-Session: https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx Co-authored-by: Claude Opus 5 (1M context) --- .../Controls/QuerySessionControl.axaml.cs | 37 +++ .../Dialogs/UnsavedChangesDialog.cs | 102 +++++++ src/PlanViewer.App/MainWindow.FileOps.cs | 19 +- src/PlanViewer.App/MainWindow.Tabs.cs | 43 +-- src/PlanViewer.App/MainWindow.axaml.cs | 165 +++++++++++- .../UnsavedQueryChangesTests.cs | 252 ++++++++++++++++++ 6 files changed, 594 insertions(+), 24 deletions(-) create mode 100644 src/PlanViewer.App/Dialogs/UnsavedChangesDialog.cs create mode 100644 tests/PlanViewer.Core.Tests/UnsavedQueryChangesTests.cs diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index 37d0759c..8be55abe 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -37,6 +37,39 @@ public partial class QuerySessionControl : UserControl /// public string? SourceFilePath { get; set; } + /// + /// The editor text as of the last load or save. A new session starts empty, so a + /// never-saved scratch tab with anything typed into it is dirty too (#462). + /// + private string _savedText = ""; + + /// + /// Whether the editor holds work that is not on disk. + /// + /// This compares text rather than latching a "was edited" bool on the first + /// keystroke, so typing something and typing it back out again leaves the session + /// clean — an undo to the original is not unsaved work, and prompting about it is + /// how a save prompt teaches people to dismiss save prompts. + /// + public bool IsDirty => !string.Equals(QueryEditor.Text, _savedText, StringComparison.Ordinal); + + /// + /// Raised whenever the editor text changes or the session is marked clean. The tab + /// header subscribes to this to keep its modified marker honest; the session cannot + /// reach its own tab, and polling per render would be worse. + /// + public event EventHandler? DirtyStateChanged; + + /// + /// Declares the current text to be what is on disk. Called after a load and after a + /// successful save — not after a failed one, which must leave the session dirty. + /// + public void MarkClean() + { + _savedText = QueryEditor.Text; + DirtyStateChanged?.Invoke(this, EventArgs.Empty); + } + private ServerConnection? _serverConnection; private string? _connectionString; private string? _selectedDatabase; @@ -72,6 +105,10 @@ public QuerySessionControl(ICredentialService credentialService, ConnectionStore QueryEditor.TextArea.TextEntering += OnTextEntering; QueryEditor.TextArea.TextEntered += OnTextEntered; + // #462: every edit is a chance for the tab's modified marker to change, in both + // directions — an undo back to the saved text clears it again. + QueryEditor.TextChanged += (_, _) => DirtyStateChanged?.Invoke(this, EventArgs.Empty); + // Focus the editor when the control is attached to the visual tree // Re-install TextMate if it was disposed on detach (tab switching disposes it) AttachedToVisualTree += (_, _) => diff --git a/src/PlanViewer.App/Dialogs/UnsavedChangesDialog.cs b/src/PlanViewer.App/Dialogs/UnsavedChangesDialog.cs new file mode 100644 index 00000000..708c1cfd --- /dev/null +++ b/src/PlanViewer.App/Dialogs/UnsavedChangesDialog.cs @@ -0,0 +1,102 @@ +using System.Threading.Tasks; +using Avalonia.Controls; +using Avalonia.Layout; +using Avalonia.Media; + +namespace PlanViewer.App.Dialogs; + +/// +/// What the user said when asked about a query tab with unsaved changes. +/// +public enum UnsavedChangesChoice +{ + /// Write the query out, then close. + Save, + + /// Close and lose the edit — an explicit answer, not a default. + DontSave, + + /// Do not close anything. + Cancel +} + +/// +/// The Save / Don't Save / Cancel prompt for a modified query tab (#462). +/// +/// Separate from rather than a parameter on it: this +/// has three answers, and the third one has to be distinguishable from the second. A yes/no +/// dialog collapses "don't save" and "cancel" into the same false, which is the one mistake +/// this prompt cannot make — it is the difference between losing a tab and losing the app. +/// +public static class UnsavedChangesDialog +{ + /// + /// Asks about one tab. Dismissing the window any other way — title-bar close, Escape — + /// is , because the safe answer to a question + /// nobody answered is to leave the work where it is. + /// + public static async Task ShowAsync(Window owner, string tabLabel) + { + var choice = UnsavedChangesChoice.Cancel; + + var messageText = new TextBlock + { + Text = $"Do you want to save the changes you made to {tabLabel}?\n\nYour changes will be lost if you don't save them.", + TextWrapping = TextWrapping.Wrap, + FontSize = 13, + Foreground = new SolidColorBrush(Color.Parse("#E4E6EB")), + Margin = new Avalonia.Thickness(0, 0, 0, 16) + }; + + var buttonPanel = new StackPanel + { + Orientation = Orientation.Horizontal, + HorizontalAlignment = HorizontalAlignment.Right + }; + + var dialog = new Window + { + Title = "Unsaved Changes", + Width = 460, + Height = 220, + MinWidth = 460, + MinHeight = 220, + Icon = owner.Icon, + Background = new SolidColorBrush(Color.Parse("#1A1D23")), + Foreground = new SolidColorBrush(Color.Parse("#E4E6EB")), + WindowStartupLocation = WindowStartupLocation.CenterOwner + }; + + Button MakeButton(string caption, UnsavedChangesChoice answer) + { + var button = new Button + { + Content = caption, + Height = 32, + MinWidth = 96, + Padding = new Avalonia.Thickness(16, 0), + FontSize = 12, + Margin = new Avalonia.Thickness(8, 0, 0, 0), + HorizontalContentAlignment = HorizontalAlignment.Center, + VerticalContentAlignment = VerticalAlignment.Center, + Theme = (Avalonia.Styling.ControlTheme)owner.FindResource("AppButton")! + }; + button.Click += (_, _) => { choice = answer; dialog.Close(); }; + buttonPanel.Children.Add(button); + return button; + } + + MakeButton("Save", UnsavedChangesChoice.Save); + MakeButton("Don't Save", UnsavedChangesChoice.DontSave); + MakeButton("Cancel", UnsavedChangesChoice.Cancel); + + dialog.Content = new StackPanel + { + Margin = new Avalonia.Thickness(20), + Children = { messageText, buttonPanel } + }; + + await dialog.ShowDialog(owner); + return choice; + } +} diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 05b30bba..189dcef2 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -112,6 +112,17 @@ private async Task SaveQueryAsync() if (MainTabControl.SelectedItem is not TabItem { Content: QuerySessionControl session } tab) return; + await SaveQueryAsync(tab, session); + } + + /// + /// Saves a named tab rather than whichever one is selected. The unsaved-changes prompt + /// (#462) walks every tab, so it needs to save one that is not the active one, and a + /// scratch tab answering Save arrives here to get a path. + /// + /// Whether the query reached disk. False also covers the user closing the picker. + private async Task SaveQueryAsync(TabItem tab, QuerySessionControl session) + { var existing = session.SourceFilePath; var options = new FilePickerSaveOptions @@ -139,8 +150,7 @@ private async Task SaveQueryAsync() var file = await StorageProvider.SaveFilePickerAsync(options); var path = file?.TryGetLocalPath(); - if (path != null) - SaveQueryToPath(tab, session, path); + return path != null && SaveQueryToPath(tab, session, path); } /// @@ -153,6 +163,9 @@ internal bool SaveQueryToPath(TabItem tab, QuerySessionControl session, string p { File.WriteAllText(path, session.QueryEditor.Text); session.SourceFilePath = path; + // Only a write that actually happened settles the dirty state; the catch below + // deliberately leaves the session modified so the work is still guarded. + session.MarkClean(); SetTabLabel(tab, Path.GetFileName(path)); return true; } @@ -243,6 +256,8 @@ internal void LoadSqlFile(string filePath) var session = new QuerySessionControl(_credentialService, _connectionStore); session.QueryEditor.Text = text; session.SourceFilePath = filePath; + // What was just loaded is what is on disk — the baseline every later edit is measured against. + session.MarkClean(); var tab = CreateTab(fileName, session); MainTabControl.Items.Add(tab); diff --git a/src/PlanViewer.App/MainWindow.Tabs.cs b/src/PlanViewer.App/MainWindow.Tabs.cs index d47477d1..6c9e7830 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -48,7 +48,7 @@ private TabItem CreateTab(string label, Control content) var closeBtn = new Button { - Content = "\u2715", + Content = CloseGlyph, MinWidth = 22, MinHeight = 22, Width = 22, @@ -75,13 +75,29 @@ private TabItem CreateTab(string label, Control content) closeBtn.Tag = tab; closeBtn.Click += CloseTab_Click; + /* #462: the modified marker. Only query sessions have a dirty state, and the button + has to re-decide on pointer-over as well as on edits, because hovering is what turns + the dot back into a close button. */ + if (content is QuerySessionControl querySession) + { + void RefreshCloseGlyph() => + closeBtn.Content = CloseButtonGlyph(querySession.IsDirty, closeBtn.IsPointerOver); + + querySession.DirtyStateChanged += (_, _) => RefreshCloseGlyph(); + closeBtn.PointerEntered += (_, _) => RefreshCloseGlyph(); + closeBtn.PointerExited += (_, _) => RefreshCloseGlyph(); + + // A session can arrive already modified — re-docking a detached window builds a + // fresh tab around a session that has been edited since it left. + RefreshCloseGlyph(); + } + // Middle-click to close header.PointerPressed += (_, e) => { if (e.GetCurrentPoint(null).Properties.PointerUpdateKind == PointerUpdateKind.MiddleButtonPressed) { - MainTabControl.Items.Remove(tab); - UpdateEmptyOverlay(); + _ = TryCloseTabAsync(tab); e.Handled = true; } }; @@ -118,10 +134,7 @@ private TabItem CreateTab(string label, Control content) private void CloseTab_Click(object? sender, RoutedEventArgs e) { if (sender is Button btn && btn.Tag is TabItem tab) - { - MainTabControl.Items.Remove(tab); - UpdateEmptyOverlay(); - } + _ = TryCloseTabAsync(tab); } private void TabContextMenu_Click(object? sender, RoutedEventArgs e) @@ -148,26 +161,16 @@ private void TabContextMenu_Click(object? sender, RoutedEventArgs e) case "Close": if (item.Tag is TabItem tab) - { - MainTabControl.Items.Remove(tab); - UpdateEmptyOverlay(); - } + _ = TryCloseTabAsync(tab); break; case "Close Other Tabs": if (item.Tag is TabItem keepTab) - { - var others = MainTabControl.Items.Cast().Where(t => t != keepTab).ToList(); - foreach (var t in others) - MainTabControl.Items.Remove(t); - MainTabControl.SelectedItem = keepTab; - UpdateEmptyOverlay(); - } + _ = CloseOtherTabsAsync(keepTab); break; case "Close All Tabs": - MainTabControl.Items.Clear(); - UpdateEmptyOverlay(); + _ = CloseTabsAsync(MainTabControl.Items.Cast().ToList()); break; case "Detach to Window": diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 154cd728..5e2c77a4 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -105,8 +105,7 @@ refresh that has to be remembered at sixteen call sites is one that gets forgott case Key.W: if (MainTabControl.SelectedItem is TabItem selected) { - MainTabControl.Items.Remove(selected); - UpdateEmptyOverlay(); + _ = TryCloseTabAsync(selected); e.Handled = true; } break; @@ -260,6 +259,168 @@ private void UpdateEmptyOverlay() } + // ── Unsaved query changes (#462) ────────────────────────────────────── + + /// + /// Set once every tab has been asked about, so the second pass through + /// does not ask the whole window again. + /// + private bool _closeConfirmed; + + private const string CloseGlyph = "\u2715"; // ✕ + private const string ModifiedGlyph = "\u25CF"; // ● + + /// + /// What a tab's close button shows. Dirty tabs get a filled dot, but it reverts to the + /// × while the pointer is over the button — a marker you cannot click through is a tab + /// you cannot close, which is the trade VS Code makes and the one Josh asked for. + /// + internal static string CloseButtonGlyph(bool isDirty, bool isPointerOver) => + isDirty && !isPointerOver ? ModifiedGlyph : CloseGlyph; + + /// + /// Whether closing this tab would throw work away. Plan tabs are read-only, so only a + /// query session can ever answer yes. + /// + internal static bool HasUnsavedChanges(TabItem tab) => + tab.Content is QuerySessionControl { IsDirty: true }; + + /// + /// Every tab that would lose work if the window closed right now, in tab order. + /// + /// Deliberately not "the selected tab": the edit at risk is usually in the tab the + /// user is not looking at, which is the whole reason a window-close prompt exists. + /// + internal List TabsWithUnsavedChanges() => + MainTabControl.Items.OfType().Where(HasUnsavedChanges).ToList(); + + /// + /// What the close path does with an answer to the prompt. + /// + internal enum CloseAction + { + /// Proceed with the close. + Close, + + /// Leave the tab, and the window, alone. + Cancel, + + /// Overwrite the file the session came from, then close. + SaveInPlace, + + /// Ask where to put it first, then close if it was actually written. + SaveAs + } + + /// + /// Turns an answer into an action, split out from the dialog so the decision can be + /// tested without a window to click. A never-saved scratch tab has nowhere to write, so + /// its Save has to become a Save As rather than silently doing nothing. + /// + internal static CloseAction DecideClose(UnsavedChangesChoice choice, bool hasFile) => choice switch + { + UnsavedChangesChoice.Cancel => CloseAction.Cancel, + UnsavedChangesChoice.DontSave => CloseAction.Close, + _ => hasFile ? CloseAction.SaveInPlace : CloseAction.SaveAs + }; + + /// + /// Asks about a tab if it needs asking about. Returns whether the close may proceed — + /// false means the user cancelled, or a save they asked for failed. + /// + private async Task ConfirmCloseAsync(TabItem tab) + { + if (!HasUnsavedChanges(tab)) + return true; + + var session = (QuerySessionControl)tab.Content!; + + MainTabControl.SelectedItem = tab; // show what is being asked about + var choice = await UnsavedChangesDialog.ShowAsync(this, GetTabLabel(tab)); + + return DecideClose(choice, session.SourceFilePath != null) switch + { + CloseAction.Cancel => false, + CloseAction.Close => true, + CloseAction.SaveInPlace => SaveQueryToPath(tab, session, session.SourceFilePath!), + _ => await SaveQueryAsync(tab, session) + }; + } + + /// + /// Closes one tab, asking first. Returns false when the close was refused, so callers + /// closing several tabs can stop rather than plough on past a Cancel. + /// + private async Task TryCloseTabAsync(TabItem tab) + { + if (!await ConfirmCloseAsync(tab)) + return false; + + MainTabControl.Items.Remove(tab); + UpdateEmptyOverlay(); + return true; + } + + /// + /// Closes a run of tabs. Cancelling any one of them abandons the rest: "close all tabs" + /// answered with Cancel means the user changed their mind about all of them, not about + /// that one. + /// + private async Task CloseTabsAsync(IEnumerable tabs) + { + foreach (var tab in tabs.ToList()) + { + if (!await TryCloseTabAsync(tab)) + return; + } + } + + /// + /// Closes everything but one tab, then puts the survivor back in front — it may not have + /// been the selected tab, and moves the selection around + /// to show what it is asking about. + /// + private async Task CloseOtherTabsAsync(TabItem keepTab) + { + await CloseTabsAsync(MainTabControl.Items.OfType().Where(t => t != keepTab)); + + if (MainTabControl.Items.Contains(keepTab)) + MainTabControl.SelectedItem = keepTab; + } + + /// + /// Holds the window open long enough to ask about every modified tab. + /// + /// The prompt is async and Closing is not, so the first pass cancels the close + /// outright and re-issues it from once every tab + /// has answered. Anything that is not a query tab, and any window with nothing modified, + /// closes on the first pass without a detour. + /// + protected override void OnClosing(WindowClosingEventArgs e) + { + if (!_closeConfirmed && TabsWithUnsavedChanges().Count > 0) + { + e.Cancel = true; + _ = ConfirmWindowCloseAsync(); + return; + } + + base.OnClosing(e); + } + + private async Task ConfirmWindowCloseAsync() + { + foreach (var tab in MainTabControl.Items.OfType().ToList()) + { + if (!await ConfirmCloseAsync(tab)) + return; // one Cancel cancels the shutdown + } + + _closeConfirmed = true; + Close(); + } + + private void Exit_Click(object? sender, RoutedEventArgs e) { Close(); diff --git a/tests/PlanViewer.Core.Tests/UnsavedQueryChangesTests.cs b/tests/PlanViewer.Core.Tests/UnsavedQueryChangesTests.cs new file mode 100644 index 00000000..b9c492af --- /dev/null +++ b/tests/PlanViewer.Core.Tests/UnsavedQueryChangesTests.cs @@ -0,0 +1,252 @@ +using System.IO; +using System.Linq; +using Avalonia.Controls; +using Avalonia.Interactivity; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Dialogs; + +namespace PlanViewer.Core.Tests; + +/// +/// #462: a query session had no idea whether it had been edited. Open a .sql, change it, close +/// the tab or the window, and the edit was gone with nothing asked and nothing marked. +/// +/// The dirty flag is the whole feature — the close prompt and the modified marker are both +/// consumers of it — so most of what is worth testing is the flag itself and the decision the +/// prompt feeds. The dialog cannot be clicked headlessly, which is why the answer is a value +/// () handed to rather +/// than something only the dialog knows. Same reasoning as OpenSaveQueryTests and the file +/// picker: what the human chooses is untestable, everything downstream of the choice is not. +/// +public class UnsavedQueryChangesTests +{ + [Fact] + public void EditingAFileBackedQueryMakesItDirtyAndMarksTheTab() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + var session = (QuerySessionControl)tab.Content!; + + Assert.False(session.IsDirty, "a file just loaded is what is on disk"); + Assert.Equal("\u2715", CloseButton(tab).Content); + + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + + Assert.True(session.IsDirty); + Assert.True(MainWindow.HasUnsavedChanges(tab)); + Assert.Equal("\u25CF", CloseButton(tab).Content); + } + finally + { + File.Delete(path); + } + }); + } + + [Fact] + public void TypingBackToTheOriginalTextClearsTheDirtyState() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + var session = (QuerySessionControl)tab.Content!; + + session.QueryEditor.Text = "SELECT 2;"; + Assert.True(session.IsDirty); + + /* The point of comparing text rather than latching a bool on the first keystroke: + an undo back to the file's contents is not unsaved work, and being asked about it + is what teaches people to click through save prompts without reading them. */ + session.QueryEditor.Text = "SELECT 1;"; + + Assert.False(session.IsDirty); + Assert.Equal("\u2715", CloseButton(tab).Content); + } + finally + { + File.Delete(path); + } + }); + } + + [Fact] + public void AScratchTabWithTypedContentIsDirtyAndItsSaveGoesThroughSaveAs() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var tab = LastQueryTab(window); + var session = (QuerySessionControl)tab.Content!; + + Assert.False(session.IsDirty, "an empty new query has nothing to lose"); + + session.QueryEditor.Text = "SELECT 'never saved';"; + + Assert.True(session.IsDirty); + Assert.Null(session.SourceFilePath); + Assert.Equal("\u25CF", CloseButton(tab).Content); + + /* Nowhere to write it, so Save has to become Save As. Saving "in place" over a null + path is the one outcome that would throw away the query it was trying to rescue. */ + Assert.Equal( + MainWindow.CloseAction.SaveAs, + MainWindow.DecideClose(UnsavedChangesChoice.Save, hasFile: false)); + }); + } + + [Fact] + public void ASuccessfulSaveSettlesTheTabAndAFailedOneDoesNot() + { + HeadlessUi.Run(() => + { + var opened = TempSql("SELECT 1;"); + var savedAs = Path.Combine(Path.GetTempPath(), $"saved_{Path.GetRandomFileName()}.sql"); + /* A directory that does not exist, so File.WriteAllText throws. */ + var unwritable = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName(), "nope.sql"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(opened); + + var tab = LastQueryTab(window); + var session = (QuerySessionControl)tab.Content!; + session.QueryEditor.Text = "SELECT 2 AS edited;"; + + Assert.False(window.SaveQueryToPath(tab, session, unwritable)); + Assert.True(session.IsDirty, "a save that threw has not saved anything"); + Assert.Equal("\u25CF", CloseButton(tab).Content); + + Assert.True(window.SaveQueryToPath(tab, session, savedAs)); + Assert.False(session.IsDirty); + Assert.Equal("\u2715", CloseButton(tab).Content); + } + finally + { + File.Delete(opened); + if (File.Exists(savedAs)) + File.Delete(savedAs); + } + }); + } + + [Fact] + public void TheModifiedMarkerGivesWayToTheCloseButtonUnderThePointer() + { + /* Josh's ask, and the reason the marker is a swap rather than an extra glyph: a dot you + cannot click is a tab you cannot close. */ + Assert.Equal("\u25CF", MainWindow.CloseButtonGlyph(isDirty: true, isPointerOver: false)); + Assert.Equal("\u2715", MainWindow.CloseButtonGlyph(isDirty: true, isPointerOver: true)); + Assert.Equal("\u2715", MainWindow.CloseButtonGlyph(isDirty: false, isPointerOver: false)); + Assert.Equal("\u2715", MainWindow.CloseButtonGlyph(isDirty: false, isPointerOver: true)); + } + + [Fact] + public void ClosingTheWindowAsksAboutEveryTabNotJustTheSelectedOne() + { + HeadlessUi.Run(() => + { + var first = TempSql("SELECT 1;"); + var second = TempSql("SELECT 2;"); + var third = TempSql("SELECT 3;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(first); + window.LoadSqlFile(second); + window.LoadSqlFile(third); + + var tabs = QueryTabs(window).ToList(); + var firstTab = tabs[^3]; + var secondTab = tabs[^2]; + var thirdTab = tabs[^1]; + + Edit(firstTab, "SELECT 1; -- edited"); + Edit(secondTab, "SELECT 2; -- edited"); + + /* The tab in front is the clean one. The edits at risk are behind it, which is the + ordinary case and exactly what a window-close prompt is for. */ + window.MainTabControl.SelectedItem = thirdTab; + + Assert.Equal( + new[] { firstTab, secondTab }, + window.TabsWithUnsavedChanges()); + } + finally + { + File.Delete(first); + File.Delete(second); + File.Delete(third); + } + }); + } + + [Fact] + public void PlanTabsAreNeverDirty() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.LoadPlanFile(Path.Combine("Plans", "row_goal_plan.sqlplan")); + + /* Plans are read-only. Nothing about opening one should make the app ask whether to + save it, and the restored-on-startup query tab must not be dragged in either. */ + Assert.Empty(window.TabsWithUnsavedChanges()); + }); + } + + [Fact] + public void CancelRefusesTheCloseAndDontSaveGoesThroughWithIt() + { + Assert.Equal( + MainWindow.CloseAction.Cancel, + MainWindow.DecideClose(UnsavedChangesChoice.Cancel, hasFile: true)); + Assert.Equal( + MainWindow.CloseAction.Cancel, + MainWindow.DecideClose(UnsavedChangesChoice.Cancel, hasFile: false)); + + Assert.Equal( + MainWindow.CloseAction.Close, + MainWindow.DecideClose(UnsavedChangesChoice.DontSave, hasFile: true)); + Assert.Equal( + MainWindow.CloseAction.Close, + MainWindow.DecideClose(UnsavedChangesChoice.DontSave, hasFile: false)); + + Assert.Equal( + MainWindow.CloseAction.SaveInPlace, + MainWindow.DecideClose(UnsavedChangesChoice.Save, hasFile: true)); + } + + private static void Edit(TabItem tab, string text) => + ((QuerySessionControl)tab.Content!).QueryEditor.Text = text; + + private static string TempSql(string text) + { + var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sql"); + File.WriteAllText(path, text); + return path; + } + + private static System.Collections.Generic.IEnumerable QueryTabs(MainWindow window) => + window.MainTabControl.Items.OfType().Where(t => t.Content is QuerySessionControl); + + private static TabItem LastQueryTab(MainWindow window) => QueryTabs(window).Last(); + + private static Button CloseButton(TabItem tab) => + ((StackPanel)tab.Header!).Children.OfType /// Whether the query reached disk. False also covers the user closing the picker. - private async Task SaveQueryAsync(TabItem tab, QuerySessionControl session) + private async Task SaveQueryAsync(TabItem? tab, QuerySessionControl session, IStorageProvider? storage = null) { + var picker = storage ?? StorageProvider; var existing = session.SourceFilePath; var options = new FilePickerSaveOptions @@ -144,10 +152,10 @@ private async Task SaveQueryAsync(TabItem tab, QuerySessionControl session { var directory = Path.GetDirectoryName(existing); if (!string.IsNullOrEmpty(directory)) - options.SuggestedStartLocation = await StorageProvider.TryGetFolderFromPathAsync(directory); + options.SuggestedStartLocation = await picker.TryGetFolderFromPathAsync(directory); } - var file = await StorageProvider.SaveFilePickerAsync(options); + var file = await picker.SaveFilePickerAsync(options); var path = file?.TryGetLocalPath(); return path != null && SaveQueryToPath(tab, session, path); @@ -156,8 +164,12 @@ private async Task SaveQueryAsync(TabItem tab, QuerySessionControl session /// /// The half of saving that does not need a human: write the text, remember where it went, /// and retitle the tab to match. Split out from the picker so it can be tested. + /// + /// is null for a session that has been detached into its own + /// window (#473). There is no tab to retitle; everything else about the save is the same, + /// including which side of the write settles the dirty state. /// - internal bool SaveQueryToPath(TabItem tab, QuerySessionControl session, string path) + internal bool SaveQueryToPath(TabItem? tab, QuerySessionControl session, string path) { try { @@ -166,7 +178,10 @@ internal bool SaveQueryToPath(TabItem tab, QuerySessionControl session, string p // Only a write that actually happened settles the dirty state; the catch below // deliberately leaves the session modified so the work is still guarded. session.MarkClean(); - SetTabLabel(tab, Path.GetFileName(path)); + + if (tab != null) + SetTabLabel(tab, Path.GetFileName(path)); + return true; } catch (Exception ex) diff --git a/src/PlanViewer.App/MainWindow.Tabs.cs b/src/PlanViewer.App/MainWindow.Tabs.cs index 05a69695..8871dc8a 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -289,11 +289,17 @@ private static string GetQueryTextFromPlan(PlanViewerControl viewer) /// Detaches a tab's content into a standalone free-floating window. /// The window's Close button closes it permanently. /// A "Re-dock" button in the toolbar allows the user to explicitly return the content to a tab. + /// + /// #473: permanently used to mean silently. A query session that leaves the tab strip + /// takes its unsaved edit with it, out of reach of both #462 prompts, so the window gets a + /// close guard and the session goes on the detached register until it comes back or the + /// window closes. /// - private void DetachTabToWindow(TabItem tab) + /// The detached window, or null when the tab had no content to detach. + internal Window? DetachTabToWindow(TabItem tab) { var content = tab.Content as Control; - if (content == null) return; + if (content == null) return null; var label = GetTabLabel(tab); @@ -305,13 +311,15 @@ private void DetachTabToWindow(TabItem tab) if (content is QueryStoreHistoryControl historyControl) historyControl.ShowCloseButton(false); - DetachedWindowHelper.ShowDetached( + var detachedWindow = DetachedWindowHelper.ShowDetached( content, title: label, icon: this.Icon, backgroundBrush: (Avalonia.Media.IBrush?)this.FindResource("BackgroundBrush"), onRedock: c => { + ForgetDetachedQuerySession(c); + if (!IsShuttingDown) { var newTab = CreateTab(label, c); @@ -322,8 +330,14 @@ private void DetachTabToWindow(TabItem tab) }, onClosing: c => { + ForgetDetachedQuerySession(c); + if (c is QueryStoreHistoryControl hc) hc.CancelFetch(); - }); + }, + closeGuard: DetachedQueryCloseGuard); + + RememberDetachedQuerySession(detachedWindow, content); + return detachedWindow; } } diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 100cc3d3..466574c1 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -294,6 +294,122 @@ internal static bool HasUnsavedChanges(TabItem tab) => internal List TabsWithUnsavedChanges() => MainTabControl.Items.OfType().Where(HasUnsavedChanges).ToList(); + // ── Unsaved changes in a detached window (#473) ─────────────────────── + + /// + /// The query sessions living in a detached window right now, and the window each is in. + /// + /// A detached session is not in .Items, so + /// cannot see it: without this register the shutdown + /// prompt honestly reports nothing to save while a dirty edit sits in another window. + /// Detaching adds to it; re-docking and closing both take back out, which is why the + /// register is keyed on the content control rather than on the tab it came from — by then + /// there is no tab. + /// + /// Only query sessions go on it. Plan and Query Store windows are read-only and have + /// nothing to lose, so there is nothing to remember about them. + /// + private readonly List<(Window Window, QuerySessionControl Session)> _detachedQuerySessions = new(); + + /// Every query session currently detached, dirty or not. + internal IReadOnlyList<(Window Window, QuerySessionControl Session)> DetachedQuerySessions => + _detachedQuerySessions; + + internal void RememberDetachedQuerySession(Window window, Control content) + { + if (content is QuerySessionControl session) + _detachedQuerySessions.Add((window, session)); + } + + internal void ForgetDetachedQuerySession(Control content) + { + if (content is QuerySessionControl session) + _detachedQuerySessions.RemoveAll(d => d.Session == session); + } + + /// + /// Whether closing a detached window has to stop and ask. + /// + /// Static and pure so the decision is testable without a window to click, the same + /// trade makes. Three ways to answer no, and each one matters: + /// read-only content (a plan, a Query Store window) has nothing to save, an unmodified + /// session has nothing to save, and once the app is shutting down the question has already + /// been asked — asks it while the main window is + /// still up, precisely so that can force these windows shut without + /// raising a dialog over a window that no longer exists. + /// + internal static bool DetachedContentNeedsSavePrompt(Control content, bool isShuttingDown) => + !isShuttingDown && content is QuerySessionControl { IsDirty: true }; + + /// + /// The close guard handed to every detached window. Returning null is the whole read-only + /// path: no prompt, no cancelled close, no detour. + /// + internal Task? DetachedQueryCloseGuard(Control content, Window detachedWindow) => + DetachedContentNeedsSavePrompt(content, IsShuttingDown) + ? ConfirmDetachedCloseAsync((QuerySessionControl)content, detachedWindow) + : null; + + /// + /// Asks about a detached session, owned by the window that is being closed rather than by + /// the main one — the answer is about that window's content, and a prompt parented to a + /// window behind it is a prompt nobody can see. The file picker a Save As needs is taken + /// from the same window for the same reason. + /// + /// Whether the close may proceed. + private async Task ConfirmDetachedCloseAsync(QuerySessionControl session, Window owner) + { + // The shutdown walk works from a snapshot, and a save taken during it settles more than + // its own entry. Same early out ConfirmCloseAsync has, for the same reason. + if (!session.IsDirty) + return true; + + owner.Activate(); // show what is being asked about, as the tab walk does with selection + + var choice = await UnsavedChangesDialog.ShowAsync(owner, owner.Title ?? "this query"); + + return DecideClose(choice, session.SourceFilePath != null) switch + { + CloseAction.Cancel => false, + CloseAction.Close => true, + CloseAction.SaveInPlace => SaveQueryToPath(null, session, session.SourceFilePath!), + _ => await SaveQueryAsync(null, session, owner.StorageProvider) + }; + } + + /// + /// Every detached window that would lose work if the app closed right now. + /// + internal List<(Window Window, QuerySessionControl Session)> DetachedSessionsWithUnsavedChanges() => + _detachedQuerySessions.Where(d => d.Session.IsDirty).ToList(); + + /// + /// Everything closing the app would throw away, in the order it will be asked about: the + /// tabs first in tab order, then the detached windows. + /// + /// One list rather than two walks, because two walks is how the second one gets + /// forgotten — which is the whole of #473. Tab is null for a detached session; there + /// is no tab, and Owner is the window that has to own its prompt. + /// + internal List<(TabItem? Tab, QuerySessionControl Session, Window Owner)> UnsavedWorkOnClose() + { + var work = new List<(TabItem? Tab, QuerySessionControl Session, Window Owner)>(); + + foreach (var tab in TabsWithUnsavedChanges()) + work.Add((tab, (QuerySessionControl)tab.Content!, this)); + + foreach (var detached in DetachedSessionsWithUnsavedChanges()) + work.Add((null, detached.Session, detached.Window)); + + return work; + } + + /// + /// Whether closing the main window has anything to ask about at all. Tabs and detached + /// windows both count: "Detach to Window" is not a way to opt out of being asked. + /// + internal bool CloseNeedsConfirmation() => UnsavedWorkOnClose().Count > 0; + /// /// What the close path does with an answer to the prompt. /// @@ -395,10 +511,13 @@ private async Task CloseOtherTabsAsync(TabItem keepTab) /// outright and re-issues it from once every tab /// has answered. Anything that is not a query tab, and any window with nothing modified, /// closes on the first pass without a detour. + /// + /// #473: "every tab" is not everything. A detached session is off the tab strip and + /// still holds unsaved work, so counts those too. /// protected override void OnClosing(WindowClosingEventArgs e) { - if (!_closeConfirmed && TabsWithUnsavedChanges().Count > 0) + if (!_closeConfirmed && CloseNeedsConfirmation()) { e.Cancel = true; _ = ConfirmWindowCloseAsync(); @@ -410,9 +529,19 @@ protected override void OnClosing(WindowClosingEventArgs e) private async Task ConfirmWindowCloseAsync() { - foreach (var tab in MainTabControl.Items.OfType().ToList()) + /* #473: the sessions that are no longer tabs are asked about here, while every window + is still up, rather than from OnClosed where they are force-closed. By then the main + window is gone and the app is on its way out — a dialog raised there is at best a + window nobody expects and at worst a shutdown that never finishes. Asking here is + what earns DetachedContentNeedsSavePrompt the right to wave that force-close + through. */ + foreach (var work in UnsavedWorkOnClose()) { - if (!await ConfirmCloseAsync(tab)) + var mayClose = work.Tab != null + ? await ConfirmCloseAsync(work.Tab) + : await ConfirmDetachedCloseAsync(work.Session, work.Owner); + + if (!mayClose) return; // one Cancel cancels the shutdown } diff --git a/tests/PlanViewer.Core.Tests/DetachedUnsavedChangesTests.cs b/tests/PlanViewer.Core.Tests/DetachedUnsavedChangesTests.cs new file mode 100644 index 00000000..9249419c --- /dev/null +++ b/tests/PlanViewer.Core.Tests/DetachedUnsavedChangesTests.cs @@ -0,0 +1,503 @@ +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Avalonia.Controls; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Helpers; + +namespace PlanViewer.Core.Tests; + +/// +/// #473: #462 gave query tabs an unsaved-changes prompt on tab close and on window close, and +/// a detached window dodged both. Its close path is the helper's own and asked nobody anything, +/// and while detached the session is out of MainTabControl.Items, so the shutdown walk honestly +/// reported nothing to save with a dirty edit sitting in another window. +/// +/// Same testing shape as UnsavedQueryChangesTests: the dialog cannot be clicked headlessly, +/// so the decision is a pure value — for +/// whether to ask at all, (#462, already pinned) for what to +/// do with the answer — and what is tested here is the walk and the wiring around them. +/// +/// Nothing here puts a PlanViewerControl inside a Window. Read-only content is represented +/// by a , which detaches through the same helper and takes +/// the same silent path a plan does. +/// +public class DetachedUnsavedChangesTests +{ + [Fact] + public void ADirtyDetachedSessionIsSeenWhereACleanOneIsNot() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + session = (QuerySessionControl)tab.Content!; + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + + detached = window.DetachTabToWindow(tab)!; + + /* The bug, stated as an assertion: the edit is no longer on the tab strip, which + is the only place #462 knew to look. */ + Assert.Empty(window.TabsWithUnsavedChanges()); + + Assert.Equal( + new[] { session }, + window.DetachedSessionsWithUnsavedChanges().Select(d => d.Session)); + + session.MarkClean(); + Assert.Empty(window.DetachedSessionsWithUnsavedChanges()); + Assert.Single(window.DetachedQuerySessions); // still detached, just not dirty + } + finally + { + PutAway(detached, session); + File.Delete(path); + } + }); + } + + [Fact] + public void TheShutdownPromptCountsDetachedSessionsAsWellAsTabs() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + session = (QuerySessionControl)tab.Content!; + + Assert.False(window.CloseNeedsConfirmation(), "nothing modified anywhere"); + + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + Assert.True(window.CloseNeedsConfirmation()); + + detached = window.DetachTabToWindow(tab)!; + + /* Detaching is not a way to opt out of being asked. Before this change the tab + walk was the whole question, so this went back to false the moment the window + was torn off and the app shut down over the top of the edit. */ + Assert.Empty(window.TabsWithUnsavedChanges()); + Assert.True(window.CloseNeedsConfirmation()); + + session.MarkClean(); + Assert.False(window.CloseNeedsConfirmation()); + } + finally + { + PutAway(detached, session); + File.Delete(path); + } + }); + } + + [Fact] + public void TheShutdownWalkAsksAboutTabsAndDetachedWindowsAlike() + { + HeadlessUi.Run(() => + { + var stays = TempSql("SELECT 1;"); + var leaves = TempSql("SELECT 2;"); + var clean = TempSql("SELECT 3;"); + Window? detached = null; + QuerySessionControl? torn = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(stays); + window.LoadSqlFile(leaves); + window.LoadSqlFile(clean); + + var tabs = window.MainTabControl.Items.OfType() + .Where(t => t.Content is QuerySessionControl).ToList(); + + var stayingTab = tabs[^3]; + var leavingTab = tabs[^2]; + var cleanTab = tabs[^1]; + + var staying = (QuerySessionControl)stayingTab.Content!; + torn = (QuerySessionControl)leavingTab.Content!; + + staying.QueryEditor.Text = "SELECT 1; -- edited"; + torn.QueryEditor.Text = "SELECT 2; -- edited"; + + detached = window.DetachTabToWindow(leavingTab)!; + + /* What the shutdown prompt will actually ask about, in the order it will ask. + Tabs first, then the windows that used to be tabs; the clean tab is in neither + list. Before this the second half of the walk did not exist, and an edit was + one Detach to Window away from being discarded without a question. */ + var work = window.UnsavedWorkOnClose(); + + Assert.Equal(new[] { staying, torn }, work.Select(w => w.Session)); + Assert.Equal(new TabItem?[] { stayingTab, null }, work.Select(w => w.Tab)); + + /* And the prompt each one gets is owned by the window it is about. A dialog + parented to the main window while the session it names is in another one is + a question about something the user cannot see. */ + Assert.Same(window, work[0].Owner); + Assert.Same(detached, work[1].Owner); + + Assert.DoesNotContain(cleanTab.Content, work.Select(w => (object?)w.Session)); + } + finally + { + PutAway(detached, torn); + File.Delete(stays); + File.Delete(leaves); + File.Delete(clean); + } + }); + } + + [Fact] + public void ReadOnlyDetachedContentIsNeverAskedAbout() + { + HeadlessUi.Run(() => + { + /* Plans and Query Store windows detach through the same helper and have nothing to + save. The guard has to answer no for them without a dialog and without cancelling + anything, or every read-only close grows a detour it has no use for. */ + Assert.False( + MainWindow.DetachedContentNeedsSavePrompt(new QueryStoreHistoryControl(), isShuttingDown: false)); + + var window = new MainWindow(); + Assert.Null(window.DetachedQueryCloseGuard(new QueryStoreHistoryControl(), window)); + }); + } + + [Fact] + public void OnlyADirtySessionIsAskedAboutAndNotOnceTheAppIsShuttingDown() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + var session = (QuerySessionControl)LastQueryTab(window).Content!; + + Assert.False( + MainWindow.DetachedContentNeedsSavePrompt(session, isShuttingDown: false), + "an unmodified session has nothing to lose"); + + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + + Assert.True( + MainWindow.DetachedContentNeedsSavePrompt(session, isShuttingDown: false)); + + /* The shutdown answer, and the reason it is a parameter rather than something the + guard reads for itself in a test. OnClosed force-closes every detached window + after the main window is already gone; a prompt raised there is owned by a + window nobody is looking at and gates a shutdown that has nowhere left to ask. + ConfirmWindowCloseAsync asks while everything is still up, which is what earns + this no. */ + Assert.False( + MainWindow.DetachedContentNeedsSavePrompt(session, isShuttingDown: true)); + } + finally + { + File.Delete(path); + } + }); + } + + [Fact] + public void RedockingDoesNotPrompt() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + session = (QuerySessionControl)tab.Content!; + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + + detached = window.DetachTabToWindow(tab)!; + + /* Re-dock moves the content, it does not destroy it, so there is nothing to save + it from — and the session is dirty, so a guard that did fire here would have + plenty to say. */ + RedockButton(detached).RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + /* The two assertions that catch a guard firing on this path. Re-dock hands the + content back whether or not the window agreed to close, so the tab coming back + proves nothing on its own: what a guard would leave behind is the emptied + window still open, with a prompt on it, asking about a session that is already + somewhere else. */ + Assert.False(detached.IsVisible, "Re-dock has to actually close the window"); + Assert.Empty(detached.OwnedWindows); + + Assert.Empty(window.DetachedQuerySessions); + + var redockedTab = LastQueryTab(window); + Assert.Same(session, redockedTab.Content); + Assert.True(MainWindow.HasUnsavedChanges(redockedTab), "the edit came back with it"); + } + finally + { + PutAway(detached, session); + File.Delete(path); + } + }); + } + + [Fact] + public void TheGuardCancelsACloseAndTheAnswerIsOnlyAskedForOnce() + { + HeadlessUi.Run(() => + { + var asked = 0; + var destroyed = 0; + var answer = false; + + var detached = DetachedWindowHelper.ShowDetached( + new TextBlock { Text = "content" }, + title: "Detached", + icon: null, + backgroundBrush: null, + onRedock: _ => { }, + onClosing: _ => destroyed++, + /* The cap is a fuse, not part of the contract: a helper that lost its latch + would re-ask its way round the post-and-close loop forever, and a test that + hangs tells you nothing. Four asks is already the failure. */ + closeGuard: (_, _) => { asked++; return Task.FromResult(answer && asked <= 3); }); + try + { + detached.Close(); + Dispatcher.UIThread.RunJobs(); + + Assert.Equal(1, asked); + Assert.Equal(0, destroyed); + Assert.True(detached.IsVisible, "Cancel has to actually keep the window open"); + + answer = true; + detached.Close(); + Dispatcher.UIThread.RunJobs(); + + /* Two closes, two questions. The re-issued close is not a third, and that is + the latch doing its job — without it the close the guard just authorised gets + handed straight back to the guard. */ + Assert.Equal(2, asked); + Assert.Equal(1, destroyed); + } + finally + { + answer = true; + PutAway(detached); + } + }); + } + + [Fact] + public void AGuardlessDetachIsUntouchedAndClosesOnTheFirstPass() + { + HeadlessUi.Run(() => + { + var destroyed = 0; + + /* The Query Store sub-tab detach passes no guard at all, so its close is the one it + always was: straight through, nothing cancelled, nothing posted. */ + var detached = DetachedWindowHelper.ShowDetached( + new QueryStoreHistoryControl(), + title: "History", + icon: null, + backgroundBrush: null, + onRedock: _ => { }, + onClosing: _ => destroyed++); + try + { + detached.Close(); + + Assert.Equal(1, destroyed); + } + finally + { + PutAway(detached); + } + }); + } + + [Fact] + public void ClosingADetachedWindowWithUnsavedWorkDoesNotJustCloseIt() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + session = (QuerySessionControl)tab.Content!; + session.QueryEditor.Text = "SELECT 2; -- unsaved work"; + + detached = window.DetachTabToWindow(tab)!; + + detached.Close(); + Dispatcher.UIThread.RunJobs(); + + /* The whole of #473 in one assertion. This used to be a closed window and a lost + edit; the close is now held while the prompt is up. Headless, nobody ever + answers it, so the window is still here — which is the right shape of failure + for a close that has a question outstanding. */ + Assert.True(detached.IsVisible); + Assert.Single(window.DetachedQuerySessions); + Assert.True(session.IsDirty, "nothing was written and nothing was discarded"); + } + finally + { + PutAway(detached, session); + File.Delete(path); + } + }); + } + + [Fact] + public void ADetachedSessionSavesWithNoTabToRetitle() + { + HeadlessUi.Run(() => + { + var opened = TempSql("SELECT 1;"); + var savedAs = Path.Combine(Path.GetTempPath(), $"saved_{Path.GetRandomFileName()}.sql"); + /* A directory that does not exist, so File.WriteAllText throws. */ + var unwritable = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName(), "nope.sql"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(opened); + + var tab = LastQueryTab(window); + session = (QuerySessionControl)tab.Content!; + session.QueryEditor.Text = "SELECT 2 AS edited;"; + + detached = window.DetachTabToWindow(tab)!; + + /* The save the prompt runs for a detached session has no tab behind it. The tab + is what SaveQueryToPath retitles, so it has to tolerate not having one — and + everything else about the save, including which side of the write settles the + dirty state, still has to hold. */ + Assert.False(window.SaveQueryToPath(null, session, unwritable)); + Assert.True(session.IsDirty, "a save that threw has not saved anything"); + Assert.Single(window.DetachedSessionsWithUnsavedChanges()); + + Assert.True(window.SaveQueryToPath(null, session, savedAs)); + Assert.False(session.IsDirty); + Assert.Equal("SELECT 2 AS edited;", File.ReadAllText(savedAs)); + Assert.Equal(savedAs, session.SourceFilePath); + Assert.Empty(window.DetachedSessionsWithUnsavedChanges()); + } + finally + { + PutAway(detached, session); + File.Delete(opened); + if (File.Exists(savedAs)) + File.Delete(savedAs); + } + }); + } + + [Fact] + public void ClosingADetachedWindowTakesTheSessionOffTheRegister() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1;"); + Window? detached = null; + QuerySessionControl? session = null; + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = LastQueryTab(window); + detached = window.DetachTabToWindow(tab)!; + Assert.Single(window.DetachedQuerySessions); + + /* Clean, so nothing is asked and the close goes through on the first pass. A + register that kept the entry would have the shutdown prompt asking about a + window that is not there any more. */ + detached.Close(); + + Assert.Empty(window.DetachedQuerySessions); + Assert.False(window.CloseNeedsConfirmation()); + } + finally + { + PutAway(detached, session); + File.Delete(path); + } + }); + } + + /// + /// Puts a detached window and any prompt it is showing away, from a finally, whatever the + /// test did or failed to do. + /// + /// One Avalonia application serves the whole assembly (see HeadlessUi), so a window + /// left open outlives the test that made it, and #474 is what that costs: a leaked window + /// poisons the session for everything queued behind it, and the failure surfaces somewhere + /// else entirely. Which makes cleanup a finally, not a last line — the run where it matters + /// is the run where an assertion above it failed. + /// + /// Deliberately not routed through the detached register: a test that has just broken + /// the register still has to be able to tidy up after itself. + /// + private static void PutAway(Window? detached, QuerySessionControl? session = null) + { + if (detached == null) + return; + + // Nothing headless can click one of the prompt's three buttons, so dismissing it is + // Cancel — which is why the session is settled before the window is closed again. + foreach (var prompt in detached.OwnedWindows.ToList()) + prompt.Close(); + + session?.MarkClean(); + detached.Close(); + Dispatcher.UIThread.RunJobs(); + } + + private static Button RedockButton(Window detached) => + ((DockPanel)detached.Content!).Children.OfType().Single() + .Children.OfType