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(); +}