Repository navigation
Persistence fix on app exit/start + Highlight dialog Import/Export error fix #662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| using System.Reflection; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Added: a regression test covering the snapshot behavior." — Partial. The test covers only the live-UI capture in GatherSessionSnapshot. Neither of the other two claims is tested: nothing exercises "Save the live filter UI state before shutdown teardown" (the ordering in OnLogTabWindowFormClosing), and SkipPersistenceSaveOnClose is never touched by any test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, “exit/start” is only partially backed by tests. The fix is primarily on the save/exit side, not on restore logic. |
||
| using System.Runtime.Versioning; | ||
|
|
||
| using LogExpert.Core.Classes.Filter; | ||
| using LogExpert.Core.Config; | ||
| using LogExpert.Core.Interfaces; | ||
| using LogExpert.UI.Controls.LogWindow; | ||
| using LogExpert.UI.Interface; | ||
|
|
||
| using Moq; | ||
|
|
||
| using NUnit.Framework; | ||
|
|
||
| namespace LogExpert.Tests.Controls; | ||
|
|
||
| [TestFixture] | ||
| [Apartment(ApartmentState.STA)] | ||
| [SupportedOSPlatform("windows")] | ||
| public sealed class LogWindowPersistenceTests : IDisposable | ||
| { | ||
| private Mock<ILogWindowCoordinator> _coordinatorMock = null!; | ||
| private Mock<IConfigManager> _configManagerMock = null!; | ||
| private Settings _settings = null!; | ||
| private WindowsFormsSynchronizationContext? _syncContext; | ||
| private LogWindow _logWindow = null!; | ||
| private bool _disposed; | ||
|
|
||
| [OneTimeSetUp] | ||
| public void OneTimeSetUp () | ||
| { | ||
| var dir = Path.GetDirectoryName(typeof(LogWindowPersistenceTests).Assembly.Location)!; | ||
| _ = PluginRegistry.PluginRegistry.Create(dir, 500); | ||
| } | ||
|
|
||
| [SetUp] | ||
| public void SetUp () | ||
| { | ||
| if (SynchronizationContext.Current == null) | ||
| { | ||
| _syncContext = new WindowsFormsSynchronizationContext(); | ||
| SynchronizationContext.SetSynchronizationContext(_syncContext); | ||
| } | ||
|
|
||
| _coordinatorMock = new Mock<ILogWindowCoordinator>(); | ||
| _ = _coordinatorMock.Setup(c => c.ResolveHighlightGroup(It.IsAny<string?>(), It.IsAny<string?>())) | ||
| .Returns(new LogExpert.Core.Entities.HighlightGroup()); | ||
| _ = _coordinatorMock.Setup(c => c.SearchParams).Returns(new LogExpert.Core.Entities.SearchParams()); | ||
|
|
||
| _configManagerMock = new Mock<IConfigManager>(); | ||
| _settings = new Settings(); | ||
| _ = _configManagerMock.Setup(cm => cm.Settings).Returns(_settings); | ||
|
|
||
| _settings.FilterList.Add(new FilterParams | ||
| { | ||
| SearchText = "SHARED_FILTER_LIST_ENTRY", | ||
| IsCaseSensitive = false, | ||
| IsRegex = false, | ||
| IsFilterTail = true, | ||
| FuzzyValue = 0, | ||
| SpreadBefore = 0, | ||
| SpreadBehind = 0 | ||
| }); | ||
|
|
||
| _logWindow = new LogWindow( | ||
| _coordinatorMock.Object, | ||
| "test.log", | ||
| isTempFile: false, | ||
| forcePersistenceLoading: false, | ||
| configManager: _configManagerMock.Object); | ||
| } | ||
|
|
||
| [TearDown] | ||
| public void TearDown () | ||
| { | ||
| _logWindow?.Dispose(); | ||
| _syncContext?.Dispose(); | ||
| _syncContext = null; | ||
| } | ||
|
|
||
| public void Dispose () | ||
| { | ||
| Dispose(true); | ||
| GC.SuppressFinalize(this); | ||
| } | ||
|
|
||
| private void Dispose (bool disposing) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicated Code / Speculative Generality (tests) — the test fixture implements a full Dispose(bool)/_disposed pattern that duplicates its own TearDown; possibly analyzer-driven (NUnit1032), but the guard-flag pattern is overkill either way.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, mostly style noise, but it is not a product bug. |
||
| { | ||
| if (_disposed) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (disposing) | ||
| { | ||
| _logWindow?.Dispose(); | ||
| _syncContext?.Dispose(); | ||
| } | ||
|
|
||
| _disposed = true; | ||
| } | ||
|
|
||
| [Test] | ||
| public void GatherSessionSnapshot_SavesLiveFilterState_InsteadOfSharedFilterList () | ||
| { | ||
| SetText("filterComboBox", "LIVE_FILTER_TEXT"); | ||
| SetText("filterRangeComboBox", "LIVE_RANGE_TEXT"); | ||
| SetChecked("filterCaseSensitiveCheckBox", true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Speculative Generality (tests) — SetChecked/SetValue use reflection (GetProperty("Checked")) on controls that could be typed directly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly style noise, not a correctness issue. |
||
| SetChecked("filterRegexCheckBox", true); | ||
| SetChecked("filterTailCheckBox", false); | ||
| SetChecked("invertFilterCheckBox", true); | ||
| SetChecked("rangeCheckBox", true); | ||
| SetChecked("columnRestrictCheckBox", true); | ||
| SetValue("knobControlFuzzy", 3); | ||
| SetValue("knobControlFilterBackSpread", 4); | ||
| SetValue("knobControlFilterForeSpread", 5); | ||
|
|
||
| var snapshot = _logWindow.GatherSessionSnapshot(); | ||
|
|
||
| Assert.That(snapshot.FilterParamsList, Has.Count.EqualTo(1)); | ||
|
|
||
| var saved = snapshot.FilterParamsList[0]; | ||
| var shared = _settings.FilterList[0]; | ||
|
|
||
| Assert.Multiple(() => | ||
| { | ||
| Assert.That(saved, Is.Not.SameAs(shared)); | ||
| Assert.That(saved.SearchText, Is.EqualTo("LIVE_FILTER_TEXT")); | ||
| Assert.That(saved.RangeSearchText, Is.EqualTo("LIVE_RANGE_TEXT")); | ||
| Assert.That(saved.IsCaseSensitive, Is.True); | ||
| Assert.That(saved.IsRegex, Is.True); | ||
| Assert.That(saved.IsFilterTail, Is.False); | ||
| Assert.That(saved.IsInvert, Is.True); | ||
| Assert.That(saved.IsRangeSearch, Is.True); | ||
| Assert.That(saved.ColumnRestrict, Is.True); | ||
| Assert.That(saved.FuzzyValue, Is.EqualTo(3)); | ||
| Assert.That(saved.SpreadBefore, Is.EqualTo(4)); | ||
| Assert.That(saved.SpreadBehind, Is.EqualTo(5)); | ||
| Assert.That(saved.SearchText, Is.Not.EqualTo(shared.SearchText)); | ||
| }); | ||
| } | ||
|
|
||
| private void SetText (string fieldName, string value) | ||
| { | ||
| var control = GetField<System.Windows.Forms.Control>(fieldName); | ||
| control.Text = value; | ||
| } | ||
|
|
||
| private void SetChecked (string fieldName, bool value) | ||
| { | ||
| var control = GetField<object>(fieldName)!; | ||
| control.GetType().GetProperty("Checked")!.SetValue(control, value); | ||
| } | ||
|
|
||
| private void SetValue (string fieldName, int value) | ||
| { | ||
| var control = GetField<object>(fieldName)!; | ||
| control.GetType().GetProperty("Value")!.SetValue(control, value); | ||
| } | ||
|
|
||
| private T GetField<T> (string fieldName) | ||
| { | ||
| var field = _logWindow.GetType().GetField(fieldName, BindingFlags.Instance | BindingFlags.NonPublic); | ||
| Assert.That(field, Is.Not.Null, $"Could not find private field '{fieldName}' on LogWindow"); | ||
| return (T)field!.GetValue(_logWindow)!; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,6 +168,15 @@ internal partial class LogWindow : DockContent, ILogPaintContextUI, ILogView, IL | |
|
|
||
| private bool _waitingForClose; | ||
|
|
||
| /// <summary> | ||
| /// Set by the application shutdown path after a pre-save pass has already captured the | ||
| /// current persistence state. Prevents the per-window close handler from writing an older | ||
| /// snapshot after child filter tabs have already been torn down. | ||
| /// </summary> | ||
| [Browsable(false)] | ||
| [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Browsable(false)]/[DesignerSerializationVisibility] on SkipPersistenceSaveOnClose are speculative decoration; LogWindow is never designer-hosted with this property.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks unnecessary because this is not a designer-driven surface, but it is harmless metadata, not a correctness issue. I would treat it as cleanup, not a defect. They are not behavior-changing. |
||
| internal bool SkipPersistenceSaveOnClose { get; set; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Control-flag coupling — LogTabWindow mutates SkipPersistenceSaveOnClose on children before closing them; a Close(savePersistence: false) path would avoid the mutable flag.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that a Close(savePersistence: false) would be cleaner. I used the existing flag here to avoid widening the close-path change in this commit and to keep the behavior isolated. |
||
|
|
||
| #endregion | ||
|
|
||
| #region cTor | ||
|
|
@@ -905,7 +914,10 @@ private void OnLogWindowClosing (object sender, CancelEventArgs e) | |
| return; | ||
| } | ||
|
|
||
| SavePersistenceData(false); | ||
| if (!SkipPersistenceSaveOnClose) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Prevent close-time tab disposal from overwriting the already saved snapshot." — Works for the guarded path (pre-save runs before RemoveAndDisposeLogWindow, and OnLogWindowClosing at [LogWindow.cs:917] (skips the re-save). But the flag is set-only and never reset — if FormClosing is ever cancelled after the handler runs, surviving windows silently skip persistence on all later manual tab closes. Also Reload() ([LogWindow.cs:7420] and ReloadNewFile() call SavePersistenceData(false) unguarded — a rollover firing mid-shutdown could still overwrite the snapshot. Narrow races, worth noting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be a concern. The flag is only set in the app shutdown cascade, immediately before the pre-save and disposal sequence. I don’t see a normal path where it remains active after that close flow completes. |
||
| { | ||
| SavePersistenceData(false); | ||
| } | ||
| CloseLogWindow(); | ||
| } | ||
|
|
||
|
|
@@ -5953,9 +5965,14 @@ public SessionSnapshot GatherSessionSnapshot () | |
|
|
||
| if (Preferences.SaveFilters) | ||
| { | ||
| //when a filter is added, its added to the Configmanager.Settings.FilterList and not to the _filterParams, this is probably an oversight and maybe a bug | ||
| //but for the consistency the FilterList should be saved as whole for every file | ||
| snapshot.FilterParamsList = [.. ConfigManager.Settings.FilterList]; | ||
| // Persist the live filter state for this window. The active filter lives in the window | ||
| // controls / _filterParams, while ConfigManager.Settings.FilterList is only the shared | ||
| // saved-filter list UI. The session loader restores the first entry as the active | ||
| // filter, so that first slot must be the current filter state. | ||
| snapshot.FilterParamsList = | ||
| [ | ||
| CaptureCurrentFilterParams(), | ||
| ]; | ||
|
|
||
| foreach (var filterPipe in _filterPipeList) | ||
| { | ||
|
|
@@ -5983,6 +6000,24 @@ public SessionSnapshot GatherSessionSnapshot () | |
| return snapshot; | ||
| } | ||
|
|
||
| private FilterParams CaptureCurrentFilterParams () | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicated Code — CaptureCurrentFilterParams() repeats the controls→FilterParams copy shape already present at ~4420–4440 (FilterSearch) and again in the dirty-check at ~4770–4795. Three sites now share the shape; one helper is warranted — though "Surgical Changes" partially excuses not touching the pre-existing sites.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I agree the duplication is real, but I kept this commit "surgical" and only touched the new capture path. |
||
| { | ||
| var filterParams = _filterParams.Clone(); | ||
| filterParams.SearchText = filterComboBox.Text; | ||
| filterParams.IsRangeSearch = rangeCheckBox.Checked; | ||
| filterParams.RangeSearchText = filterRangeComboBox.Text; | ||
| filterParams.IsCaseSensitive = filterCaseSensitiveCheckBox.Checked; | ||
| filterParams.IsRegex = filterRegexCheckBox.Checked; | ||
| filterParams.IsFilterTail = filterTailCheckBox.Checked; | ||
| filterParams.IsInvert = invertFilterCheckBox.Checked; | ||
| filterParams.FuzzyValue = knobControlFuzzy.Value; | ||
| filterParams.SpreadBefore = knobControlFilterBackSpread.Value; | ||
| filterParams.SpreadBehind = knobControlFilterForeSpread.Value; | ||
| filterParams.ColumnRestrict = columnRestrictCheckBox.Checked; | ||
| filterParams.CurrentColumnizer = CurrentColumnizer; | ||
| return filterParams; | ||
| } | ||
|
|
||
| public void Close (bool dontAsk) | ||
| { | ||
| Preferences.AskForClose = !dontAsk; | ||
|
|
@@ -7957,4 +7992,4 @@ public void RefreshLogView () | |
| } | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fix for export, not import! Import already has it.