From 8ef1fb114686fedabbcffe92b17986aca446d94f Mon Sep 17 00:00:00 2001 From: Dan Moisan Date: Wed, 2 Sep 2026 06:53:56 -0400 Subject: [PATCH] fix(quickfiler): resolve issue #726 code-correctness sweep (6 of 8 findings) Consolidated fix for eight code-correctness findings surfaced during the bugs-638-644-647 parallel-orchestration review sweep, each left unfixed by its originating item to stay within that item's declared file-footprint. 1. FilerQueue drain-barrier hardening (Major): wrap the consumer loop body in try/finally so _consumerRunning always clears, add a null guard on Enqueue, and a null/empty-safe diagnostic path in the catch handler. ConsumeAsync narrowed to internal (no external callers). Applied the identical try/finally hardening to the sibling TaskVisualization/FlagChangeTrainingQueue.cs guard-reset. 2. QfcHomeController.Metrics: null-guard the diagnostics line array and delete the orphaned calendar appointment when diagnostics are empty, instead of reordering (GetMoveDiagnostics takes the appointment by ref). 4. EfcItemController: route both InitializeWebViewAsync fire-and-forget call sites through a new guarded wrapper (mirroring QfcItemController's existing fault boundary) so a fault is logged instead of silently finalized away. 5. Guard both WebView fault-boundary sinks (Efc and Qfc) and EfcFormController.BoundaryErrorSink against a null or throwing sink delegate, so a misconfigured sink cannot reinstate an unobserved fault. 6. Removed QfcFormController's dead _helperTasks field: it started a FromMailItemAsync call per item and discarded the result unawaited, while the sync pipeline it feeds builds its own MailItemHelper via a separate synchronous constructor -- confirmed by grep as write-only with no reader anywhere in the repo. 7. TaskViewer.ProcessCmdKey now falls through to base.ProcessCmdKey when the controller does not consume the Alt-chord keystroke, instead of unconditionally claiming every Alt-modified key. 8. QfcFormViewer.ProcessCmdKey: removed two dead locals with no effect on the real event pipeline. Added the missing positive regression test for Keys.Menu | Keys.Alt in EfcViewerTests (the Keys.Menu disjunct of ClaimsAltChord's guard was previously deletable without failing any test). Deferred, with evidence, not fixed in this change: - Finding 3 (StoreWrapperController.EvaluateLaunchReadiness): already tracked as an open item in docs/features/active/2026-07-09-storewrapper-dialog-imprecise-for-genuine-failure-287/spec.md, crosses the UtilitiesCS/TaskMaster assembly boundary. - Finding 6, LoadFolderHandler/InitAsync gap: the sync LoadFolderHandler has no synchronous equivalent of FolderPredictor.InitAsync, and its three call sites run through PopulateFolderComboBox, which checks InvokeRequired -- forcing a synchronous await risks a UI-thread deadlock. A minimal fix here would trade a latent hang for a more certain one. - Finding 6, dead QfcHighConfidencePreFilter.FilterAsync: confirmed unreachable in production (HighConfidencePreFilterLoader is declared but never invoked), but removal cascades into three test files (QfcHighConfidencePreFilterTests.cs, QfcHomeControllerIssue218Tests.cs, QfcHomeControllerRunAsyncHighConfidenceTests.cs), exceeding this change's minimal-fix scope. - Finding 6, two stale comments and one stale test docstring from item #678's remediation: the issue gives no file/line citation and #678 touches 26 files, too broad to identify reliably without expanding scope for a documentation-only latent nit. New regression tests: FilerQueue null-Enqueue and empty-Helpers-drain-recovery; EfcViewer Keys.Menu | Keys.Alt positive Alt-chord match. Toolchain: CSharpier format, analyzer rebuild (EnableNETAnalyzers+EnforceCodeStyleInBuild), nullable rebuild (TreatWarningsAsErrors), MSTest 1475/1475 passed across QuickFiler.Test + TaskVisualization.Test. Fixes #726 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01ATYLDoRLKXS5sgAzegW7ZL --- QuickFiler.Test/Controllers/EfcViewerTests.cs | 18 ++++ .../Controllers/FilerQueueTests.cs | 77 ++++++++++++++++- QuickFiler/Controllers/EfcFormController.cs | 39 +++++++-- .../EfcItemController.WebViewFaultBoundary.cs | 67 +++++++++++++++ QuickFiler/Controllers/EfcItemController.cs | 10 ++- QuickFiler/Controllers/FilerQueue.cs | 84 +++++++++++++------ .../Controllers/QfcFormController.Actions.cs | 9 +- QuickFiler/Controllers/QfcFormController.cs | 1 - .../Controllers/QfcHomeController.Metrics.cs | 18 +++- .../QfcItemController.WebViewFaultBoundary.cs | 28 ++++++- QuickFiler/QuickFiler.csproj | 1 + QuickFiler/Viewers/QfcFormViewer.cs | 8 +- TaskVisualization/FlagChangeTrainingQueue.cs | 34 +++++--- TaskVisualization/TaskViewer.cs | 14 +++- 14 files changed, 345 insertions(+), 63 deletions(-) create mode 100644 QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs diff --git a/QuickFiler.Test/Controllers/EfcViewerTests.cs b/QuickFiler.Test/Controllers/EfcViewerTests.cs index be0718a56..89c4688cb 100644 --- a/QuickFiler.Test/Controllers/EfcViewerTests.cs +++ b/QuickFiler.Test/Controllers/EfcViewerTests.cs @@ -119,6 +119,24 @@ public void ClaimsAltChord_WithBareAltAndHandler_ReturnsTrue() .BeTrue("bare Alt is the chord the keyboard dialog services"); } + // Issue #726 finding 8: ClaimsAltChord's guard is `keyCode == Keys.Menu || keyCode == + // Keys.None` -- the bare-Alt test above only exercises the Keys.None disjunct (Keys.Alt + // carries no key-code bits). Keys.Menu | Keys.Alt is the VK_MENU keystroke shape (the Alt + // key itself pressed as a key) and exercises the other disjunct; without this test the + // Keys.Menu arm of the predicate was deletable without failing any existing test. + [TestMethod] + public void ClaimsAltChord_WithMenuKeyAndAlt_ReturnsTrue() + { + var handler = new Mock(); + + EfcViewer + .ClaimsAltChord(handler.Object, Keys.Menu | Keys.Alt) + .Should() + .BeTrue( + "Keys.Menu | Keys.Alt is the VK_MENU keystroke shape of the bare Alt chord" + ); + } + [TestMethod] public void ClaimsAltChord_WithAltF_ReturnsFalse() { diff --git a/QuickFiler.Test/Controllers/FilerQueueTests.cs b/QuickFiler.Test/Controllers/FilerQueueTests.cs index b7b56f1fd..b93dec733 100644 --- a/QuickFiler.Test/Controllers/FilerQueueTests.cs +++ b/QuickFiler.Test/Controllers/FilerQueueTests.cs @@ -97,9 +97,9 @@ private static TaskCompletionSource NewGate() => new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); /// - /// Enqueues one item. Every enqueued item carries a real helper because the preserved - /// worker catch block calls item.Helpers.First(); an empty list would raise - /// inside the catch, escape the worker loop, and leave the drain permanently incomplete. + /// Enqueues one item carrying a real helper, for tests that are not themselves about the + /// empty-helpers diagnostic path fixed under issue #726 (see + /// ). /// private static void EnqueueOne(FilerQueue queue) => queue.Enqueue(new EmailFiler(), OneHelper()); @@ -354,5 +354,76 @@ public async Task ItemProcessor_ThatThrows_StillDecrementsAndDrainCompletes() drain.IsCompleted.Should().BeTrue("the throwing item still decrements the counter"); invocations.Should().Be(2, "the worker loop continues past the failing item"); } + + [TestMethod] + public void Enqueue_NullItem_ThrowsArgumentNullException() + { + // Arrange + var queue = new FilerQueue(); + + // Act + Action act = () => queue.Enqueue((FilerQueueItem)null); + + // Assert + act.Should() + .Throw( + "issue #726 finding 1: a null item must never reach the queue -- it previously " + + "left the outstanding counter permanently incremented, since nothing was " + + "actually queued to decrement it" + ); + } + + /// + /// Regression test for issue #726 finding 1. Before the fix, an item constructed with an + /// empty (not null) helpers list -- which 's constructor + /// permits -- would cause item.Helpers.First() inside the worker's diagnostic catch + /// handler to throw . That exception escaped the + /// catch itself, unwound the entire worker loop, and left the consumer-running flag + /// permanently set with no try/finally to clear it -- so no later Enqueue + /// call would ever start a new worker again, permanently hanging the background mover. + /// + [TestMethod] + public async Task ConsumeAsync_ItemWithEmptyHelpersThrows_ConsumerRecoversForLaterItems() + { + // Arrange + var queue = new FilerQueue(); + var emptyHelpersItem = new FilerQueueItem(new EmailFiler(), new List()); + TaskCompletionSource secondProcessed = NewGate(); + int invocations = 0; + queue.ItemProcessor = item => + { + int index = Interlocked.Increment(ref invocations) - 1; + if (index == 0) + { + // Provoked by an EMPTY (not null) Helpers list, exactly like the diagnostic + // path's own item.Helpers.First() would throw before the fix. + throw new InvalidOperationException( + "processing fails for the empty-helpers item" + ); + } + + secondProcessed.TrySetResult(true); + return Task.CompletedTask; + }; + + // Act: enqueue the poisoning item, wait for the queue to fully drain (proving the + // worker did not hang), then enqueue a second, normal item and confirm a NEW worker + // starts and processes it -- which only happens if the consumer-running flag was + // actually cleared rather than left stuck by the first item's escaping exception. + queue.Enqueue(emptyHelpersItem); + await queue.WhenDrainedAsync(); + EnqueueOne(queue); + await secondProcessed.Task; + await queue.WhenDrainedAsync(); + + // Assert + invocations + .Should() + .Be( + 2, + "the consumer-running flag must clear on the exceptional exit path so a later " + + "Enqueue can start a fresh worker, not just on the normal empty-queue exit" + ); + } } } diff --git a/QuickFiler/Controllers/EfcFormController.cs b/QuickFiler/Controllers/EfcFormController.cs index b6ee2f5e6..ffc12116c 100644 --- a/QuickFiler/Controllers/EfcFormController.cs +++ b/QuickFiler/Controllers/EfcFormController.cs @@ -128,6 +128,33 @@ internal EfcFormController InitializeDataFields(EfcDataModel dataModel) internal System.Action BoundaryErrorSink { get; set; } = (message, exception) => logger.Error(message, exception); + /// + /// Issue #726 finding 5: invokes defensively so a null or + /// throwing sink delegate cannot silently reinstate the unobserved-fault behavior this + /// boundary exists to prevent -- these call sites all sit in an async void handler's + /// catch block, where an escaping exception would crash the process rather than merely fail + /// to log. + /// + private void TryReportBoundaryFault(string message, System.Exception exception) + { + var sink = BoundaryErrorSink; + if (sink is null) + { + logger.Error(message, exception); + return; + } + + try + { + sink(message, exception); + } + catch (System.Exception sinkException) + { + logger.Error($"{message} (and the error sink itself threw)", sinkException); + logger.Error(message, exception); + } + } + private IApplicationGlobals _globals; private System.Action _parentCleanup; private EfcDataModel _dataModel; @@ -453,7 +480,7 @@ internal async Task ButtonCancelClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -470,7 +497,7 @@ internal async Task ButtonOkClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -488,7 +515,7 @@ internal async Task ButtonRefreshClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -550,7 +577,7 @@ await _dataModel.MoveToFolderAsync( } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -565,7 +592,7 @@ internal async Task ButtonDeleteClickAsync() } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } @@ -1135,7 +1162,7 @@ public async Task PopulateFolderCombobox(object folderList = null) } catch (System.Exception ex) { - BoundaryErrorSink(ex.Message, ex); + TryReportBoundaryFault(ex.Message, ex); } } diff --git a/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs b/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs new file mode 100644 index 000000000..76e7dd099 --- /dev/null +++ b/QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs @@ -0,0 +1,67 @@ +using System; +using System.Threading.Tasks; + +namespace QuickFiler.Controllers +{ + internal partial class EfcItemController + { + /// + /// Issue #726 finding 4: fault-boundary sink for , + /// mirroring QfcItemController.WebViewInitializationErrorSink. Named distinctly so no + /// shared contract with the QFC sink or with + /// is implied. + /// + internal Action WebViewInitializationErrorSink { get; set; } = + (message, exception) => logger.Error(message, exception); + + /// + /// Issue #726 finding 4: fault boundary for . Both + /// production call sites previously discarded the task returned by + /// Task.Run(() => InitializeWebViewAsync()), so a fault there was never observed -- + /// under .NET Framework 4.5+, a discarded faulted task is silently finalized with no + /// diagnostic. This member contains the fault instead of returning it: the task it returns + /// never transitions to Faulted. + /// + internal async Task InitializeWebViewGuardedAsync() + { + try + { + await InitializeWebViewAsync(); + } + catch (OperationCanceledException) + { + // Cooperative cancellation during teardown is expected and is not a fault. + } + catch (Exception ex) + { + // Issue #726 finding 5: guard against a null or throwing sink delegate so a + // misconfigured sink cannot silently reinstate the unobserved-fault behavior this + // boundary exists to prevent. + TryReportWebViewInitializationFault(ex); + } + } + + private void TryReportWebViewInitializationFault(Exception ex) + { + var sink = WebViewInitializationErrorSink; + if (sink is null) + { + logger.Error("WebView2 initialization failed.", ex); + return; + } + + try + { + sink("WebView2 initialization failed.", ex); + } + catch (Exception sinkException) + { + logger.Error( + "WebView2 initialization failed, and the error sink itself threw.", + sinkException + ); + logger.Error("Original WebView2 initialization failure.", ex); + } + } + } +} diff --git a/QuickFiler/Controllers/EfcItemController.cs b/QuickFiler/Controllers/EfcItemController.cs index d320b6395..37b14c1e5 100644 --- a/QuickFiler/Controllers/EfcItemController.cs +++ b/QuickFiler/Controllers/EfcItemController.cs @@ -23,7 +23,7 @@ namespace QuickFiler.Controllers { [ExcludeFromCodeCoverage] - internal class EfcItemController : IItemControler + internal partial class EfcItemController : IItemControler { #region Constructors and Initializers @@ -94,7 +94,9 @@ public EfcItemController InitializeDataFields(EfcDataModel dataModel) PopulateControls(dataModel); PopulateConversation(); WireEvents(); - Task.Run(() => InitializeWebViewAsync()); + // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged + // instead of silently finalized away as an unobserved discarded-task exception. + _ = InitializeWebViewGuardedAsync(); return this; } @@ -150,7 +152,9 @@ private void Initialize(bool async) _itemPositionTips.Toggle(Enums.ToggleState.Off, shareColumn: true); WireEvents(); - Task.Run(() => InitializeWebViewAsync()); + // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged + // instead of silently finalized away as an unobserved discarded-task exception. + _ = InitializeWebViewGuardedAsync(); } private static readonly log4net.ILog logger = log4net.LogManager.GetLogger( diff --git a/QuickFiler/Controllers/FilerQueue.cs b/QuickFiler/Controllers/FilerQueue.cs index d76508178..1f78af291 100644 --- a/QuickFiler/Controllers/FilerQueue.cs +++ b/QuickFiler/Controllers/FilerQueue.cs @@ -47,6 +47,8 @@ public class FilerQueue public void Enqueue(FilerQueueItem item) { + item.ThrowIfNull(); + bool startWorker; lock (_sync) @@ -117,46 +119,78 @@ public Task WhenDrainedAsync() } } - public async Task ConsumeAsync() + internal async Task ConsumeAsync() { await Task.Run(async () => { - while (true) + // Issue #726 finding 1: this outer try/finally is a safety net, not the primary + // mechanism. The normal exit path (TryTake fails) still clears the flag inside the + // same critical section as the failed take, which is what closes the orphaned-item + // window described below. This finally exists so that ANY exception escaping the + // loop -- including one from the diagnostic branch of the inner catch, which the + // inner catch cannot catch itself -- still clears the flag rather than leaving + // _consumerRunning permanently true and hanging the background mover. + try { - FilerQueueItem item; - - lock (_sync) + while (true) { - // Clearing the flag in the same critical section in which TryTake fails is what - // closes the orphaned-item window: a producer cannot observe "a worker is - // running" after this worker has decided to stop. - if (!Queue.TryTake(out item)) + FilerQueueItem item; + + lock (_sync) { - _consumerRunning = false; - return; + // Clearing the flag in the same critical section in which TryTake fails is + // what closes the orphaned-item window: a producer cannot observe "a worker + // is running" after this worker has decided to stop. + if (!Queue.TryTake(out item)) + { + _consumerRunning = false; + return; + } } - } - try - { - await ItemProcessor(item); - } - catch (Exception e) - { - var first = item.Helpers.First(); - logger.Error( - $"Error sorting mail items Subject: {first.Subject} Sent On: {first.SentOn} from {first.SenderName} {e.Message}", - e - ); + try + { + await ItemProcessor(item); + } + catch (Exception e) + { + LogItemFailure(item, e); + } + finally + { + CompleteItem(); + } } - finally + } + finally + { + lock (_sync) { - CompleteItem(); + _consumerRunning = false; } } }); } + /// + /// Issue #726 finding 1: Helpers may legitimately be empty (the constructor guards + /// against null and null elements, but not against an empty list), so the original + /// item.Helpers.First() could throw from + /// inside this catch handler -- an exception the catch cannot catch itself, which escaped + /// the worker loop entirely and left stuck. + /// + private void LogItemFailure(FilerQueueItem item, Exception e) + { + var first = item.Helpers?.FirstOrDefault(); + var subject = first?.Subject ?? "(no helpers)"; + var sentOn = first?.SentOn; + var sender = first?.SenderName ?? "(unknown)"; + logger.Error( + $"Error sorting mail items Subject: {subject} Sent On: {sentOn} from {sender} {e.Message}", + e + ); + } + /// /// Decrements the outstanding-work counter and, when it reaches zero, completes and clears the /// drain signal. The signal is captured under the monitor and completed outside it. diff --git a/QuickFiler/Controllers/QfcFormController.Actions.cs b/QuickFiler/Controllers/QfcFormController.Actions.cs index 7d57f3965..a69385bee 100644 --- a/QuickFiler/Controllers/QfcFormController.Actions.cs +++ b/QuickFiler/Controllers/QfcFormController.Actions.cs @@ -2,7 +2,6 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Threading; using System.Threading.Tasks; using System.Windows.Forms; @@ -43,9 +42,11 @@ listObjects is null return; } - _helperTasks = listObjects - .Select(x => MailItemHelper.FromMailItemAsync(x, _globals, Token, false)) - .ToList(); + // Issue #726 finding 6: this sync pipeline reaches QfcItemController.Initialize(false) + // -> PopulateControls(MailItem, int), which builds its own MailItemHelper via the + // synchronous constructor -- it never consumes a helper produced here. The prior + // per-item FromMailItemAsync call below was started, discarded unawaited, and its + // result read by nothing, so it was pure duplicated Outlook COM work. Removed. _groups = new QfcCollectionController( AppGlobals: _globals, viewerInstance: _formViewer, diff --git a/QuickFiler/Controllers/QfcFormController.cs b/QuickFiler/Controllers/QfcFormController.cs index b7fe4fd69..e06644692 100644 --- a/QuickFiler/Controllers/QfcFormController.cs +++ b/QuickFiler/Controllers/QfcFormController.cs @@ -89,7 +89,6 @@ public IQfcFormController Init() private Dictionary _themes; private BlockingCollection _undoQueue = []; private Task _undoConsumerTask; - private List> _helperTasks = []; #endregion diff --git a/QuickFiler/Controllers/QfcHomeController.Metrics.cs b/QuickFiler/Controllers/QfcHomeController.Metrics.cs index 38d33fdac..cf5e1cc77 100644 --- a/QuickFiler/Controllers/QfcHomeController.Metrics.cs +++ b/QuickFiler/Controllers/QfcHomeController.Metrics.cs @@ -151,6 +151,13 @@ public async Task WriteMetricsAsync(string filename) // If DebugLVL And vbCommand Then Debug.Print SubNm & " Variable durationText = " & durationText durationMinutesText = (Duration / 60d).ToString("##0.00", CultureInfo.InvariantCulture); + + // Issue #726 finding 2: GetMoveDiagnostics needs an AppointmentItem reference before it + // runs (it may annotate the appointment WriteMoveToCalendar creates), so the calendar + // write itself cannot move after the empty-diagnostics check below. What can move is + // the DECISION to keep the appointment: an empty-diagnostics session was previously + // producing a calendar entry with no corresponding metrics file. Deleting the orphaned + // appointment when there is nothing to report keeps the two artifacts symmetric. WriteMoveToCalendar( OlEndTime, OlStartTime, @@ -170,10 +177,17 @@ ref OlAppointment // The call is made through IQfcCollectionController.GetMoveDiagnostics, which carries // no XML documentation and therefore no non-null element guarantee, so this filter - // defends the interface contract rather than a known producer defect. - var lines = strOutput.Where(line => !string.IsNullOrWhiteSpace(line)).ToArray(); + // defends the interface contract rather than a known producer defect. Issue #726 + // finding 2: the array itself is also treated as possibly null for the same reason. + var lines = (strOutput ?? []).Where(line => !string.IsNullOrWhiteSpace(line)).ToArray(); if (lines.Length == 0) { + // Issue #726 finding 2: nothing to report, so the appointment WriteMoveToCalendar + // just created is deleted rather than left orphaned with no matching metrics file. + if (OlAppointment is not null) + { + OlAppointment.Delete(); + } return; } diff --git a/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs b/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs index e383e31b3..e798826de 100644 --- a/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs +++ b/QuickFiler/Controllers/QfcItemController.WebViewFaultBoundary.cs @@ -34,7 +34,33 @@ internal async Task InitializeWebViewGuardedAsync() } catch (Exception ex) { - WebViewInitializationErrorSink("WebView2 initialization failed.", ex); + // Issue #726 finding 5: guard against a null or throwing sink delegate so a + // misconfigured sink cannot silently reinstate the unobserved-fault behavior this + // boundary exists to prevent. + TryReportWebViewInitializationFault(ex); + } + } + + private void TryReportWebViewInitializationFault(Exception ex) + { + var sink = WebViewInitializationErrorSink; + if (sink is null) + { + logger.Error("WebView2 initialization failed.", ex); + return; + } + + try + { + sink("WebView2 initialization failed.", ex); + } + catch (Exception sinkException) + { + logger.Error( + "WebView2 initialization failed, and the error sink itself threw.", + sinkException + ); + logger.Error("Original WebView2 initialization failure.", ex); } } } diff --git a/QuickFiler/QuickFiler.csproj b/QuickFiler/QuickFiler.csproj index 2f0782930..4f9fc6eee 100644 --- a/QuickFiler/QuickFiler.csproj +++ b/QuickFiler/QuickFiler.csproj @@ -301,6 +301,7 @@ + diff --git a/QuickFiler/Viewers/QfcFormViewer.cs b/QuickFiler/Viewers/QfcFormViewer.cs index 7bfac67be..53a5377d9 100644 --- a/QuickFiler/Viewers/QfcFormViewer.cs +++ b/QuickFiler/Viewers/QfcFormViewer.cs @@ -57,11 +57,11 @@ protected override bool ProcessCmdKey(ref Message msg, Keys keyData) { if (Controllers.QfcFormKeyHandler.ClaimsAltChord(_keyboardHandler, keyData)) { + // Issue #726 finding 8: the pre-existing `sender`/`e` locals from before this + // predicate was extracted were dead -- ToggleKeyboardDialogAsync() takes no + // arguments, and e.Handled had no effect since `e` was never wired to the real + // event pipeline. Removed rather than left as unrelated cruft. SynchronizationContext.SetSynchronizationContext(UiSyncContext); - object sender = FromHandle(msg.HWnd); - var e = new KeyEventArgs(keyData); - //_keyboardHandler.ToggleKeyboardDialog(sender, e); - e.Handled = true; _ = _keyboardHandler.ToggleKeyboardDialogAsync(); return true; } diff --git a/TaskVisualization/FlagChangeTrainingQueue.cs b/TaskVisualization/FlagChangeTrainingQueue.cs index b9e2c4693..2cb698809 100644 --- a/TaskVisualization/FlagChangeTrainingQueue.cs +++ b/TaskVisualization/FlagChangeTrainingQueue.cs @@ -39,21 +39,33 @@ internal async Task ConsumeAsync() await Task.Run( async () => { - while (Queue.TryTake(out var item)) + // Issue #726 finding 1: the identical handshake window that motivated the + // FilerQueue fix exists here too. The guard reset was the loop's last statement, + // unprotected by try/finally, so any exception escaping the loop -- including one + // from the catch handler's own diagnostic expression -- would leave _guard + // permanently in its already-fired state and stop Immediate-mode consumption from + // ever restarting. + try { - try + while (Queue.TryTake(out var item)) { - await item.ProcessGroupAsync(); - } - catch (Exception e) - { - logger.Error( - $"Error training flags for email with subject: {(item as FlagChangeGroup)?.Subject}. {e.Message}", - e - ); + try + { + await item.ProcessGroupAsync(); + } + catch (Exception e) + { + logger.Error( + $"Error training flags for email with subject: {(item as FlagChangeGroup)?.Subject}. {e.Message}", + e + ); + } } } - _guard = new ThreadSafeSingleShotGuard(); + finally + { + _guard = new ThreadSafeSingleShotGuard(); + } }, Cancel ); diff --git a/TaskVisualization/TaskViewer.cs b/TaskVisualization/TaskViewer.cs index 4c913de9a..77dd88f58 100644 --- a/TaskVisualization/TaskViewer.cs +++ b/TaskVisualization/TaskViewer.cs @@ -254,11 +254,19 @@ protected override bool ProcessCmdKey(ref Message msg, Keys keyData) { if (keyData.HasFlag(Keys.Alt)) { - // If keyData = Keys.Up OrElse keyData = Keys.Down OrElse keyData = Keys.Left OrElse keyData = Keys.Right OrElse keyData = Keys.Alt Then + // Issue #726 finding 7: the return value of KeyboardHandler_KeyDown was previously + // discarded and this method unconditionally returned true, claiming the entire Alt + // chord class regardless of whether the handler actually consumed the key -- the + // same over-claim shape already fixed for the QuickFiler EFC/QFC surfaces (#467, + // #663). Falling through to base.ProcessCmdKey when the handler does not consume the + // key restores standard WinForms menu-mnemonic routing for Alt chords it doesn't own. object sender = FromHandle(msg.HWnd); var e = new KeyEventArgs(keyData); - _controller.KeyboardHandler_KeyDown(sender, e); - return true; + bool consumed = _controller.KeyboardHandler_KeyDown(sender, e); + if (consumed) + { + return true; + } } return base.ProcessCmdKey(ref msg, keyData);