diff --git a/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs b/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs index 6a2fd885..e2cb1a88 100644 --- a/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs +++ b/Source/LibationUiBase/ProcessQueue/BadBookSessionContext.cs @@ -14,5 +14,22 @@ public class BadBookSessionContext /// internal SemaphoreSlim DialogGate { get; } = new(1, 1); - public void Reset() => Override = null; + /// + /// The book the user actually answered "Abort" for, when one was asked. Null when the abort came + /// from the Bad Book setting rather than a dialog, and null when nothing has aborted. + /// + /// + /// Recorded because tearing the queue down and reporting the abort are different jobs. The + /// teardown is claimed by whichever book finishes first and the books are interchangeable for + /// it; the status left on a row is read afterwards by a person who remembers which book they + /// were asked about, and "Cancelled" on that row with "Error, Abort" on some other book's is + /// the wrong way round. + /// + public ProcessBookViewModel? AbortOriginator { get; set; } + + public void Reset() + { + Override = null; + AbortOriginator = null; + } } diff --git a/Source/LibationUiBase/ProcessQueue/ProcessBookViewModel.cs b/Source/LibationUiBase/ProcessQueue/ProcessBookViewModel.cs index bcf3da20..5f6d4ad8 100644 --- a/Source/LibationUiBase/ProcessQueue/ProcessBookViewModel.cs +++ b/Source/LibationUiBase/ProcessQueue/ProcessBookViewModel.cs @@ -533,6 +533,14 @@ public class ProcessBookViewModel : ReactiveObject { var result = await BadBookActionDialogBase.Show(skipDialogText, "Skip this book?"); + // This is the book the user was looking at when they said stop, so this is the book whose + // row should say so afterwards. Every other book in flight inherits the answer through + // Override below and was cancelled by it, which is a different thing and should read as + // one. Written before Override, not after: a book reading Override and racing ahead to + // the queue loop must not find the originator still unset and conclude there wasn't one. + if (result.Action is DialogResult.Abort && _badBookSession is not null) + _badBookSession.AbortOriginator = this; + // Abort is a statement about the run, not about this one book, so it becomes the session // answer whether or not "apply to all" was checked. Without this a user who aborts with // several books in flight is asked the same question again by every one of them, and the diff --git a/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs b/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs index 85c10efc..0862bca5 100644 --- a/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs +++ b/Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs @@ -29,6 +29,13 @@ public class ProcessQueueViewModel : ReactiveObject public ObservableCollection LogEntries { get; } = new(); public TrackedQueue Queue { get; } = new(); private readonly BadBookSessionContext _badBookSession = new(); + + /// + /// Internal rather than private so a test can stand in for a user having answered the bad book + /// dialog, which the dispatch tests otherwise never reach - their books finish on command instead + /// of failing their way into it. + /// + internal BadBookSessionContext BadBookSession => _badBookSession; public Task? QueueRunner { get; private set; } public bool Running => !QueueRunner?.IsCompleted ?? false; @@ -756,10 +763,21 @@ public class ProcessQueueViewModel : ReactiveObject { if (tearsDownTheQueue) await CancelAllAsync(book); - else + + // Tearing the queue down and reporting the abort are separate jobs, and only the + // first one is indifferent to which book does it. The status left on a row is read + // afterwards by someone who remembers which book they were asked about, so the + // abort belongs to the book they answered for - not to whichever book happened to + // reach the teardown first, which put "Cancelled" on the row they aborted and + // "Error, Abort" on an unrelated one. With no dialog in play (Bad Book set to + // Abort in settings) nobody answered anything, and the book that claimed the + // teardown keeps the abort as before. + var originator = _badBookSession.AbortOriginator; + var reportsTheAbort = originator is null ? tearsDownTheQueue : ReferenceEquals(originator, book); + + if (!reportsTheAbort) { - // Inherited the abort rather than triggering the teardown. It was cancelled by - // the book that did, and that is what it should report. + // Cancelled by the abort rather than the cause of it. book.Result = ProcessBookResult.Cancelled; book.Status = ProcessBookStatus.Cancelled; } diff --git a/Source/_Tests/LibationUiBase.Tests/ProcessQueueDispatchTests.cs b/Source/_Tests/LibationUiBase.Tests/ProcessQueueDispatchTests.cs index b8d023dc..672b160b 100644 --- a/Source/_Tests/LibationUiBase.Tests/ProcessQueueDispatchTests.cs +++ b/Source/_Tests/LibationUiBase.Tests/ProcessQueueDispatchTests.cs @@ -264,8 +264,11 @@ public class ProcessQueueDispatchTests queue.AddToQueue([a, b, c]); await books.WaitForStarted(3); - // All three inherit the abort, which is what a session-wide Abort answer produces. Only the - // first through gets to tear the queue down; the others were cancelled by it. + // No dialog was answered here - this is Bad Book set to Abort in settings, where every book + // that fails aborts on its own account. Nobody is the one the user was asked about, so the + // first book through tears the queue down and keeps the abort; the others were cancelled by it. + Assert.IsNull(queue.BadBookSession.AbortOriginator); + books.Finish("A", ProcessBookResult.FailedAbort); books.Finish("B", ProcessBookResult.FailedAbort); books.Finish("C", ProcessBookResult.FailedAbort); @@ -278,6 +281,36 @@ public class ProcessQueueDispatchTests Assert.AreEqual(2, cancelled, "Books that inherited the abort should report as cancelled."); } + [TestMethod] + public async Task the_book_the_user_aborted_reports_the_abort_whichever_book_tears_the_queue_down() + { + var (queue, books) = NewQueue(atOnce: 3); + + var a = Book("A"); + var b = Book("B"); + var c = Book("C"); + queue.AddToQueue([a, b, c]); + await books.WaitForStarted(3); + + // C is the book the user was looking at when they answered Abort. A and B inherit that answer + // through the session override, which is what puts all three here reporting FailedAbort. + queue.BadBookSession.AbortOriginator = c; + + // A finishes first and so claims the teardown. Nothing below depends on it winning - that race + // is what made the old status arbitrary - but this is the ordering that used to leave the row + // the user actually aborted saying "Cancelled" while A's said "Error, Abort". + books.Finish("A", ProcessBookResult.FailedAbort); + await Task.Delay(50); + books.Finish("B", ProcessBookResult.FailedAbort); + books.Finish("C", ProcessBookResult.FailedAbort); + + await RunToCompletion(queue); + + Assert.AreEqual(ProcessBookResult.FailedAbort, c.Result, "The book the user aborted did not report the abort."); + Assert.AreEqual(ProcessBookResult.Cancelled, a.Result, "A book that inherited the abort reported it as its own."); + Assert.AreEqual(ProcessBookResult.Cancelled, b.Result, "A book that inherited the abort reported it as its own."); + } + [TestMethod] public async Task a_book_that_throws_is_logged_and_the_rest_of_the_queue_still_finishes() {