From f816ccf3e4a97300d8b335daee84d98c85b57906 Mon Sep 17 00:00:00 2001 From: Allamagoosa <36551512+dmatlock171@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:13:30 -0700 Subject: [PATCH] Clear the review nits Deleted rather than documented: TrackedQueue's Current, Active, ClearCurrent() and MoveNext(). The first three had no callers at all, and MoveNext() had exactly one - the Avalonia design-mode preview, which now builds its sample queue from TryDequeueNext and MarkCompleted, the same calls the dispatch loop makes. The "legacy / kept for compatibility" labels pointed at callers that stopped existing when the loop stopped being sequential. TrackedQueue.Current's doc comment cited the speed limit display as its reason to exist, which stopped being true when the speed limit moved to GetActive(). SpeedLimit itself now records why the sequential loop's per-book read-back was dropped rather than leaving it unexplained: with several books running there is no single book to read it back from, and doing so would have whichever started last overwrite what the user typed. Restored the UTF-8 BOMs on ProcessQueueViewModel.cs and BadBookSessionContext.cs, and put back the eight "// " separators in the WinForms designer that had been rewritten to "//". That file's diff is now identical with and without whitespace, so everything left in it is a real change. --- .../Views/ProcessQueueControl.axaml.cs | 14 ++-- .../ProcessQueue/BadBookSessionContext.cs | 2 +- .../ProcessQueue/ProcessQueueViewModel.cs | 68 +++++++++++-------- Source/LibationUiBase/TrackedQueue[T].cs | 53 +-------------- .../ProcessQueueControl.Designer.cs | 32 ++++----- .../LibationUiBase.Tests/TrackedQueueTests.cs | 3 +- 6 files changed, 65 insertions(+), 107 deletions(-) diff --git a/Source/LibationAvalonia/Views/ProcessQueueControl.axaml.cs b/Source/LibationAvalonia/Views/ProcessQueueControl.axaml.cs index d6f872d9..7969640d 100644 --- a/Source/LibationAvalonia/Views/ProcessQueueControl.axaml.cs +++ b/Source/LibationAvalonia/Views/ProcessQueueControl.axaml.cs @@ -79,13 +79,13 @@ public partial class ProcessQueueControl : UserControl }; vm.Queue.Enqueue(testList); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); - vm.Queue.MoveNext(); + + // Six completed, one active, one still queued - the mix this preview exists to show. + // Driven through the same calls the queue loop makes, rather than the sequential + // MoveNext() that used to live here for this one caller. + for (int i = 0; i < 6 && vm.Queue.TryDequeueNext(out var finished); i++) + vm.Queue.MarkCompleted(finished); + vm.Queue.TryDequeueNext(out _); return; } #endif diff --git a/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs b/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs index ff3da416..6a2fd885 100644 --- a/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs +++ b/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs @@ -1,4 +1,4 @@ -using System.Threading; +using System.Threading; using LibationFileManager; namespace LibationUiBase.ProcessQueue; diff --git a/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs b/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs index fe2edfd4..7e624253 100644 --- a/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs +++ b/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs @@ -1,4 +1,4 @@ -using ApplicationServices; +using ApplicationServices; using DataLayer; using FileLiberator; using LibationFileManager; @@ -129,6 +129,14 @@ public class ProcessQueueViewModel : ReactiveObject public decimal SpeedLimitIncrement { get; private set; } private decimal _speedLimit; + /// + /// The sequential loop also re-read this from each book as that book started + /// (SpeedLimit = nextBook.Configuration.DownloadSpeedLimit / 1024m / 1024), which kept the + /// displayed number in step when only one book could be downloading. With several running there is + /// no single book to read it back from, and doing so would have whichever book happened to start + /// last overwrite what the user had just typed. The setter is now the only writer: it stores the + /// value once and pushes it out to every active book. + /// public decimal SpeedLimit { get => _speedLimit; @@ -573,9 +581,9 @@ public class ProcessQueueViewModel : ReactiveObject /// Moves the book being held back to the end of the queue without counting it as completed. private void RequeueLast(ProcessBookViewModel book) { - // Removes this book, not the first active one. ClearCurrent() drops Active[0], which with - // several books in flight is some other book's download: deferring the second of three - // active books would silently evict the first instead. + // Removes this book by identity, not whichever happens to be first. With several books in + // flight the first active one is some other book's download, so deferring the second of three + // would silently evict the first instead. Queue.RemoveActive(book); Queue.Enqueue([book]); } @@ -688,6 +696,20 @@ public class ProcessQueueViewModel : ReactiveObject using var counterTimer = new Timer(_ => RunningTime = timeToStr(DateTime.Now - startingTime), null, 0, 500); + // True for the one book that gets to tear the queue down, false for every book that arrives + // after it. Written under the lock and read by the dispatch loop without it, hence the + // volatile write. + bool ClaimAbort() + { + lock (resultLock) + { + if (aborted) + return false; + Volatile.Write(ref aborted, true); + return true; + } + } + async Task ProcessBookAsync(ProcessBookViewModel book) { Serilog.Log.Logger.Information("Begin processing queued item: '{item_LibraryBook}'", book.LibraryBook); @@ -717,26 +739,6 @@ public class ProcessQueueViewModel : ReactiveObject Queue.MarkCompleted(book); if (result == ProcessBookResult.FailedAbort) - { - // Same reasoning: several books can run out of disk at once. - if (tearsDownTheQueue) - await CancelAllAsync(book); - // True for the one book that gets to tear the queue down, false for every book that arrives - // after it. Written under the lock and read by the dispatch loop without it, hence the - // volatile write. - bool ClaimAbort() - { - lock (resultLock) - { - if (aborted) - return false; - Volatile.Write(ref aborted, true); - return true; - } - } - - } - else if (result == ProcessBookResult.DiskFull) { if (tearsDownTheQueue) await CancelAllAsync(book); @@ -747,6 +749,12 @@ public class ProcessQueueViewModel : ReactiveObject book.Result = ProcessBookResult.Cancelled; book.Status = ProcessBookStatus.Cancelled; } + } + else if (result == ProcessBookResult.DiskFull) + { + // Same reasoning: several books can run out of disk at once. + if (tearsDownTheQueue) + await CancelAllAsync(book); bool show; lock (resultLock) { show = !shownDiskFullMessage; shownDiskFullMessage = true; } if (show) @@ -847,8 +855,8 @@ public class ProcessQueueViewModel : ReactiveObject Serilog.Log.Logger.Information("Queue was cancelled while waiting on the daily download limit."); nextBook.Result = ProcessBookResult.Cancelled; nextBook.Status = ProcessBookStatus.Cancelled; - // The sequential loop left this to the next MoveNext(). Nothing moves this book - // off the active list now, so it is retired here. + // The sequential loop retired this book on its next step. A dispatch loop has no + // such step, so nothing else would take it off the active list - it is retired here. Queue.MarkCompleted(nextBook); continue; } @@ -887,6 +895,10 @@ public class ProcessQueueViewModel : ReactiveObject } finally { + // Scoped to the run, not to the drain. A queue parked in WaitForDailyLimitAsync only + // re-reads this every DailyLimitPollInterval, so clearing it when the last cancellation + // settles would let the gate wake up, see false, and resume the book just cancelled. + cancelAllRequested = false; DiskSpaceBackupPreflight.ResetBulkPreflightForQueueRun(); } @@ -895,7 +907,3 @@ public class ProcessQueueViewModel : ReactiveObject : $"{time.TotalHours:F0}:{time:mm\\:ss}"; } } - // Scoped to the run, not to the drain. A queue parked in WaitForDailyLimitAsync only - // re-reads this every DailyLimitPollInterval, so clearing it when the last cancellation - // settles would let the gate wake up, see false, and resume the book just cancelled. - cancelAllRequested = false; diff --git a/Source/LibationUiBase/TrackedQueue[T].cs b/Source/LibationUiBase/TrackedQueue[T].cs index 034bbad1..1677d0e4 100644 --- a/Source/LibationUiBase/TrackedQueue[T].cs +++ b/Source/LibationUiBase/TrackedQueue[T].cs @@ -33,9 +33,6 @@ public class TrackedQueue : IReadOnlyCollection, IList, INotifyCollectionC public event EventHandler? QueuedCountChanged; public event NotifyCollectionChangedEventHandler? CollectionChanged; - /// Returns the first active item for backward compatibility (e.g. speed limit display). - public T? Current => _active.FirstOrDefault(); - public IReadOnlyList Active => _active; public IReadOnlyList Completed => _completed; private List Queued { get; } = new(); @@ -317,7 +314,7 @@ public class TrackedQueue : IReadOnlyCollection, IList, INotifyCollectionC } /// - /// The active items, copied under the lock. is the live list, so enumerating + /// The active items, copied under the lock. The backing list is live, so enumerating /// it while book tasks start and finish throws; callers that need to iterate use this. /// public IReadOnlyList GetActive() @@ -326,22 +323,6 @@ public class TrackedQueue : IReadOnlyCollection, IList, INotifyCollectionC return _active.ToList(); } - /// Legacy single-item sequential accessor — kept for compatibility. - public void ClearCurrent() - { - lock (lockObject) - { - var first = _active.FirstOrDefault(); - if (first != null) - { - int displayIndex = _completed.Count; - _active.Remove(first); - Pend(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, first, displayIndex)); - } - } - DispatchPending(); - } - public void ClearQueue() { lock (lockObject) @@ -389,38 +370,6 @@ public class TrackedQueue : IReadOnlyCollection, IList, INotifyCollectionC DispatchPending(); } - /// - /// Legacy sequential MoveNext — completes the first active item and dequeues the next. - /// Only valid when at most one item is active at a time. - /// - public bool MoveNext() - { - try - { - lock (lockObject) - { - var oldActive = _active.FirstOrDefault(); - if (oldActive != null) - { - _active.Remove(oldActive); - _completed.Add(oldActive); - Pend(_completed.Count); - } - if (Queued.Count == 0) - return false; - var next = Queued[0]; - Queued.RemoveAt(0); - _active.Add(next); - PendQueued(Queued.Count); - return true; - } - } - finally - { - DispatchPending(); - } - } - public void Enqueue(IList item) { lock (lockObject) diff --git a/Source/LibationWinForms/ProcessQueue/ProcessQueueControl.Designer.cs b/Source/LibationWinForms/ProcessQueue/ProcessQueueControl.Designer.cs index 100a01e3..5bbabd93 100644 --- a/Source/LibationWinForms/ProcessQueue/ProcessQueueControl.Designer.cs +++ b/Source/LibationWinForms/ProcessQueue/ProcessQueueControl.Designer.cs @@ -171,9 +171,9 @@ this.panel1.Name = "panel1"; this.panel1.Size = new System.Drawing.Size(390, 52); this.panel1.TabIndex = 2; - // + // // queueSettingsTable - // + // this.queueSettingsTable.ColumnCount = 3; this.queueSettingsTable.ColumnStyles.Add(new System.Windows.Forms.ColumnStyle(System.Windows.Forms.SizeType.AutoSize)); this.queueSettingsTable.ColumnStyles.Add(new System.Windows.Forms.ColumnStyle(System.Windows.Forms.SizeType.Absolute, 70F)); @@ -193,9 +193,9 @@ this.queueSettingsTable.RowStyles.Add(new System.Windows.Forms.RowStyle(System.Windows.Forms.SizeType.Percent, 50F)); this.queueSettingsTable.Size = new System.Drawing.Size(220, 50); this.queueSettingsTable.TabIndex = 9; - // + // // autoScrollChk - // + // this.autoScrollChk.Anchor = System.Windows.Forms.AnchorStyles.Left; this.autoScrollChk.AutoSize = true; this.autoScrollChk.Checked = true; @@ -205,9 +205,9 @@ this.autoScrollChk.TabIndex = 6; this.autoScrollChk.Text = "Auto-scroll"; this.autoScrollChk.UseVisualStyleBackColor = true; - // + // // concurrencyLbl - // + // this.concurrencyLbl.Anchor = System.Windows.Forms.AnchorStyles.Left | System.Windows.Forms.AnchorStyles.Right; this.concurrencyLbl.AutoSize = false; this.concurrencyLbl.Name = "concurrencyLbl"; @@ -215,16 +215,16 @@ this.concurrencyLbl.TextAlign = System.Drawing.ContentAlignment.MiddleRight; this.concurrencyLbl.TabIndex = 7; this.concurrencyLbl.Text = "At once:"; - // + // // concurrencyNum - // + // this.concurrencyNum.Anchor = System.Windows.Forms.AnchorStyles.Left; this.concurrencyNum.Name = "concurrencyNum"; this.concurrencyNum.Size = new System.Drawing.Size(84, 23); this.concurrencyNum.TabIndex = 8; - // + // // label1 - // + // this.label1.Anchor = System.Windows.Forms.AnchorStyles.Left | System.Windows.Forms.AnchorStyles.Right; this.label1.AutoSize = false; this.label1.Name = "label1"; @@ -232,9 +232,9 @@ this.label1.TextAlign = System.Drawing.ContentAlignment.MiddleRight; this.label1.TabIndex = 5; this.label1.Text = "DL Limit:"; - // + // // numericUpDown1 - // + // this.numericUpDown1.Anchor = System.Windows.Forms.AnchorStyles.Left; this.numericUpDown1.DecimalPlaces = 1; this.numericUpDown1.Increment = new decimal(new int[] { @@ -260,9 +260,9 @@ 0, 0}); this.numericUpDown1.ValueChanged += new System.EventHandler(this.numericUpDown1_ValueChanged); - // + // // btnCleanFinished - // + // this.btnCleanFinished.Dock = System.Windows.Forms.DockStyle.Right; this.btnCleanFinished.Location = new System.Drawing.Point(298, 0); this.btnCleanFinished.Name = "btnCleanFinished"; @@ -271,9 +271,9 @@ this.btnCleanFinished.Text = "Clear Finished"; this.btnCleanFinished.UseVisualStyleBackColor = true; this.btnCleanFinished.Click += new System.EventHandler(this.btnClearFinished_Click); - // + // // cancelAllBtn - // + // this.cancelAllBtn.Dock = System.Windows.Forms.DockStyle.Left; this.cancelAllBtn.Location = new System.Drawing.Point(0, 0); this.cancelAllBtn.Name = "cancelAllBtn"; diff --git a/Source/_Tests/LibationUiBase.Tests/TrackedQueueTests.cs b/Source/_Tests/LibationUiBase.Tests/TrackedQueueTests.cs index 905667ea..b3b634a3 100644 --- a/Source/_Tests/LibationUiBase.Tests/TrackedQueueTests.cs +++ b/Source/_Tests/LibationUiBase.Tests/TrackedQueueTests.cs @@ -183,7 +183,8 @@ public class TrackedQueueTests public void deferring_an_active_book_sends_it_to_the_back_and_leaves_the_others_running() { // What the daily download limit does when it holds a book back: the book being deferred is - // removed and re-queued. ClearCurrent() would have dropped A, some other book's download. + // removed by identity and re-queued. Removing whichever was first would have dropped A, some + // other book's download. Book a = new("A"), b = new("B"), c = new("C"); var queue = QueueOf(a, b, c); queue.TryDequeueNext(out _);