From 847f7988ceeed9490339c7e51c10dafc3fdc5995 Mon Sep 17 00:00:00 2001 From: BRUNER Patrick Date: Thu, 30 Jul 2026 13:46:42 +0200 Subject: [PATCH] fix: replace the last-open-files list when a window closes (#686) SaveLastOpenFilesList only ever appended, and the list was cleared in one branch of LoadStartupFiles. Startup with command-line files, or with "Open last files" turned off, skipped that clear, so every close appended the current tabs to a stale list: it grew without bound and restored tabs closed sessions ago. The closing window now owns the list. The first window to close replaces it, dropping entries from earlier sessions; windows closing after it append, so a multi-window session is still restored as a whole. The clear in LoadStartupFiles is gone, which also means a crash mid-session no longer wipes the set of files to restore. Tests cover the replace, the append for a second closing window, the clear when only temp files are open, and that loading keeps the list. --- .../Services/FileOperationServiceTests.cs | 107 ++++++++++++++++-- .../FileOperationService.cs | 19 +++- 2 files changed, 116 insertions(+), 10 deletions(-) diff --git a/src/LogExpert.Tests/Services/FileOperationServiceTests.cs b/src/LogExpert.Tests/Services/FileOperationServiceTests.cs index 51b326fe..32830f65 100644 --- a/src/LogExpert.Tests/Services/FileOperationServiceTests.cs +++ b/src/LogExpert.Tests/Services/FileOperationServiceTests.cs @@ -54,6 +54,10 @@ public void Setup () _settings = new Settings(); _ = _configManagerMock.Setup(cm => cm.Settings).Returns(_settings); + _ = _configManagerMock.Setup(cm => cm.ClearLastOpenFilesList()).Callback(() => _settings.LastOpenFilesList.Clear()); + + // Process-wide state: every test starts as if no window has saved its files yet + FileOperationService.LastOpenFilesListReplaced = false; _ = _pluginRegistryMock.Setup(pr => pr.RegisteredColumnizers).Returns([]); _factoryCalls = []; @@ -799,9 +803,6 @@ public void LoadStartupFiles_WithStartupFiles_LoadsStartupFiles_IgnoresLastOpen // Assert — startup files are loaded (via LoadFilesWithOption → AddFileTab) Assert.That(_factoryCalls, Has.Count.EqualTo(1)); Assert.That(_factoryCalls[0].Request.FileName, Is.EqualTo("startup.log")); - - // Last-open files should NOT be loaded - _configManagerMock.Verify(cm => cm.ClearLastOpenFilesList(), Times.Never); } [Test] @@ -818,7 +819,6 @@ public void LoadStartupFiles_NoStartupFiles_OpenLastFilesEnabled_LoadsLastOpenFi Assert.That(_factoryCalls, Has.Count.EqualTo(2)); Assert.That(_factoryCalls[0].Request.FileName, Is.EqualTo("file1.log")); Assert.That(_factoryCalls[1].Request.FileName, Is.EqualTo("file2.log")); - _configManagerMock.Verify(cm => cm.ClearLastOpenFilesList(), Times.Once); } [Test] @@ -850,21 +850,23 @@ public void LoadStartupFiles_EmptyStartupArray_TreatsAsNoStartupFiles () // Assert — empty startup array should fall through to last-open-files path Assert.That(_factoryCalls, Has.Count.EqualTo(1)); Assert.That(_factoryCalls[0].Request.FileName, Is.EqualTo("file1.log")); - _configManagerMock.Verify(cm => cm.ClearLastOpenFilesList(), Times.Once); } [Test] - public void LoadStartupFiles_ClearsLastOpenFilesAfterLoading () + public void LoadStartupFiles_KeepsLastOpenFilesList () { // Arrange _settings.Preferences.OpenLastFiles = true; + _settings.LastOpenFilesList.Add("file1.log"); var lastOpenFiles = new List { "file1.log" }; // Act _sut.LoadStartupFiles(lastOpenFiles, null); - // Assert - _configManagerMock.Verify(cm => cm.ClearLastOpenFilesList(), Times.Once); + // Assert — restoring tabs must not empty the persisted list: SaveLastOpenFilesList owns it, + // so a crash mid-session still leaves the files to restore next time + _configManagerMock.Verify(cm => cm.ClearLastOpenFilesList(), Times.Never); + Assert.That(_settings.LastOpenFilesList, Is.EqualTo(new[] { "file1.log" })); } [Test] @@ -945,6 +947,74 @@ public void SaveLastOpenFilesList_MultipleMixed_OnlySavesNonTemp () Assert.That(_settings.LastOpenFilesList, Does.Contain("server.log")); } + [Test] + public void SaveLastOpenFilesList_ReplacesStaleEntries () + { + // Arrange — a list left over from an earlier session, e.g. because LogExpert was + // started with command-line files and so never cleared it during startup + _settings.LastOpenFilesList.Add("old1.log"); + _settings.LastOpenFilesList.Add("old2.log"); + + var coordinatorMock = new Mock(); + using var normalWindow = new LogWindow(coordinatorMock.Object, "current.log", false, false, _configManagerMock.Object, PluginRegistry.PluginRegistry.Instance); + normalWindow.GivenFileName = "current.log"; + + _ = _tabControllerMock + .Setup(tc => tc.GetAllWindowsFromDockPanel()) + .Returns(new List { normalWindow }.AsReadOnly()); + + // Act + _sut.SaveLastOpenFilesList(); + + // Assert — the saved list holds the current tabs only, not the stale ones + Assert.That(_settings.LastOpenFilesList, Is.EqualTo(new[] { "current.log" })); + } + + [Test] + public void SaveLastOpenFilesList_SecondClosingWindow_AppendsToTheFirstWindowsEntries () + { + // Arrange — two LogTabWindows of one session, each with its own tab controller but + // sharing the config manager + _settings.LastOpenFilesList.Add("old.log"); + + var coordinatorMock = new Mock(); + using var firstWindow = new LogWindow(coordinatorMock.Object, "first.log", false, false, _configManagerMock.Object, PluginRegistry.PluginRegistry.Instance); + firstWindow.GivenFileName = "first.log"; + using var secondWindow = new LogWindow(coordinatorMock.Object, "second.log", false, false, _configManagerMock.Object, PluginRegistry.PluginRegistry.Instance); + secondWindow.GivenFileName = "second.log"; + + var firstService = CreateServiceForWindows(firstWindow); + var secondService = CreateServiceForWindows(secondWindow); + + // Act — the first window to close drops the stale entry, the second one adds to it + firstService.SaveLastOpenFilesList(); + secondService.SaveLastOpenFilesList(); + + // Assert + Assert.That(_settings.LastOpenFilesList, Is.EqualTo(new[] { "first.log", "second.log" })); + } + + [Test] + public void SaveLastOpenFilesList_NoFilesToSave_ClearsStaleEntries () + { + // Arrange — only a temp file is open, so nothing gets saved + _settings.LastOpenFilesList.Add("old1.log"); + + var coordinatorMock = new Mock(); + using var tempWindow = new LogWindow(coordinatorMock.Object, "temp.log", true, false, _configManagerMock.Object, PluginRegistry.PluginRegistry.Instance); + tempWindow.GivenFileName = "temp.log"; + + _ = _tabControllerMock + .Setup(tc => tc.GetAllWindowsFromDockPanel()) + .Returns(new List { tempWindow }.AsReadOnly()); + + // Act + _sut.SaveLastOpenFilesList(); + + // Assert + Assert.That(_settings.LastOpenFilesList, Is.Empty); + } + [Test] public void AddFileTabDeferred_SetsDoNotAddToDockPanelTrue () { @@ -985,6 +1055,27 @@ public void LoadFiles_DelegatesToAddFileTabs () Assert.That(_factoryCalls[1].Request.FileName, Is.EqualTo("b.log")); } + /// + /// Builds a second service on the shared config manager, standing in for another LogTabWindow + /// of the same session with its own tab controller. + /// + private FileOperationService CreateServiceForWindows (params LogWindow[] windows) + { + var tabControllerMock = new Mock(); + _ = tabControllerMock + .Setup(tc => tc.GetAllWindowsFromDockPanel()) + .Returns(windows.ToList().AsReadOnly()); + + return new FileOperationService( + _configManagerMock.Object, + tabControllerMock.Object, + _ledServiceMock.Object, + _pluginRegistryMock.Object, + _factory, + () => _clipboardText, + (fileName, isSingle) => _projectCallbackCalls.Add((fileName, isSingle))); + } + public void Dispose () { Dispose(true); diff --git a/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs b/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs index 3f5d511b..d91134bf 100644 --- a/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs +++ b/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs @@ -27,6 +27,14 @@ internal sealed class FileOperationService ( { private static readonly Logger _logger = LogManager.GetCurrentClassLogger(); + /// + /// Whether a closing window has already replaced the persisted last-open-files list in this + /// process. All LogTabWindows share one list, so the first window to close replaces it — dropping + /// entries left over from earlier sessions — and every window closing after it appends, which + /// restores a multi-window session as a whole. + /// + internal static bool LastOpenFilesListReplaced { get; set; } + private readonly IConfigManager _configManager = configManager; private readonly ITabController _tabController = tabController; private readonly ILedIndicatorService _ledIndicatorService = ledIndicatorService; @@ -329,6 +337,15 @@ public void LoadFiles (string[] fileNames) public void SaveLastOpenFilesList () { + // Nothing else clears the list, so the first window to close has to replace it: appending + // unconditionally would keep entries from earlier sessions and grow the list without + // bound. Windows closing after it append, so all of their files are restored. + if (!LastOpenFilesListReplaced) + { + _configManager.ClearLastOpenFilesList(); + LastOpenFilesListReplaced = true; + } + foreach (var logWin in _tabController.GetAllWindowsFromDockPanel().Where(logwin => !logwin.IsTempFile)) { _configManager.Settings.LastOpenFilesList.Add(logWin.GivenFileName); @@ -349,8 +366,6 @@ public void LoadStartupFiles (IList lastOpenFiles, string[]? startupFile { _ = AddFileTab(new FileTabRequest { FileName = name }); } - - _configManager.ClearLastOpenFilesList(); } } } \ No newline at end of file