diff --git a/CITATION.cff b/CITATION.cff index c8e61707..b5360578 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,8 +9,8 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.22.0" -date-released: "2026-08-31" +version: "1.23.0" +date-released: "2026-09-02" keywords: - sql-server - execution-plan diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 17a0f0af..32cb4c9e 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.22.0 + 1.23.0 Erik Darling Darling Data LLC Performance Studio 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/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs index daaaaf5c..99132efe 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs @@ -163,7 +163,49 @@ private void StatementsGrid_SelectionChanged(object? sender, SelectionChangedEve RenderStatement(row.Statement); } + private void StatementsContextMenu_Opening(object? sender, System.ComponentModel.CancelEventArgs e) + { + UpdateStatementMenuForSelection(); + } + + /// + /// Relabels the statement context menu for the selected statement, and shows the parameterized + /// variant only when there is a difference between the two forms to choose from (#467). + /// + /// Runs when the menu opens rather than when selection changes, so it describes whatever + /// is selected at the moment it is shown — the same row the two click handlers act on. + /// + private void UpdateStatementMenuForSelection() + { + var substitutable = StatementsGrid.SelectedItem is StatementRow row + && ParameterSubstitution.Apply(row.Statement.StatementText, row.Statement.Parameters) + .SubstitutionCount > 0; + + // Say so on the label rather than substituting silently: the text the plan records and the + // text that will run are different things, and which one you just copied matters. + CopyStatementTextItem.Header = substitutable ? "Copy Query Text (with values)" : "Copy Query Text"; + OpenStatementInEditorItem.Header = substitutable + ? "Open in Query Editor (with values)" + : "Open in Query Editor"; + + ParameterizedStatementSeparator.IsVisible = substitutable; + CopyParameterizedStatementTextItem.IsVisible = substitutable; + } + private async void CopyStatementText_Click(object? sender, RoutedEventArgs e) + { + if (StatementsGrid.SelectedItem is not StatementRow row) return; + var text = RunnableStatementText(row.Statement); + if (string.IsNullOrEmpty(text)) return; + + await ClipboardHelper.TrySetTextAsync(this, text); + } + + /// + /// Copies the statement exactly as the plan records it, parameter names and all. Reachable only + /// when that differs from the substituted form. + /// + private async void CopyParameterizedStatementText_Click(object? sender, RoutedEventArgs e) { if (StatementsGrid.SelectedItem is not StatementRow row) return; var text = row.Statement.StatementText; @@ -175,12 +217,23 @@ private async void CopyStatementText_Click(object? sender, RoutedEventArgs e) private void OpenInEditor_Click(object? sender, RoutedEventArgs e) { if (StatementsGrid.SelectedItem is not StatementRow row) return; - var text = row.Statement.StatementText; + var text = RunnableStatementText(row.Statement); if (string.IsNullOrEmpty(text)) return; OpenInEditorRequested?.Invoke(this, text); } + /// + /// The statement text with the plan's parameter values put back, or the text as-is when the plan + /// carries no values to put back. + /// + /// The plan is the only source used. The query editor's buffer holds the original text in + /// exactly one case — the user just executed it from that tab — and is wrong for a plan opened + /// from a file, from Query Store, or from another session. + /// + private static string RunnableStatementText(PlanStatement statement) => + ParameterSubstitution.Apply(statement.StatementText, statement.Parameters).Text; + private static void CollectNodeWarnings(PlanNode node, List warnings) { warnings.AddRange(node.Warnings); diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml b/src/PlanViewer.App/Controls/PlanViewerControl.axaml index 56de3c7f..b0a7c35c 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml @@ -241,9 +241,16 @@ Background="{DynamicResource BackgroundDarkBrush}" BorderThickness="0"> - - - + + + + + diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index ed97937f..b35d66ad 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -164,8 +164,10 @@ public PlanViewerControl() _zoomTransform = (ScaleTransform)layoutTransform.LayoutTransform!; Helpers.DataGridBehaviors.Attach(StatementsGrid); + // Same text the Copy Query Text menu entry produces (#467) — Ctrl+C is that entry's + // unlabelled twin, and handing the two of them different statements is its own bug report. Helpers.DataGridBehaviors.AttachCopyGuard(StatementsGrid, - item => item is StatementRow row ? row.Statement.StatementText : null); + item => item is StatementRow row ? RunnableStatementText(row.Statement) : null); // Wire minimap resize grip (defined in AXAML, not in canvas) MinimapResizeGrip.PointerPressed += MinimapResizeGrip_PointerPressed; 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/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/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/Helpers/DetachedWindowHelper.cs b/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs index 180b1d6e..a613a63b 100644 --- a/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs +++ b/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs @@ -1,7 +1,9 @@ using System; +using System.Threading.Tasks; using Avalonia.Controls; using Avalonia.Layout; using Avalonia.Media; +using Avalonia.Threading; namespace PlanViewer.App.Helpers; @@ -20,6 +22,17 @@ internal static class DetachedWindowHelper /// Window background brush. /// Called when the user clicks Re-dock. Content has already been removed from the wrapper. /// Called when the window is closing (before destroy). Use to cancel fetches etc. + /// + /// Asked, synchronously, whether this close has a question attached to it (#473). + /// + /// Returning null closes on the spot with nothing asked and nothing delayed — that is + /// read-only content, an unmodified query, and app shutdown, so the path a plan or a Query + /// Store window takes is the one it always took. Returning a task cancels the close until + /// that task answers: true re-issues it, false leaves the window open. + /// + /// Re-dock never consults it. The content is being moved, not destroyed, so there is + /// nothing to save it from. + /// /// The created Window instance. public static Window ShowDetached( Control content, @@ -27,7 +40,8 @@ public static Window ShowDetached( WindowIcon? icon, Avalonia.Media.IBrush? backgroundBrush, Action onRedock, - Action? onClosing = null) + Action? onClosing = null, + Func?>? closeGuard = null) { var redockBtn = new Button { @@ -70,6 +84,9 @@ public static Window ShowDetached( bool redocked = false; + // Set once the guard has answered, so the re-issued close is not questioned again. + bool closeConfirmed = false; + redockBtn.Click += (_, _) => { if (redocked) return; @@ -81,10 +98,41 @@ public static Window ShowDetached( onRedock(content); }; - detachedWindow.Closing += (_, _) => + // Avalonia's Closing is synchronous and an unsaved-changes prompt is not, so the first + // pass cancels the close outright and re-issues it once the question has an answer. + // Same cancel-then-reissue MainWindow.OnClosing uses (#462); closeConfirmed is the + // latch that stops the second pass asking all over again. + async Task ReissueCloseIfConfirmed(Task pending) { - if (!redocked) - onClosing?.Invoke(content); + if (!await pending) + return; + + closeConfirmed = true; + + // Posted rather than called: a guard that answers without ever actually waiting + // would otherwise land Close() in the middle of the Closing handler that called + // it. Safe to post because the guard returns null during app shutdown, so nothing + // is ever queued against a dispatcher that is going away. + Dispatcher.UIThread.Post(detachedWindow.Close); + } + + detachedWindow.Closing += (_, e) => + { + if (redocked) + return; + + if (!closeConfirmed && closeGuard != null) + { + var pending = closeGuard(content, detachedWindow); + if (pending != null) + { + e.Cancel = true; + _ = ReissueCloseIfConfirmed(pending); + return; + } + } + + onClosing?.Invoke(content); }; detachedWindow.Show(); diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 99c56d6e..78b5d2bd 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -112,6 +112,25 @@ 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. + /// + /// #473 brings two wrinkles, both from sessions that are no longer tabs. A detached + /// session has no to retitle, hence the null. And the picker has to + /// come off the window doing the asking: here is the + /// main window's, so a detached window closing with unsaved work would put its Save As + /// dialog on a window behind the one the user is looking at — and, at shutdown, on one + /// that is closing. is how the caller says which window. + /// + /// Whether the query reached disk. False also covers the user closing the picker. + private async Task SaveQueryAsync(TabItem? tab, QuerySessionControl session, IStorageProvider? storage = null) + { + var picker = storage ?? StorageProvider; var existing = session.SourceFilePath; var options = new FilePickerSaveOptions @@ -133,27 +152,36 @@ private async Task SaveQueryAsync() { 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(); - if (path != null) - SaveQueryToPath(tab, session, path); + return path != null && SaveQueryToPath(tab, session, path); } /// /// 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 { File.WriteAllText(path, session.QueryEditor.Text); session.SourceFilePath = path; - SetTabLabel(tab, Path.GetFileName(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(); + + if (tab != null) + SetTabLabel(tab, Path.GetFileName(path)); + return true; } catch (Exception ex) @@ -243,6 +271,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); @@ -384,11 +414,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 +431,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..8871dc8a 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; } }; @@ -89,8 +105,8 @@ private TabItem CreateTab(string label, Control content) // Right-click context menu var copyPathItem = new MenuItem { Header = "Copy Path", Tag = tab }; // Only visible when tab content has a file path - var filePath = GetTabFilePath(tab); - copyPathItem.IsVisible = filePath != null; + void RefreshCopyPathVisibility() => copyPathItem.IsVisible = GetTabFilePath(tab) != null; + RefreshCopyPathVisibility(); var contextMenu = new ContextMenu { @@ -107,6 +123,12 @@ private TabItem CreateTab(string label, Control content) } }; + /* #472: whether there is a path to copy is not a fact about the tab's birth. A scratch + query gains one the moment it is saved, and a tab can lose one. The menu is only + consulted when it opens, so that is when the question gets asked — the call above is + just the answer for a menu nobody has opened yet. */ + contextMenu.Opening += (_, _) => RefreshCopyPathVisibility(); + foreach (var item in contextMenu.Items.OfType()) item.Click += TabContextMenu_Click; @@ -118,10 +140,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 +167,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": @@ -187,6 +196,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 +216,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; } @@ -266,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); @@ -282,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); @@ -299,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 154cd728..466574c1 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,297 @@ 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(); + + // ── 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. + /// + 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. + /// + /// #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 && CloseNeedsConfirmation()) + { + e.Cancel = true; + _ = ConfirmWindowCloseAsync(); + return; + } + + base.OnClosing(e); + } + + private async Task ConfirmWindowCloseAsync() + { + /* #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()) + { + var mayClose = work.Tab != null + ? await ConfirmCloseAsync(work.Tab) + : await ConfirmDetachedCloseAsync(work.Session, work.Owner); + + if (!mayClose) + return; // one Cancel cancels the shutdown + } + + _closeConfirmed = true; + Close(); + } + + private void Exit_Click(object? sender, RoutedEventArgs e) { Close(); @@ -351,6 +641,16 @@ internal void RefreshComparePlanAvailability() // ── Recent Plans & Session Restore ──────────────────────────────────── + /// + /// One modal for everything that goes wrong outside a file operation — a plan that is not + /// a plan, a clipboard with nothing in it, advice that could not be built. + /// + /// Shown ownerless until this window is visible, for the same reason ShowFileError is. + /// Both the command-line open and the restore of the previous session's tabs run from the + /// constructor, so a plan file that fails XML validation at startup reaches here before there + /// is anything to be modal over, and ShowDialog against a window that has not been shown + /// throws rather than reporting the problem it was called about. + /// private void ShowError(string message) { var dialog = new Window @@ -377,7 +677,11 @@ private void ShowError(string message) } } }; - dialog.ShowDialog(this); + + if (IsVisible) + dialog.ShowDialog(this); + else + dialog.Show(); } private async Task CheckForUpdatesOnStartupAsync() 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/src/PlanViewer.Core/Services/ParameterSubstitution.cs b/src/PlanViewer.Core/Services/ParameterSubstitution.cs new file mode 100644 index 00000000..1428e2e7 --- /dev/null +++ b/src/PlanViewer.Core/Services/ParameterSubstitution.cs @@ -0,0 +1,292 @@ +using System; +using System.Collections.Generic; +using System.Text; +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// Puts the plan's captured parameter values back into a statement's text (#467). +/// +/// Why this exists. The statement text in a plan is what SQL Server compiled, not what +/// anyone typed. Under PARAMETERIZATION FORCED the engine rewrites literals into @0, +/// @1 … before compiling, so the plan honestly records a statement nobody can run — every +/// parameter is undeclared. The same shape shows up for stored procedure parameters and for +/// sp_executesql. In all three cases the values are sitting in the plan's +/// ParameterList, which the parser already reads into +/// ; nothing was putting them back. +/// +/// Values arrive pre-quoted. A string is '123456' and stays that way. A number +/// is (5) and loses its wrapper, because (5) pasted into an IN list is not +/// what anyone means. A uniqueidentifier is {guid'…'}, which is showplan notation and not +/// T-SQL at all. +/// +public static class ParameterSubstitution +{ + /// + /// Rewrites with every parameter that has a value in + /// replaced by that value. + /// + /// Runtime values win over compiled values: the compiled value is what the plan was built + /// for, the runtime value is what the execution being looked at actually passed, and the point + /// of copying a statement out is to run the case in front of you. + /// + /// Parameters in the list that never appear in the text are ignored, and names in the text + /// with no value in the list are left alone — a plan with OPTION(RECOMPILE) or local + /// variables carries names without values, and inventing something there would be worse than + /// leaving the name visible. + /// + public static ParameterSubstitutionResult Apply( + string? statementText, + IReadOnlyList? parameters) + { + if (string.IsNullOrEmpty(statementText)) + return new ParameterSubstitutionResult(statementText ?? "", 0); + + var values = BuildValueLookup(parameters); + if (values.Count == 0) + return new ParameterSubstitutionResult(statementText, 0); + + var sb = new StringBuilder(statementText.Length); + var substitutions = 0; + var i = 0; + + while (i < statementText.Length) + { + var c = statementText[i]; + + /* Regions where an @name is text, not a parameter. A string literal is the case that + matters in practice — LIKE 'kexin%' sits right next to the parameters in the #466 + repro — but a delimited identifier can hold anything, and a comment is not code. */ + if (c == '\'' || c == '"') + { + i = CopyDelimited(statementText, i, c, c, sb); + continue; + } + + if (c == '[') + { + i = CopyDelimited(statementText, i, '[', ']', sb); + continue; + } + + if (c == '-' && i + 1 < statementText.Length && statementText[i + 1] == '-') + { + i = CopyLineComment(statementText, i, sb); + continue; + } + + if (c == '/' && i + 1 < statementText.Length && statementText[i + 1] == '*') + { + i = CopyBlockComment(statementText, i, sb); + continue; + } + + /* A whole token, or nothing. "@1" inside "@11" is a different parameter, and "@0" at the + tail of an identifier such as "t@0" is part of that identifier. The scan below claims + the longest run of identifier characters, which handles the first; the preceding + character is checked here, which handles the second. */ + if (c == '@' && !IsIdentifierPart(i > 0 ? statementText[i - 1] : '\0')) + { + var end = i + 1; + while (end < statementText.Length && IsIdentifierPart(statementText[end])) + end++; + + var token = statementText[i..end]; + if (values.TryGetValue(token, out var value)) + { + sb.Append(value); + substitutions++; + } + else + { + sb.Append(token); + } + + i = end; + continue; + } + + sb.Append(c); + i++; + } + + return substitutions == 0 + ? new ParameterSubstitutionResult(statementText, 0) + : new ParameterSubstitutionResult(sb.ToString(), substitutions); + } + + /// + /// Maps parameter name to the literal that should replace it, skipping parameters with no + /// captured value at all. + /// + private static Dictionary BuildValueLookup(IReadOnlyList? parameters) + { + var values = new Dictionary(StringComparer.OrdinalIgnoreCase); + if (parameters == null) + return values; + + foreach (var param in parameters) + { + if (string.IsNullOrEmpty(param.Name)) + continue; + + var raw = !string.IsNullOrEmpty(param.RuntimeValue) + ? param.RuntimeValue + : param.CompiledValue; + + if (string.IsNullOrEmpty(raw)) + continue; + + /* First name wins. Duplicates in a ParameterList are not expected, but silently taking + the last one would make the result depend on document order for no reason. */ + values.TryAdd(param.Name, StripGuidWrapper(StripOuterParentheses(raw))); + } + + return values; + } + + /// + /// Copies a delimited region — string literal, quoted identifier, or bracketed identifier — + /// verbatim, and returns the index just past it. Doubled delimiters escape, so 'it''s' + /// is one literal. An unterminated region runs to the end of the text, which is what SQL Server + /// would do with it and what a truncated statement text looks like. + /// + private static int CopyDelimited(string text, int start, char open, char close, StringBuilder sb) + { + sb.Append(open); + var i = start + 1; + + while (i < text.Length) + { + if (text[i] == close) + { + if (i + 1 < text.Length && text[i + 1] == close) + { + sb.Append(close).Append(close); + i += 2; + continue; + } + + sb.Append(close); + return i + 1; + } + + sb.Append(text[i]); + i++; + } + + return i; + } + + private static int CopyLineComment(string text, int start, StringBuilder sb) + { + var i = start; + while (i < text.Length && text[i] != '\n') + { + sb.Append(text[i]); + i++; + } + return i; + } + + /// + /// Copies a block comment verbatim. T-SQL nests these, so the depth is tracked rather than + /// stopping at the first */. + /// + private static int CopyBlockComment(string text, int start, StringBuilder sb) + { + var depth = 0; + var i = start; + + while (i < text.Length) + { + if (i + 1 < text.Length && text[i] == '/' && text[i + 1] == '*') + { + depth++; + sb.Append("/*"); + i += 2; + continue; + } + + if (i + 1 < text.Length && text[i] == '*' && text[i + 1] == '/') + { + depth--; + sb.Append("*/"); + i += 2; + if (depth == 0) + return i; + continue; + } + + sb.Append(text[i]); + i++; + } + + return i; + } + + /// + /// T-SQL identifier body characters. @ is one of them, which is why @@ROWCOUNT + /// reads as a single token and never matches a parameter named @ROWCOUNT. + /// + private static bool IsIdentifierPart(char c) => + char.IsLetterOrDigit(c) || c == '_' || c == '@' || c == '#' || c == '$'; + + /// + /// Unwraps a showplan-parenthesized value: (5) becomes 5. + /// + /// Only one level comes off, and only when the opening parenthesis is closed by the very + /// last character. A value whose outer parentheses are two separate pairs is not a wrapper, and + /// slicing the ends off it would produce text that no longer parses. + /// + private static string StripOuterParentheses(string value) + { + if (value.Length < 2 || value[0] != '(' || value[^1] != ')') + return value; + + var depth = 0; + for (var i = 0; i < value.Length; i++) + { + if (value[i] == '(') + { + depth++; + } + else if (value[i] == ')') + { + depth--; + if (depth == 0) + return i == value.Length - 1 ? value[1..^1] : value; + } + } + + return value; + } + + /// + /// Unwraps showplan's uniqueidentifier notation: {guid'AB12…'} becomes 'AB12…'. + /// The braces are showplan's, not T-SQL's, and would be a syntax error if pasted. + /// + private static string StripGuidWrapper(string value) + { + if (value.StartsWith("{guid'", StringComparison.OrdinalIgnoreCase) + && value.EndsWith("'}", StringComparison.Ordinal)) + { + return "'" + value[6..^2] + "'"; + } + + return value; + } +} + +/// +/// The rewritten statement text and how many parameter references were actually replaced. +/// +/// +/// The statement with values substituted, or the original text unchanged when nothing was replaced. +/// +/// +/// Replacements made. Zero means the statement gains nothing from being offered in substituted form, +/// which is how the UI decides whether the menu entry is worth showing. +/// +public sealed record ParameterSubstitutionResult(string Text, int SubstitutionCount); diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index 9170ad8f..a38cc167 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.22.0.0")] -[assembly: AssemblyFileVersion("1.22.0.0")] +[assembly: AssemblyVersion("1.23.0.0")] +[assembly: AssemblyFileVersion("1.23.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index d6b707c9..15b50bca 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS diff --git a/tests/PlanViewer.Core.Tests/CopyPathVisibilityTests.cs b/tests/PlanViewer.Core.Tests/CopyPathVisibilityTests.cs new file mode 100644 index 00000000..5f628df4 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/CopyPathVisibilityTests.cs @@ -0,0 +1,146 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using Avalonia.Controls; +using Avalonia.Input; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #472: Copy Path answered the question "is there a path to copy?" once, while the tab was +/// being built, and never again. Open a scratch query and save it — the session gains a +/// SourceFilePath, the tab is retitled after the file, and the menu item is still hidden, because +/// it was told there was nothing to copy back when that was true. Only a restart, which rebuilds +/// the tab, put it right. +/// +/// The fix asks on , the one moment the menu is actually consulted, +/// so these drive that event rather than reading the flag straight after construction — reading it +/// cold is what the old code got right and is exactly what these have to not do. +/// +/// How the menu is opened here. Avalonia raises ContextMenu.Opening from its +/// ContextRequested handler on the attached control, not from ContextMenu.Open(): calling Open() +/// directly sets IsOpen and never asks. So raises ContextRequestedEvent on +/// the tab header, which is the same path a real right-click takes. +/// +public class CopyPathVisibilityTests +{ + /// + /// The report, end to end: scratch query, no path, save, right-click, and the item is there + /// and copies the file it was saved to. + /// + [Fact] + public void CopyPathAppearsOnceAScratchQueryHasBeenSaved() + { + HeadlessUi.Run(() => + { + var savedAs = Path.Combine(Path.GetTempPath(), $"saved_{Path.GetRandomFileName()}.sql"); + try + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var tab = QueryTabs(window).Last(); + var session = (QuerySessionControl)tab.Content!; + var copyPath = ContextMenuItem(tab, "Copy Path"); + + RightClick(tab); + Assert.False(copyPath.IsVisible, + "a never-saved scratch query has no file, so there is nothing to copy"); + + session.QueryEditor.Text = "SELECT 1 AS saved;"; + Assert.True(window.SaveQueryToPath(tab, session, savedAs)); + + RightClick(tab); + Assert.True(copyPath.IsVisible, + "the query has a file now, and this menu is being opened after the save"); + + copyPath.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + Assert.Equal(savedAs, Pump(ClipboardHelper.TryGetTextAsync(window))); + } + finally + { + if (File.Exists(savedAs)) + File.Delete(savedAs); + } + }); + } + + /// + /// The other direction, which recomputing gets for free: a tab that stops having a file stops + /// offering to copy one. A stale true would put a Copy Path on the menu that copies + /// nothing when clicked. + /// + [Fact] + public void CopyPathGoesAwayAgainWhenTheTabStopsHavingAFile() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS opened;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = QueryTabs(window).Last(); + var copyPath = ContextMenuItem(tab, "Copy Path"); + + RightClick(tab); + Assert.True(copyPath.IsVisible, "the query was opened from a file"); + + ((QuerySessionControl)tab.Content!).SourceFilePath = null; + + RightClick(tab); + Assert.False(copyPath.IsVisible, "there is no longer a path behind this tab"); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// Raises the event a real right-click raises on the tab header, then closes the menu again so + /// the next call is a fresh open rather than a no-op on an already-open menu. + /// + private static void RightClick(TabItem tab) + { + var header = (StackPanel)tab.Header!; + header.RaiseEvent(new ContextRequestedEventArgs()); + header.ContextMenu!.Close(); + } + + 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 without + /// awaiting it, so the read that follows can be a dispatcher turn early. + /// + 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 QueryTabs(MainWindow window) => + window.MainTabControl.Items.OfType().Where(t => t.Content is QuerySessionControl); +} 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 internal static class HeadlessUi { @@ -38,10 +45,93 @@ internal static class HeadlessUi /// internal static void Run(Action body) { + var failure = Dispatch(body); + var sessionBroken = Dispatch(EnsureSessionSurvived); + + /* The body's own failure wins. A test that both failed its assertion and left the session + unusable is still described best by the assertion it failed. */ + if (failure is not null) + { + ExceptionDispatchInfo.Capture(failure).Throw(); + } + + if (sessionBroken is not null) + { + ExceptionDispatchInfo.Capture(sessionBroken).Throw(); + } + } + + /// + /// Runs one body on the UI thread and hands back whatever it threw rather than throwing here, + /// so can decide which of two failures to report. + /// + /// Why the queue is drained before returning (#474). Avalonia's per-dispatch + /// teardown disposes the session's FontManager and only then calls + /// Dispatcher.ResetForUnitTests, which executes whatever is still queued. A window whose + /// content is involved enough to leave a deferred render pass behind — a + /// PlanViewerControl reliably does, a TextBlock does not — therefore renders text against + /// a font manager that has just been disposed, throws KeyNotFoundException for + /// fonts:SystemFonts, and that exception escapes the teardown delegate before it reaches + /// scope.Dispose(). The locator scope is then never popped: every later dispatch nests + /// inside the leaked one, resolves the disposed font manager through its parent chain, and dies + /// constructing any at all. The guilty test passes, because the throw + /// happens after its result has been recorded. + /// + /// Draining here leaves the teardown nothing to run, which is the whole fix. It is done + /// even when the body failed, because a failing test is no less capable of poisoning the + /// session than a passing one. + /// + private static Exception? Dispatch(Action body) + { + Exception? failure = null; + Session.Value.Dispatch(() => { - body(); + try + { + body(); + } + catch (Exception ex) + { + failure = ex; + } + + try + { + Dispatcher.UIThread.RunJobs(); + } + catch (Exception ex) + { + failure ??= ex; + } + return Task.CompletedTask; }, default).GetAwaiter().GetResult(); + + return failure; + } + + /// + /// Checks that the session a test just used is still usable by the next one, so a test that + /// breaks it fails saying so instead of leaving a trail of unrelated red. + /// + /// Constructing a is the check because it is the symptom: a window + /// builds a compositor, which asks the font manager for a typeface before it does anything + /// else. Nothing is shown and nothing is laid out, so this costs one object. + /// + private static void EnsureSessionSurvived() + { + try + { + _ = new Window(); + } + catch (Exception ex) + { + throw new InvalidOperationException( + "This test left the shared Avalonia session unusable — a bare Window can no longer " + + "be constructed, so every UI test that runs after it will fail too, on something " + + "that is not their fault. See #474.", + ex); + } } } diff --git a/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs b/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs new file mode 100644 index 00000000..ce41d892 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs @@ -0,0 +1,242 @@ +using System.Collections.Generic; +using PlanViewer.Core.Models; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #467: the statement text in a plan is what SQL Server compiled, which under +/// PARAMETERIZATION FORCED means every literal has already been replaced by @0, +/// @1 … The values are in the plan's ParameterList, and these cover putting them back. +/// +/// The token boundary is the part worth being paranoid about. @1 is a prefix of +/// @11, so a naive replace corrupts the statement instead of fixing it, and an @0 +/// inside a string literal is data that must survive untouched. +/// +public class ParameterSubstitutionTests +{ + private static PlanParameter Param(string name, string? compiled, string? runtime = null) => + new() { Name = name, DataType = "int", CompiledValue = compiled, RuntimeValue = runtime }; + + [Fact] + public void NumericValue_LosesItsParentheses() + { + var result = ParameterSubstitution.Apply( + "WHERE [t].[StatusId] = @1", + new List { Param("@1", "(5)") }); + + Assert.Equal("WHERE [t].[StatusId] = 5", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void StringValue_KeepsItsQuotes() + { + var result = ParameterSubstitution.Apply( + "WHERE [t0].[AuthUserId] = @0", + new List { Param("@0", "'123456'") }); + + Assert.Equal("WHERE [t0].[AuthUserId] = '123456'", result.Text); + } + + [Fact] + public void ShorterNameIsNotSubstitutedInsideALongerOne() + { + /* The corruption a naive Replace("@1", "5") produces: "@11" becomes "51". */ + var result = ParameterSubstitution.Apply( + "WHERE a = @1 AND b = @11", + new List { Param("@1", "(5)"), Param("@11", "(99)") }); + + Assert.Equal("WHERE a = 5 AND b = 99", result.Text); + Assert.Equal(2, result.SubstitutionCount); + } + + [Fact] + public void ShorterNameWithNoLongerCounterpartStillLeavesTheLongerNameAlone() + { + var result = ParameterSubstitution.Apply( + "WHERE a = @1 AND b = @11", + new List { Param("@1", "(5)") }); + + Assert.Equal("WHERE a = 5 AND b = @11", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void NameInsideAStringLiteral_IsLeftAlone() + { + var result = ParameterSubstitution.Apply( + "WHERE a = @0 AND note = 'sent to @0 by hand'", + new List { Param("@0", "'x'") }); + + Assert.Equal("WHERE a = 'x' AND note = 'sent to @0 by hand'", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void NameInsideAStringLiteralWithDoubledQuotes_IsStillLeftAlone() + { + /* The doubled quote does not end the literal, so @0 after it is still inside one. */ + var result = ParameterSubstitution.Apply( + "WHERE note = 'it''s @0 already' AND a = @0", + new List { Param("@0", "(7)") }); + + Assert.Equal("WHERE note = 'it''s @0 already' AND a = 7", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void NameInsideADelimitedIdentifierOrComment_IsLeftAlone() + { + var result = ParameterSubstitution.Apply( + "SELECT [@0] /* not @0 either */ FROM t -- and not @0\nWHERE a = @0", + new List { Param("@0", "(1)") }); + + Assert.Equal( + "SELECT [@0] /* not @0 either */ FROM t -- and not @0\nWHERE a = 1", + result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void NameAtTheTailOfAnIdentifier_IsNotAParameterReference() + { + /* "t@0" is one identifier. Substituting the tail of it produces nonsense. */ + var result = ParameterSubstitution.Apply( + "SELECT t@0 FROM x WHERE a = @0", + new List { Param("@0", "(1)") }); + + Assert.Equal("SELECT t@0 FROM x WHERE a = 1", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void GlobalVariable_IsNotMistakenForAParameter() + { + /* @@ROWCOUNT reads as one token, so a parameter named @ROWCOUNT cannot land inside it. */ + var result = ParameterSubstitution.Apply( + "SELECT @@ROWCOUNT, @ROWCOUNT", + new List { Param("@ROWCOUNT", "(3)") }); + + Assert.Equal("SELECT @@ROWCOUNT, 3", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void NullValue_IsSubstituted() + { + var result = ParameterSubstitution.Apply( + "WHERE a = @p", + new List { Param("@p", "NULL") }); + + Assert.Equal("WHERE a = NULL", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void RuntimeValueWins_WhenBothArePresent() + { + /* Compiled is what the plan was built for; runtime is what this execution passed, and the + point of copying the statement out is to run the case in front of you. */ + var result = ParameterSubstitution.Apply( + "WHERE a = @p", + new List { Param("@p", "(1)", "(2)") }); + + Assert.Equal("WHERE a = 2", result.Text); + } + + [Fact] + public void CompiledValueIsUsed_WhenRuntimeIsMissing() + { + /* @6 in the #466 repro has a compiled value and no runtime value. */ + var result = ParameterSubstitution.Apply( + "WHERE a >= @6", + new List { Param("@6", "'2026-05-28 10:28:07.3132561'") }); + + Assert.Equal("WHERE a >= '2026-05-28 10:28:07.3132561'", result.Text); + } + + [Fact] + public void ParameterWithNoValueAtAll_IsLeftAlone() + { + /* OPTION(RECOMPILE) and local variables produce names without values. Inventing one would + be worse than leaving the name where the user can see it. */ + var result = ParameterSubstitution.Apply( + "WHERE a = @known AND b = @unknown", + new List { Param("@known", "(1)"), Param("@unknown", null) }); + + Assert.Equal("WHERE a = 1 AND b = @unknown", result.Text); + Assert.Equal(1, result.SubstitutionCount); + } + + [Fact] + public void ParameterInTheListButNotInTheText_ChangesNothing() + { + var result = ParameterSubstitution.Apply( + "SELECT 1", + new List { Param("@unused", "(1)") }); + + Assert.Equal("SELECT 1", result.Text); + Assert.Equal(0, result.SubstitutionCount); + } + + [Fact] + public void MissingInputs_AreHandledRatherThanThrown() + { + /* Both nulls are reachable: a plan can carry a statement with no text at all, and callers + hand over whatever the parser produced. */ + Assert.Equal("", ParameterSubstitution.Apply(null, new List()).Text); + Assert.Equal("SELECT 1", ParameterSubstitution.Apply("SELECT 1", null).Text); + Assert.Equal("SELECT 1", ParameterSubstitution.Apply("SELECT 1", new List()).Text); + } + + [Fact] + public void GuidValue_LosesShowplansBraces() + { + /* {guid'...'} is showplan notation, not T-SQL, and is a syntax error if pasted. */ + var result = ParameterSubstitution.Apply( + "WHERE id = @g", + new List + { + Param("@g", "{guid'6F9619FF-8B86-D011-B42D-00C04FC964FF'}") + }); + + Assert.Equal("WHERE id = '6F9619FF-8B86-D011-B42D-00C04FC964FF'", result.Text); + } + + [Fact] + public void ValueWhoseParenthesesAreNotAWrapper_KeepsThem() + { + /* "(a)+(b)" starts and ends with a parenthesis but is not wrapped in one pair; slicing the + ends off produces text that no longer parses. */ + var result = ParameterSubstitution.Apply( + "WHERE a = @p", + new List { Param("@p", "(a)+(b)") }); + + Assert.Equal("WHERE a = (a)+(b)", result.Text); + } + + [Fact] + public void ForcedParameterizationPlan_GetsItsLiteralsBack() + { + /* The #466 reproduction, end to end from the plan file: seven parameters the engine + manufactured, none of them declared anywhere, so the copied statement does not run. */ + var plan = PlanTestHelper.LoadAndAnalyze("forced_parameterization_plan.sqlplan"); + var statement = PlanTestHelper.FirstStatement(plan); + + Assert.Equal(7, statement.Parameters.Count); + Assert.Contains("@0", statement.StatementText); + + var result = ParameterSubstitution.Apply(statement.StatementText, statement.Parameters); + + Assert.Equal(7, result.SubstitutionCount); + Assert.Contains("[t0].[AuthUserId]='123456'", result.Text); + Assert.Contains("[t].[StatusId] in (5,6,7,8,9)", result.Text); + Assert.Contains("[t].[FromDateTime]>='2026-05-28 10:28:07.3132561'", result.Text); + + /* The literal that was never parameterized is still a literal, and no manufactured + parameter name survives anywhere in the text. */ + Assert.Contains("like 'kexin%'", result.Text); + Assert.DoesNotContain("@", result.Text); + } +} diff --git a/tests/PlanViewer.Core.Tests/Plans/forced_parameterization_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/forced_parameterization_plan.sqlplan new file mode 100644 index 00000000..3766d1f0 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/forced_parameterization_plan.sqlplan @@ -0,0 +1 @@ + \ No newline at end of file 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(); +} 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(); + } +} diff --git a/tests/PlanViewer.Core.Tests/ShowErrorBeforeVisibleTests.cs b/tests/PlanViewer.Core.Tests/ShowErrorBeforeVisibleTests.cs new file mode 100644 index 00000000..df00a0aa --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ShowErrorBeforeVisibleTests.cs @@ -0,0 +1,73 @@ +using System.IO; +using PlanViewer.App; + +namespace PlanViewer.Core.Tests; + +/// +/// #471: the same startup crash #459 cured in ShowFileError, still living in ShowError. +/// +/// Both the command-line open and the restore of the previous session's tabs run from +/// the MainWindow constructor, before the window has been shown. A plan file that fails XML +/// validation on that path reaches ShowError with nothing to be modal over, and ShowDialog +/// against a window that has not been shown throws InvalidOperationException: "Cannot show +/// window with non-visible owner" — so the app dies at launch instead of reporting the file +/// it was called about. +/// +/// These drive LoadPlanFile against a never-shown window, which is exactly the startup +/// shape, and assert only that nothing escapes. What the dialog says is not the point; that +/// it can be raised at all is. +/// +public class ShowErrorBeforeVisibleTests +{ + [Fact] + public void AMalformedPlanFileIsReportedRatherThanThrownAtStartup() + { + HeadlessUi.Run(() => + { + /* A .sql routed through LoadPlanFile is how this reproduced during #463's red run: + XDocument.Parse throws, ValidatePlanXml calls ShowError. */ + var path = TempPlan("SELECT 1 AS not_a_plan;"); + try + { + var window = new MainWindow(); + Assert.False(window.IsVisible); + + Assert.Null(Record.Exception(() => window.LoadPlanFile(path))); + } + finally + { + File.Delete(path); + } + }); + } + + [Fact] + public void WellFormedXmlThatIsNotAPlanIsAlsoReportedRatherThanThrown() + { + HeadlessUi.Run(() => + { + /* The other arm of ValidatePlanXml: parses cleanly, no ShowPlanXML anywhere in it. + A corrupt or truncated file can land on either arm, so both have to survive + being reported before the window exists. */ + var path = TempPlan(""); + try + { + var window = new MainWindow(); + Assert.False(window.IsVisible); + + Assert.Null(Record.Exception(() => window.LoadPlanFile(path))); + } + finally + { + File.Delete(path); + } + }); + } + + private static string TempPlan(string text) + { + var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sqlplan"); + File.WriteAllText(path, text); + return path; + } +} diff --git a/tests/PlanViewer.Core.Tests/StatementParameterMenuTests.cs b/tests/PlanViewer.Core.Tests/StatementParameterMenuTests.cs new file mode 100644 index 00000000..d98dcf08 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/StatementParameterMenuTests.cs @@ -0,0 +1,110 @@ +using System.IO; +using System.Linq; +using Avalonia.Controls; +using Avalonia.Input; +using Avalonia.Interactivity; +using Avalonia.LogicalTree; +using PlanViewer.App.Controls; + +namespace PlanViewer.Core.Tests; + +/// +/// #467: the substitution is only worth anything if it reaches the menu the reporter actually used, +/// and only when there is something to substitute. +/// +/// These drive the real control rather than the rewriter in isolation, because the failure in +/// #466 was not in producing text — it was in which text the two existing menu entries handed out. +/// A pure test of the rewriter would have passed all along. +/// +/// They also open the real menu on a real control in a real window. That used to be +/// impossible — a in a headless took the +/// shared Avalonia session's font manager down with it, so these tests called the menu's Opening +/// work directly instead. #474 fixed the session, so the right-click the reporter performed is now +/// the right-click the test performs. +/// +public class StatementParameterMenuTests +{ + [Fact] + public void WithParameters_TheMenuOffersValuesAndKeepsTheParameterizedFormReachable() + { + HeadlessUi.Run(() => + { + var grid = OpenStatementMenu(LoadPlan("forced_parameterization_plan.sqlplan")); + + Assert.Equal("Copy Query Text (with values)", MenuItemNamed(grid, "CopyStatementTextItem").Header); + Assert.Equal( + "Open in Query Editor (with values)", + MenuItemNamed(grid, "OpenStatementInEditorItem").Header); + Assert.True(MenuItemNamed(grid, "CopyParameterizedStatementTextItem").IsVisible); + }); + } + + [Fact] + public void WithoutParameters_TheMenuIsTheTwoEntriesItAlwaysWas() + { + HeadlessUi.Run(() => + { + var grid = OpenStatementMenu(LoadPlan("isnull_plan.sqlplan")); + + Assert.Equal("Copy Query Text", MenuItemNamed(grid, "CopyStatementTextItem").Header); + Assert.Equal("Open in Query Editor", MenuItemNamed(grid, "OpenStatementInEditorItem").Header); + Assert.False(MenuItemNamed(grid, "CopyParameterizedStatementTextItem").IsVisible); + }); + } + + [Fact] + public void OpenInQueryEditor_HandsOverTextThatWillActuallyRun() + { + HeadlessUi.Run(() => + { + var viewer = LoadPlan("forced_parameterization_plan.sqlplan"); + var grid = OpenStatementMenu(viewer); + + string? opened = null; + viewer.OpenInEditorRequested += (_, text) => opened = text; + + MenuItemNamed(grid, "OpenStatementInEditorItem") + .RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + + Assert.NotNull(opened); + Assert.DoesNotContain("@", opened); + Assert.Contains("[t].[StatusId] in (5,6,7,8,9)", opened); + }); + } + + private static PlanViewerControl LoadPlan(string planFileName) + { + var path = Path.Combine("Plans", planFileName); + Assert.True(File.Exists(path), $"Test plan not found: {path}"); + var xml = File.ReadAllText(path).Replace("encoding=\"utf-16\"", "encoding=\"utf-8\""); + + var viewer = new PlanViewerControl(); + Assert.True(viewer.LoadPlan(xml, planFileName), $"Plan failed to load: {viewer.LastLoadError}"); + + return viewer; + } + + /// + /// Right-clicks the statements grid and hands back the grid the menu hangs off. + /// + /// The window and the layout pass are what make the right-click possible: a ContextMenu + /// needs a TopLevel to open into, and the grid has no rows to select from until it has been + /// laid out. The request is raised rather than ContextMenu.Open being called, because + /// Open skips the Opening event and Opening is where the labels are decided — calling it + /// would test the menu with nothing having configured it. + /// + private static DataGrid OpenStatementMenu(PlanViewerControl viewer) + { + var window = new Window { Content = viewer, Width = 1400, Height = 900 }; + window.Show(); + window.UpdateLayout(); + + var grid = viewer.GetLogicalDescendants().OfType().First(g => g.Name == "StatementsGrid"); + grid.RaiseEvent(new ContextRequestedEventArgs()); + + return grid; + } + + private static MenuItem MenuItemNamed(DataGrid grid, string name) => + grid.ContextMenu!.Items.OfType().First(i => i.Name == name); +} 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