diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 39bc1df..e2777f6 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -4,9 +4,9 @@ This is the current ledger for defects and review findings in EncodingChecker. It is organised by status, not discovery date, so the open work is visible in one place. Longer evidence and history follow the ledger. - + -**Derived count: 82 findings — 72 fixed, 6 open, 1 not reproduced, 1 withdrawn, +**Derived count: 83 findings — 73 fixed, 6 open, 1 not reproduced, 1 withdrawn, 1 intentional behavior, and 1 design decision.** Recompute and check these figures with: @@ -89,6 +89,7 @@ the 2026-09-08 reformat to findings that previously had only a sentence. | BL-36 | A conversion refused for a changed file still replaced its earlier backup | Fixed | — | — | [BL-36](#bl-36) | | BL-37 | A contradicting source choice that was also the target was not refused | Fixed | — | — | [BL-37](#bl-37) | | BL-38 | `-Validate utf-8` rejected ASCII files that conversion treats as already UTF-8 | Fixed | — | — | [BL-38](#bl-38) | +| BL-39 | A source chosen in a cancelled GUI review carried into the next review | Fixed | — | — | [BL-39](#bl-39) | | EC-32 | A cancellation timeout could hide repeated refusals to press Cancel | Fixed | — | — | [EC-32](#ec-32) | | EC-15 | Plans and journals showed fixed safety flags as if they recorded checks | Fixed | — | — | [EC-15](#ec-15) | | BL-18 | BOM-less UTF-16 could be detected and converted as UTF-32 | Fixed | — | — | [BL-18](#bl-18) | @@ -1403,6 +1404,24 @@ as ASCII, so a non-ASCII byte past the detection sample is reported as invalid, as conversion reports it. Tests cover each list, the explanation and the late byte. +### BL-39 + +**A source chosen in a GUI review that ends without writing no longer carries into +the next review.** Choosing a source for a refused file updates that row, so the +review can show it as ready. The rows survive between runs, and cancelling left +the choice on them: the next Convert showed the file as ready to convert with a +source nobody chose in that review, while the list still showed the detected +encoding. Found by a 2026-09-20 review and confirmed through the orchestration +sequence; no file was written. + +The choices are now saved when the review opens and put back when it ends without +writing: cancelled, stale, or not plannable, or cancelled before the write pass. +Choices that existed before the review opened are kept. A completed or +interrupted run keeps the choices it wrote with. Tests choose and then cancel, +and run the same rows again, keep an earlier choice, and cover a review that went +stale; removing the restore, clearing every choice, or restoring only on +cancellation each fails them. + ## Decisions and mistakes that must remain visible ### A known defect shipped after being reported closed diff --git a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs index 6cfee22..66b77bf 100644 --- a/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs +++ b/sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs @@ -258,6 +258,124 @@ public void Ascii_IsConvertedAutomatically() Assert.Equal(text, Encoding.Unicode.GetString(File.ReadAllBytes(path))); } + // ------------------------------------------------------ abandoned source choices + + private static ConfirmationResponse Choose(string charset, string path) => + new(ConfirmationChoice.ChooseSourceEncoding, charset, [path]); + + // The plan the next review would open with, read without writing anything. + private PlannedFile NextReviewShows(List rows, string path) + { + ConversionPlan? shown = null; + + Convert(rows, plan => + { + shown = plan; + return ConfirmationResponse.Cancel; + }); + + return Assert.Single( + shown!.Files, f => f.RelativePath == Path.GetFileName(path)); + } + + [Fact] + public void ChoosingASourceThenCancelling_LeavesNoChoiceForTheNextReview() + { + string path = Write("french.txt", "Le café était déjà prêt", "windows-1252"); + byte[] before = File.ReadAllBytes(path); + + List rows = View(); + ConversionReportEntry row = Assert.Single(rows); + string? labelBefore = row.CurrentCharsetLabel; + + int shown = 0; + OrchestrationResult result = Convert(rows, plan => + { + if (++shown == 1) + return Choose("windows-1252", path); + + // The choice took effect inside this review... + Assert.Equal(PlannedAction.Convert, Assert.Single(plan.Files).Action); + + // ...and the user then cancels it. + return ConfirmationResponse.Cancel; + }); + + Assert.Equal(OrchestrationOutcome.Cancelled, result.Outcome); + Assert.Equal(before, File.ReadAllBytes(path)); + Assert.False(row.SourceEncodingWasSpecified); + Assert.Equal(labelBefore, row.CurrentCharsetLabel); + + // A second run on the same rows asks for the source again instead of calling the + // file ready with a choice nobody made in that review. + PlannedFile next = NextReviewShows(rows, path); + Assert.Equal(PlannedAction.Refuse, next.Action); + Assert.True(next.NeedsSourceChoice); + Assert.Equal(before, File.ReadAllBytes(path)); + } + + [Fact] + public void CancellingAReviewKeepsAChoiceThatExistedBeforeItOpened() + { + string kept = Write("kept.txt", "Le café était déjà prêt", "windows-1252"); + string chosen = Write("chosen.txt", "Привет мир, это русский текст", "koi8-r"); + + List rows = View(); + + // A choice left by an earlier review, as a run interrupted after it began writing + // leaves one on the rows it did not reach. + ConversionReportEntry keptRow = Assert.Single(rows, r => r.FilePath == kept); + keptRow.CurrentCharsetLabel = "windows-1252"; + keptRow.SourceEncodingWasSpecified = true; + + ConversionReportEntry chosenRow = Assert.Single(rows, r => r.FilePath == chosen); + + int shown = 0; + Convert(rows, _ => ++shown == 1 + ? Choose("koi8-r", chosen) + : ConfirmationResponse.Cancel); + + Assert.True(keptRow.SourceEncodingWasSpecified); + Assert.Equal("windows-1252", keptRow.CurrentCharsetLabel); + Assert.False(chosenRow.SourceEncodingWasSpecified); + + PlannedFile keptNext = NextReviewShows(rows, kept); + Assert.Equal(PlannedAction.Convert, keptNext.Action); + Assert.True(keptNext.SourceWasSpecified); + Assert.Equal("windows-1252", keptNext.SourceEncoding); + + PlannedFile chosenNext = NextReviewShows(rows, chosen); + Assert.Equal(PlannedAction.Refuse, chosenNext.Action); + Assert.True(chosenNext.NeedsSourceChoice); + } + + [Fact] + public void AChoiceInAReviewThatWentStaleIsNotKeptEither() + { + // Proceeding does not make the choice used: the file changed, nothing was written, + // and the next review starts from the file as it is. + string path = Write("french.txt", "Le café était déjà prêt", "windows-1252"); + + List rows = View(); + ConversionReportEntry row = Assert.Single(rows); + + int shown = 0; + OrchestrationResult result = Convert(rows, _ => + { + if (++shown == 1) + return Choose("windows-1252", path); + + File.WriteAllBytes( + path, Encoding.GetEncoding("windows-1252").GetBytes("Le café a changé")); + + return ConfirmationResponse.Proceed; + }); + + Assert.Equal(OrchestrationOutcome.PlanWentStale, result.Outcome); + Assert.False(row.SourceEncodingWasSpecified); + Assert.False(File.Exists(path + ".bak")); + } + // ------------------------------------------------------------ explicit source [Fact] diff --git a/sources/EncodingChecker/ConversionOrchestrator.cs b/sources/EncodingChecker/ConversionOrchestrator.cs index 4155077..43a1d2f 100644 --- a/sources/EncodingChecker/ConversionOrchestrator.cs +++ b/sources/EncodingChecker/ConversionOrchestrator.cs @@ -123,11 +123,28 @@ internal OrchestrationResult Run( foreach (ConversionReportEntry entry in entries) entry.ResetAttemptEvidence(); + // A source chosen in a review that ends without writing was never used. Kept, it + // would show those files as ready in the next review without being chosen there. + // Choices that existed before this review opened are what gets restored. + (string? Label, bool Specified)[] choicesBefore = + [ + .. entries.Select(e => (e.CurrentCharsetLabel, e.SourceEncodingWasSpecified)), + ]; + try { - return DecideConfirmAndRun( + OrchestrationResult result = DecideConfirmAndRun( entries, baseDirectory, targetCharset, targetWriteBom, backup, preview, maxParallelism, onEntry, cancellationToken, startedUtc); + + if (result.Outcome is OrchestrationOutcome.Cancelled + or OrchestrationOutcome.PlanWentStale + or OrchestrationOutcome.CouldNotPlan) + { + RestoreSourceChoices(entries, choicesBefore); + } + + return result; } catch (OperationCanceledException) { @@ -136,10 +153,22 @@ internal OrchestrationResult Run( // own cancellation handling that returns normally instead of throwing, so it // never reaches this catch; only a decide-phase cancellation does. ConversionReportEntry.MarkUnattempted(entries, []); + RestoreSourceChoices(entries, choicesBefore); throw; } } + private static void RestoreSourceChoices( + IReadOnlyList entries, + (string? Label, bool Specified)[] choices) + { + for (int i = 0; i < entries.Count; i++) + { + entries[i].CurrentCharsetLabel = choices[i].Label; + entries[i].SourceEncodingWasSpecified = choices[i].Specified; + } + } + /// /// The decide/confirm/write sequence, split out so can wrap all of /// it in one cancellation handler.