From 4c5f5e72e2ce0bd148e1d7b8f0435021fe22c91f Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 27 Aug 2026 05:15:50 +0300 Subject: [PATCH 1/2] One policy engine, and a GUI that asks before it writes Building the GUI on the same model as the CLI turned up the reason it needed to be: the GUI was not applying the ambiguity refusal at all. Classification ran only during a Convert-mode scan. The GUI scans in Detect mode and converts the rows the user checks, so every entry reached conversion carrying the default "unambiguous" and the gate never fired. The tool converted, on the strength of whatever detection returned, the exact files it tells CLI users it will not convert. Nothing failed, because no test drove the GUI's sequence - View, then Convert - as a sequence. ConversionPolicyTests now does, and the first version of that test failed on the classification before it ever reached the result. So the decision moves into ConversionPolicy, and every route asks it: the CLI's Convert scan, a written plan, and the GUI. It is asked at the point of decision rather than during the scan, which is what makes it reachable from all three. Entries carry the answer, and an entry that reaches a plan without one now raises rather than defaulting to Convert - defaulting is the exact shape of the bug. Applying a plan still does not classify again. An entry that arrives already decided keeps its decision, so -Apply re-asserts the gate from what the plan recorded instead of recomputing it, which is the property the plan exists for. The GUI's Convert now runs twice: a WhatIf pass that decides, and a pass that carries out what was confirmed. Both use the same entry objects, so the second does not classify anything - the conversion that happens is the one that was shown. A preview run is its own answer and skips the dialog, since it writes nothing to confirm. The dialog keeps the three outcomes apart: an encoding the bytes determine, several codecs that agree on the text, several that disagree. Only the third is refused, and for it the dialog names the encodings actually in conflict and offers the one thing that resolves it. That selection is the GUI's -From: it replaces detection for those files and nothing else. Strict decoding, output verification and the backup requirement all still apply, and there are tests for each. Three test files that constructed entries directly began failing once the gate reached them. They were right to: they name a source encoding rather than having it detected, which is what -From means, and they now say so. Also covers the dialog itself, on an STA thread, against real plans - every outcome mix, nothing-refused, everything-refused, and that it reports the plan it was handed rather than recounting a directory that has since changed. It is the only part of the safety model a GUI user reads and was the only part no test had ever executed. 402 passing. Co-Authored-By: Claude Opus 5 --- README.md | 49 +- .../ConversionConfirmationFormTests.cs | 241 ++++++++++ .../ConversionMetadataTests.cs | 7 +- .../ConversionPolicyTests.cs | 419 ++++++++++++++++++ .../StaleConversionStateTests.cs | 7 +- .../ConversionConfirmationForm.cs | 355 +++++++++++++++ sources/EncodingChecker/ConversionPlan.cs | 29 +- sources/EncodingChecker/ConversionPolicy.cs | 90 ++++ sources/EncodingChecker/ConversionReport.cs | 31 ++ sources/EncodingChecker/MainForm.cs | 135 +++++- sources/EncodingChecker/Program.cs | 4 +- sources/EncodingChecker/ScanEngine.cs | 80 ++-- 12 files changed, 1388 insertions(+), 59 deletions(-) create mode 100644 sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs create mode 100644 sources/EncodingChecker.Tests/ConversionPolicyTests.cs create mode 100644 sources/EncodingChecker/ConversionConfirmationForm.cs create mode 100644 sources/EncodingChecker/ConversionPolicy.cs diff --git a/README.md b/README.md index d1318bb..c543290 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,8 @@ Each [release](https://github.com/amrali-eg/EncodingChecker/releases) publishes - Layered detection: byte-order-mark and heuristic checks for Unicode encodings, [UtfUnknown](https://github.com/CharsetDetector/UTF-unknown) for legacy code pages, each candidate independently verified by strict decoding before being trusted. - Lossless, safe conversion: every write is verified afterward by comparing a SHA-256 hash of the decoded content, so a silent encoder substitution (e.g. an unrepresentable character) is caught and reported as an error instead of corrupting the file. -- Refuses to convert files whose encoding the bytes do not determine, naming the encodings actually in conflict, with `-From` to supply the answer yourself. +- Refuses to convert files whose encoding the bytes do not determine, naming the encodings actually in conflict, with `-From` (or the GUI's source-encoding selection) to supply the answer yourself. One policy engine decides for every surface. +- The GUI confirms before writing, showing exactly what will happen to each file and carrying out that same plan rather than re-deciding. - `-Plan`/`-Apply` preflight: review what a conversion would do, then carry out exactly that — the plan is bound to the files' hashes and is refused whole if they change. - Optional `.bak` backup before overwriting, and a `-WhatIf` dry-run mode that reports what would happen without touching any file. - Covered by an xUnit test suite exercising the detection/conversion engine, CLI argument parsing, and CSV report formatting across multilingual content and edge cases. @@ -28,6 +29,43 @@ Two options apply to **Convert**: - **Back up original files before converting (.bak)** — keeps each original as `.bak` before it is replaced. The equivalent of the CLI's `-Backup`. - **Preview changes without modifying files** — reports which files *would* be converted without writing anything and without creating any `.bak`. Previewed rows keep their current encoding and stay selected, so you can review the result and then convert for real. The equivalent of the CLI's `-WhatIf`. +### Confirming a conversion + +**Convert** does not write anything immediately. It first works out what would happen to +every selected file, then shows that for approval: + +``` +Convert 417 of 480 selected file(s) to utf-8 without BOM + + 386 Encoding determined by the file's own bytes Will convert. + 31 Encoding undetermined, every reading agrees on the text Will convert; the label + is a choice, the content + is not. + 22 Encoding undetermined, readings disagree on the text WILL NOT be converted. + 39 Already in the target encoding Nothing to do. + 2 Encoding could not be identified Left alone. + +Directory C:\Source +Source encoding detected per file +Backups enabled — each original kept as .bak +Guarantees strict codecs, verified output, atomic install, ambiguity refusal +``` + +The conversion that runs is the one shown. Nothing is detected a second time between the +confirmation and the writing, so the dialog cannot describe one set of conclusions while +a different set is carried out — the same property `-Apply` has. + +When files are refused, the dialog names the encodings actually in conflict and offers the +one thing that resolves it: saying which encoding they are. That selection is the GUI's +`-From`. It replaces detection for those files and nothing else — the bytes must still +decode strictly as the chosen encoding, the output is still verified to hold exactly the +same text, and a failed backup still stops the conversion. + +The GUI and the CLI ask the same question of the same code. There is one policy engine +([`ConversionPolicy`](sources/EncodingChecker/ConversionPolicy.cs)); detection or an +explicit source produces a classification, the classification produces an action, and +every surface acts on that action rather than reaching its own conclusion. + ## Command-line usage Launch `EncodingChecker.exe` with arguments to run in console mode instead. Run `EncodingChecker.exe -?` (or `-h`, `/?`, `--help`) at any time to print this from the tool itself. @@ -219,8 +257,13 @@ These are the guarantees the implementation actually provides. opt-in, since a script can keep the report. - A conversion whose source encoding cannot be determined from the file's own bytes is refused rather than guessed at, when the competing encodings would - produce different text. `-From` overrides the detection, not the conversion - safeguards. See [Ambiguous encodings](#ambiguous-encodings-and--from). + produce different text. `-From`, and the GUI's source-encoding selection, + override the detection, not the conversion safeguards. + See [Ambiguous encodings](#ambiguous-encodings-and--from). +
That decision is made in one place for every surface, so the GUI and the + CLI cannot diverge on what is safe. They previously could, and did: the + classification ran only during a Convert-mode scan, the GUI scans in Detect + mode, and so the GUI converted the files the CLI refused. - A plan written by `-Plan` is bound to the SHA-256 of every file it schedules, to the directory those files are under, and to the conversion behaviour it was approved under. `-Apply` verifies all of them before writing anything and diff --git a/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs new file mode 100644 index 0000000..55c37a8 --- /dev/null +++ b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs @@ -0,0 +1,241 @@ +using System.Text; +using System.Windows.Forms; + +namespace EncodingChecker.Tests; + +/// +/// The confirmation dialog is the only part of the safety model a GUI user ever reads, +/// and until now it was also the only part no test had ever executed. Layout code that +/// has never run is layout code that throws the first time somebody converts a directory +/// with an unusual mix of outcomes — and it would throw at exactly the moment the user is +/// being asked to approve something. +/// +/// These build it against real plans rather than asserting on pixels: every outcome mix, +/// on an STA thread, checking that it constructs and that what it says matches the plan +/// it was given. +/// +public sealed class ConversionConfirmationFormTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec_dialog_").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + /// Runs on an STA thread, as WinForms requires. + private static void OnUiThread(Action body) + { + Exception? failure = null; + + var thread = new Thread(() => + { + try + { + body(); + } + catch (Exception ex) + { + failure = ex; + } + }); + + thread.SetApartmentState(ApartmentState.STA); + thread.Start(); + + Assert.True(thread.Join(TimeSpan.FromSeconds(30)), "the dialog did not finish"); + + if (failure is not null) + throw new Xunit.Sdk.XunitException($"the dialog threw: {failure}"); + } + + private void Write(string name, string text, string charset) => + File.WriteAllBytes( + Path.Combine(_root, name), Encoding.GetEncoding(charset).GetBytes(text)); + + /// A plan over whatever is currently in the directory. + private ConversionPlan Plan(bool backup = true, string target = "utf-8") + { + var entries = new List(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + IncludeSubdirectories = true, + IncludePatterns = ["*"], + Action = ScanAction.Convert, + TargetCharset = target, + TargetWriteBom = false, + WhatIf = true, + }, + entries.Add, + CancellationToken.None); + + return ConversionPlan.FromEntries( + entries, _root, target, targetHasBom: false, + backupEnabled: backup, explicitSource: null); + } + + private static IEnumerable Descendants(Control root) + { + foreach (Control child in root.Controls) + { + yield return child; + + foreach (Control nested in Descendants(child)) + yield return nested; + } + } + + private static string AllText(Control root) => + string.Join("\n", Descendants(root).Select(c => c.Text)); + + [Fact] + public void ItBuildsForAMixOfEveryOutcome() + { + Write("jp.txt", "こんにちは世界。日本語のテキストです。", "shift_jis"); + Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + Write("plain.txt", "just ascii here", "ascii"); + Write("already.txt", "already utf-8 世界", "utf-8"); + + ConversionPlan plan = Plan(); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + Assert.NotEmpty(Descendants(form).ToList()); + }); + } + + [Fact] + public void ItBuildsWhenNothingIsRefused() + { + // The common case, and the one where a refusal panel must not appear at all. + Write("jp.txt", "こんにちは世界。日本語のテキストです。", "shift_jis"); + + ConversionPlan plan = Plan(); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + Assert.DoesNotContain("could not be determined", AllText(form)); + }); + } + + [Fact] + public void ItBuildsWhenEverythingIsRefused() + { + // Nothing to convert. The button has to say so rather than offering an action + // that would do nothing. + Write("a.txt", "Le café était déjà prêt", "windows-1252"); + Write("b.txt", "Привет мир, это русский", "koi8-r"); + + ConversionPlan plan = Plan(); + + Assert.All(plan.Files, f => Assert.Equal(PlannedAction.Refuse, f.Action)); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + string text = AllText(form); + + Assert.Contains("could not be determined", text); + Assert.Contains("Nothing to convert", text); + Assert.DoesNotContain("Convert 1 file", text); + }); + } + + [Fact] + public void ItNamesTheCompetingEncodingsRatherThanJustReportingLowConfidence() + { + // "Could not be determined" on its own gives a user nothing to act on. The + // alternatives and the way out are what make the refusal actionable. + Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + + ConversionPlan plan = Plan(); + PlannedFile refused = Assert.Single(plan.Files); + + Assert.NotEmpty(refused.CompetingEncodings); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + List cells = + [ + .. Descendants(form) + .OfType() + .SelectMany(v => v.Items.Cast()) + .SelectMany(i => i.SubItems.Cast()) + .Select(sub => sub.Text) + ]; + + Assert.Contains("ambiguous.txt", cells); + Assert.Contains(cells, c => c.Contains(refused.CompetingEncodings[0])); + + // And the way out is offered, populated from the charsets EC supports. + ComboBox chooser = Assert.Single(Descendants(form).OfType()); + + Assert.True(chooser.Items.Count > 1); + Assert.Contains("windows-1252", chooser.Items.Cast()); + }); + } + + [Fact] + public void ItSaysWhetherOriginalsWillBeKept() + { + // Whether a conversion is undoable is part of what is being approved. + Write("jp.txt", "こんにちは世界。テキスト", "shift_jis"); + + ConversionPlan withBackups = Plan(backup: true); + ConversionPlan without = Plan(backup: false); + + OnUiThread(() => + { + using var kept = new ConversionConfirmationForm(withBackups); + using var lost = new ConversionConfirmationForm(without); + + Assert.Contains(".bak", AllText(kept)); + Assert.Contains("DISABLED", AllText(lost)); + }); + } + + [Fact] + public void ItReportsThePlanItWasGivenRatherThanRecountingTheDirectory() + { + // The dialog must describe the decisions that will execute. Recomputing here is + // how a confirmation ends up describing something other than what happens. + Write("jp.txt", "こんにちは世界。日本語のテキストです。", "shift_jis"); + Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + + ConversionPlan plan = Plan(); + + int convert = plan.Files.Count(f => f.Action == PlannedAction.Convert); + + // Change the directory after planning. The dialog must not notice. + File.Delete(Path.Combine(_root, "jp.txt")); + Write("late-arrival.txt", "added after the plan", "ascii"); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + string text = AllText(form); + + Assert.Contains($"Convert {convert} of {plan.Files.Count} selected", text); + Assert.DoesNotContain("late-arrival", text); + }); + } +} diff --git a/sources/EncodingChecker.Tests/ConversionMetadataTests.cs b/sources/EncodingChecker.Tests/ConversionMetadataTests.cs index 6de01ef..434adb9 100644 --- a/sources/EncodingChecker.Tests/ConversionMetadataTests.cs +++ b/sources/EncodingChecker.Tests/ConversionMetadataTests.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; using System.Text.Json; namespace EncodingChecker.Tests; @@ -41,6 +41,11 @@ private string Convert(string name, string text, string sourceCharset, string ta SourceHasBom = false, TargetEncoding = sourceCharset, TargetHasBom = false, + + // These name the source encoding rather than having it detected, which is + // what -From means. Without saying so, the ambiguity gate correctly refuses + // single-byte content whose encoding its bytes do not identify. + SourceEncodingWasSpecified = true, }; ScanEngine.ConvertFiles( diff --git a/sources/EncodingChecker.Tests/ConversionPolicyTests.cs b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs new file mode 100644 index 0000000..fb49c14 --- /dev/null +++ b/sources/EncodingChecker.Tests/ConversionPolicyTests.cs @@ -0,0 +1,419 @@ +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// One policy engine, asked by every surface. +/// +/// This exists because the GUI had quietly grown a second answer. Ambiguity was +/// classified only during a Convert-mode scan; the GUI scans in Detect mode and converts +/// the rows the user checks, so every entry arrived carrying the default "unambiguous" +/// and the refusal never fired. The tool converted, on the strength of whatever detection +/// returned, the exact files it tells CLI users it will not convert — and nothing failed, +/// because no test drove the GUI's sequence. +/// +/// The lesson is the one the audit already taught once: a safety rule enforced at one +/// call site is a safety rule the next call site does not have. So the decision lives in +/// , and these pin that every route reaches it. +/// +public sealed class ConversionPolicyTests : IDisposable +{ + private readonly string _root = + Directory.CreateTempSubdirectory("ec_policy_").FullName; + + public void Dispose() + { + try + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private string Write(string name, string text, string charset) + { + string path = Path.Combine(_root, name); + File.WriteAllBytes(path, Encoding.GetEncoding(charset).GetBytes(text)); + return path; + } + + /// Detect-mode scan then convert the rows: what the GUI's buttons do. + private List ViewThenConvert( + string target = "utf-8", bool whatIf = false) + { + var scanned = new List(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + IncludeSubdirectories = true, + IncludePatterns = ["*"], + Action = ScanAction.Detect, + }, + scanned.Add, + CancellationToken.None); + + var completed = new List(); + + ScanEngine.ConvertFiles( + scanned, + target, + targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: whatIf, + backup: false, + completed.Add, + CancellationToken.None); + + return completed; + } + + [Fact] + public void TheGuiSequenceRefusesWhatTheCliRefuses() + { + // The regression. Before the policy was extracted this converted the file. + byte[] original = + Encoding.GetEncoding("windows-1252").GetBytes("Le café était déjà prêt"); + string path = Path.Combine(_root, "ambiguous.txt"); + File.WriteAllBytes(path, original); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert()); + + Assert.Equal(PlannedAction.Refuse, entry.Action); + Assert.Equal(ConversionRowResult.Error, entry.Result); + Assert.Equal(AmbiguityClass.TextChanging, entry.Ambiguity); + Assert.Contains("could not be determined uniquely", entry.Diagnostic); + Assert.Equal(original, File.ReadAllBytes(path)); + } + + [Fact] + public void TheGuiSequenceStillConvertsWhatIsSafe() + { + // The other direction. A gate that refuses everything is not safe, it is broken. + const string text = "こんにちは世界。日本語のテキストです。"; + string path = Write("jp.txt", text, "shift_jis"); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert()); + + Assert.Equal(PlannedAction.Convert, entry.Action); + Assert.Equal(ConversionRowResult.Converted, entry.Result); + Assert.Equal(text, Encoding.UTF8.GetString(File.ReadAllBytes(path))); + } + + [Fact] + public void BothSurfacesReachTheSameDecisionForTheSameFile() + { + // Stated directly rather than inferred from two separately-asserted outcomes. + Write("ambiguous.txt", "Le café était déjà prêt", "windows-1252"); + Write("jp.txt", "こんにちは世界。日本語のテキストです。", "shift_jis"); + Write("plain.txt", "just ascii here", "ascii"); + + Dictionary viaGui = ViewThenConvert(whatIf: true) + .ToDictionary(e => Path.GetFileName(e.FilePath), e => e.Action); + + var viaCli = new List(); + + ScanEngine.ScanDirectory( + new ScanDirectoryOptions + { + BaseDirectory = _root, + IncludeSubdirectories = true, + IncludePatterns = ["*"], + Action = ScanAction.Convert, + TargetCharset = "utf-8", + TargetWriteBom = false, + WhatIf = true, + }, + viaCli.Add, + CancellationToken.None); + + Assert.Equal(3, viaGui.Count); + + foreach (ConversionReportEntry entry in viaCli) + Assert.Equal(entry.Action, viaGui[Path.GetFileName(entry.FilePath)]); + } + + [Fact] + public void TheThreeClassificationsMapToTheirActions() + { + // Unambiguous converts; several codecs agreeing on the text converts, with the + // ambiguity disclosed; several codecs disagreeing refuses until somebody chooses. + // These three are the whole user-facing contract of the refusal. + AssertMaps(AmbiguityClass.Unambiguous, PlannedAction.Convert, discloses: false); + AssertMaps(AmbiguityClass.TextEquivalent, PlannedAction.Convert, discloses: true); + AssertMaps(AmbiguityClass.TextChanging, PlannedAction.Refuse, discloses: false); + } + + private static void AssertMaps( + AmbiguityClass ambiguity, PlannedAction expected, bool discloses) + { + PlannedAction action = ConversionPolicy.Decide( + "windows-1252", sourceHasBom: false, + "utf-8", targetHasBom: false, + ambiguity, ["iso-8859-1"], out string? reason); + + Assert.Equal(expected, action); + + // A refusal always says why; a conversion never needs to. + Assert.Equal(expected == PlannedAction.Refuse, reason is not null); + Assert.Equal(discloses, ConversionPolicy.NeedsDisclosure(ambiguity)); + } + + [Fact] + public void AFileAlreadyInTheTargetEncodingIsNotRefusedForAmbiguity() + { + // Nothing is written, so there is no reading of it to get wrong. Refusing here + // would report a danger that does not exist. + Assert.Equal( + PlannedAction.Unchanged, + ConversionPolicy.Decide( + "utf-8", sourceHasBom: false, + "utf-8", targetHasBom: false, + AmbiguityClass.TextChanging, ["iso-8859-1"], out _)); + } + + [Fact] + public void AnUnidentifiedSourceIsSkippedByBothTheGuardAndThePolicy() + { + // ConvertFiles has to answer for an unidentified source before it can resolve an + // Encoding, so it cannot reach the policy. The two must not drift apart. + Assert.Equal( + PlannedAction.Skip, + ConversionPolicy.Decide( + ScanEngine.UNKNOWN_CHARSET, sourceHasBom: false, + "utf-8", targetHasBom: false, + AmbiguityClass.Unambiguous, [], out _)); + + var entry = new ConversionReportEntry + { + FilePath = Write("binary.bin", "irrelevant", "ascii"), + SourceEncoding = ScanEngine.UNKNOWN_CHARSET, + SourceHasBom = false, + TargetEncoding = "utf-8", + TargetHasBom = false, + }; + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: false, completed.Add, CancellationToken.None); + + Assert.Equal(PlannedAction.Skip, Assert.Single(completed).Action); + } + + [Fact] + public void AnExplicitSourceSkipsClassificationButNotTheConversionSafeguards() + { + // Explicit source is an answer to "which encoding is this?", not permission to + // convert regardless. EUC-JP bytes carrying a JIS X 0212 sequence still cannot + // be decoded by code page 51932. + byte[] unrepresentable = + [0x8F, 0xB0, 0xDF, 0xB9, 0xA5, 0xA1, 0xA4, 0xC0, 0xA4, 0xB3]; + string path = Path.Combine(_root, "named.txt"); + File.WriteAllBytes(path, unrepresentable); + + var entry = new ConversionReportEntry + { + FilePath = path, + SourceEncoding = "euc-jp", + SourceHasBom = false, + TargetEncoding = "euc-jp", + TargetHasBom = false, + SourceEncodingWasSpecified = true, + }; + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: false, completed.Add, CancellationToken.None); + + ConversionReportEntry result = Assert.Single(completed); + + // The policy let it through - naming the encoding settled the ambiguity - and + // the conversion engine refused it anyway. + Assert.Equal(PlannedAction.Convert, result.Action); + Assert.Equal(ConversionRowResult.Error, result.Result); + Assert.Equal(unrepresentable, File.ReadAllBytes(path)); + } + + /// + /// What the confirmation dialog does when the user answers a refusal by naming the + /// encoding: the same override -From uses, applied to the refused entries. + /// + private static void ChooseSourceEncoding( + ConversionReportEntry entry, string charset) + { + entry.CurrentCharsetLabel = charset; + entry.SourceEncodingWasSpecified = true; + entry.Ambiguity = AmbiguityClass.Unambiguous; + entry.AmbiguityReason = AmbiguityReason.ExplicitlySpecified; + entry.CompetingEncodings = []; + entry.Diagnostic = null; + entry.Action = null; + } + + [Fact] + public void ChoosingTheSourceEncodingResolvesARefusalInTheGui() + { + // The refusal tells the user to say which encoding it is. Saying so has to work, + // or the safety feature issues advice its own interface cannot take. + byte[] bytes = Encoding.GetEncoding("windows-1252").GetBytes("Le café était prêt"); + string path = Path.Combine(_root, "ambiguous.txt"); + File.WriteAllBytes(path, bytes); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert()); + Assert.Equal(PlannedAction.Refuse, entry.Action); + + ChooseSourceEncoding(entry, "windows-1252"); + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: false, completed.Add, CancellationToken.None); + + ConversionReportEntry result = Assert.Single(completed); + + Assert.Equal(PlannedAction.Convert, result.Action); + Assert.Equal(ConversionRowResult.Converted, result.Result); + Assert.Equal("Le café était prêt", Encoding.UTF8.GetString(File.ReadAllBytes(path))); + } + + [Fact] + public void TheChosenEncodingIsWhatTheConversionActuallyUses() + { + // Not just permission to proceed. Naming a different encoding for the same bytes + // has to produce different text, or the choice is decoration. + byte[] bytes = Encoding.GetEncoding("windows-1252").GetBytes("café"); + string path = Path.Combine(_root, "interpretation.txt"); + File.WriteAllBytes(path, bytes); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert()); + ChooseSourceEncoding(entry, "koi8-r"); + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: false, completed.Add, CancellationToken.None); + + Assert.Equal(ConversionRowResult.Converted, Assert.Single(completed).Result); + + string text = Encoding.UTF8.GetString(File.ReadAllBytes(path)); + + Assert.NotEqual("café", text); + Assert.Equal(Encoding.GetEncoding("koi8-r").GetString(bytes), text); + } + + [Fact] + public void APlanShowsTheChosenEncodingRatherThanTheDetectedOne() + { + // The confirmation is read after the choice is made. It has to describe the + // conversion that will happen, not the one that was refused. + File.WriteAllBytes( + Path.Combine(_root, "ambiguous.txt"), + Encoding.GetEncoding("windows-1252").GetBytes("Le café était prêt")); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert(whatIf: true)); + Assert.NotEqual("koi8-r", entry.SourceEncoding); + + ChooseSourceEncoding(entry, "koi8-r"); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: true, backup: false, _ => { }, CancellationToken.None); + + ConversionPlan plan = ConversionPlan.FromEntries( + [entry], _root, "utf-8", targetHasBom: false, + backupEnabled: false, explicitSource: "koi8-r"); + + PlannedFile planned = Assert.Single(plan.Files); + + Assert.Equal("koi8-r", planned.SourceEncoding); + Assert.Equal(PlannedAction.Convert, planned.Action); + Assert.True(planned.SourceWasSpecified); + Assert.False(planned.MayChangeText); + } + + [Fact] + public void ChoosingAnEncodingTheBytesCannotBeIsStillRefused() + { + // Explicit source ends the ambiguity question, not the conversion safeguards. + // These EUC-JP bytes carry a JIS X 0212 sequence code page 51932 cannot map. + byte[] unrepresentable = + [0x8F, 0xB0, 0xDF, 0xB9, 0xA5, 0xA1, 0xA4, 0xC0, 0xA4, 0xB3]; + string path = Path.Combine(_root, "undecodable.txt"); + File.WriteAllBytes(path, unrepresentable); + + List scanned = ViewThenConvert(whatIf: true); + ConversionReportEntry entry = Assert.Single(scanned); + + ChooseSourceEncoding(entry, "euc-jp"); + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: false, completed.Add, CancellationToken.None); + + Assert.Equal(ConversionRowResult.Error, Assert.Single(completed).Result); + Assert.Equal(unrepresentable, File.ReadAllBytes(path)); + } + + [Fact] + public void ChoosingAnEncodingDoesNotSkipTheBackupRequirement() + { + byte[] original = Encoding.GetEncoding("windows-1252").GetBytes("café"); + string path = Path.Combine(_root, "backupfail.txt"); + File.WriteAllBytes(path, original); + + ConversionReportEntry entry = Assert.Single(ViewThenConvert(whatIf: true)); + ChooseSourceEncoding(entry, "windows-1252"); + + // A directory where the .bak has to go: the copy cannot succeed. + Directory.CreateDirectory(path + ".bak"); + + var completed = new List(); + + ScanEngine.ConvertFiles( + [entry], "utf-8", targetWriteBom: false, + ScanEngine.DefaultMaxParallelism, + whatIf: false, backup: true, completed.Add, CancellationToken.None); + + Assert.Equal(ConversionRowResult.Error, Assert.Single(completed).Result); + Assert.Equal(original, File.ReadAllBytes(path)); + } + + [Fact] + public void AnEntryNobodyDecidedOnCannotReachAPlan() + { + // The shape of the bug this class exists for: an entry that never went through + // the policy must not be planned as a conversion by default. + var undecided = new ConversionReportEntry + { + FilePath = Write("undecided.txt", "text", "ascii"), + SourceEncoding = "us-ascii", + SourceHasBom = false, + TargetEncoding = "utf-8", + TargetHasBom = false, + }; + + Assert.Null(undecided.Action); + + Assert.Throws(() => ConversionPlan.FromEntries( + [undecided], _root, "utf-8", targetHasBom: false, + backupEnabled: false, explicitSource: null)); + } +} diff --git a/sources/EncodingChecker.Tests/StaleConversionStateTests.cs b/sources/EncodingChecker.Tests/StaleConversionStateTests.cs index 2aac63c..d1ae868 100644 --- a/sources/EncodingChecker.Tests/StaleConversionStateTests.cs +++ b/sources/EncodingChecker.Tests/StaleConversionStateTests.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; namespace EncodingChecker.Tests; @@ -38,6 +38,11 @@ private static ConversionReportEntry Entry( SourceHasBom = sourceHasBom, TargetEncoding = sourceEncoding, TargetHasBom = sourceHasBom, + + // These name the source encoding rather than having it detected, which is + // what -From means. Without saying so, the ambiguity gate correctly refuses + // single-byte content whose encoding its bytes do not identify. + SourceEncodingWasSpecified = true, }; private static ConversionReportEntry Convert( diff --git a/sources/EncodingChecker/ConversionConfirmationForm.cs b/sources/EncodingChecker/ConversionConfirmationForm.cs new file mode 100644 index 0000000..e1a3a77 --- /dev/null +++ b/sources/EncodingChecker/ConversionConfirmationForm.cs @@ -0,0 +1,355 @@ +using System; +using System.Collections.Generic; +using System.Drawing; +using System.Linq; +using System.Windows.Forms; + +namespace EncodingChecker; + +/// +/// Shows what a conversion is about to do, and asks. +/// +/// +/// Displays a plan that has already been decided rather than working out the answer for +/// itself. That is the whole point: a dialog that describes one set of conclusions and a +/// conversion that reaches its own is a dialog that can be wrong, and the user approved +/// what the dialog said. The same entries that were classified here are the ones that +/// convert - no second detection pass, the same property -Apply has. +/// +/// The three outcomes it has to keep apart are the ones the classification draws: +/// an encoding the bytes determine, several codecs that agree on the text, and several +/// codecs that disagree. Only the third is refused, and for it the dialog names the +/// alternatives and offers the one thing that resolves it - saying which encoding it is. +/// +/// +internal sealed class ConversionConfirmationForm : Form +{ + private readonly ConversionPlan _plan; + private readonly ComboBox _sourceChoice = new(); + private readonly Button _resolve = new(); + + /// + /// The encoding the user chose for the refused files, or if + /// they did not choose one. + /// + /// + /// Exactly what -From supplies on the command line: an answer to "which + /// encoding is this?", replacing detection and nothing else. Every conversion + /// safeguard still applies to the files it is used for. + /// + internal string? ChosenSourceEncoding { get; private set; } + + internal ConversionConfirmationForm(ConversionPlan plan) + { + _plan = plan; + + Text = "Confirm conversion"; + FormBorderStyle = FormBorderStyle.Sizable; + StartPosition = FormStartPosition.CenterParent; + MinimizeBox = false; + MaximizeBox = false; + ShowInTaskbar = false; + AutoScaleMode = AutoScaleMode.Font; + ClientSize = new Size(680, 520); + MinimumSize = new Size(560, 420); + + Controls.Add(BuildBody()); + Controls.Add(BuildButtons()); + } + + private int Count(PlannedAction action) => + _plan.Files.Count(f => f.Action == action); + + private List Refused => + [.. _plan.Files.Where(f => f is { Action: PlannedAction.Refuse, MayChangeText: true })]; + + private Control BuildBody() + { + var body = new TableLayoutPanel + { + Dock = DockStyle.Fill, + ColumnCount = 1, + Padding = new Padding(12), + AutoScroll = true, + }; + + int convert = Count(PlannedAction.Convert); + int equivalent = _plan.Files.Count( + f => f.Action == PlannedAction.Convert + && f.Ambiguity == AmbiguityClass.TextEquivalent); + + List refused = Refused; + + body.Controls.Add(Heading( + $"Convert {convert} of {_plan.Files.Count} selected file(s) to " + + _plan.TargetEncoding + + (_plan.TargetHasBom ? " with BOM" : " without BOM"))); + + // The three states, kept apart. The counts sum to the selected population. + body.Controls.Add(Rows( + [ + ("Encoding determined by the file's own bytes", convert - equivalent, + "Will convert."), + ("Encoding undetermined, every reading agrees on the text", equivalent, + "Will convert; the label is a choice, the content is not."), + ("Encoding undetermined, readings disagree on the text", refused.Count, + "WILL NOT be converted."), + ("Already in the target encoding", Count(PlannedAction.Unchanged), + "Nothing to do."), + ("Encoding could not be identified", Count(PlannedAction.Skip), + "Left alone."), + ("Could not be read", Count(PlannedAction.Refuse) - refused.Count, + "Left alone."), + ])); + + body.Controls.Add(Rule()); + + body.Controls.Add(Rows( + [ + ("Directory", _plan.BaseDirectory), + ("Source encoding", + string.IsNullOrEmpty(_plan.ExplicitSourceEncoding) + ? "detected per file" + : $"{_plan.ExplicitSourceEncoding} (chosen; detection bypassed)"), + ("Backups", _plan.BackupEnabled + ? "enabled — each original kept as .bak" + : "DISABLED — originals will not be kept"), + ("Guarantees", ConversionSemantics.Describes), + ])); + + if (refused.Count > 0) + body.Controls.Add(BuildRefusalPanel(refused)); + + foreach (Control control in body.Controls) + control.Dock = DockStyle.Top; + + // Docked children stack in reverse, so the first added must be added last. + var ordered = body.Controls.Cast().Reverse().ToArray(); + body.Controls.Clear(); + body.Controls.AddRange(ordered); + + return body; + } + + private Control BuildRefusalPanel(List refused) + { + var panel = new Panel { AutoSize = true, Padding = new Padding(0, 12, 0, 0) }; + + var explanation = new Label + { + AutoSize = true, + MaximumSize = new Size(620, 0), + ForeColor = Color.FromArgb(150, 40, 0), + Text = + $"The source encoding of {refused.Count} file(s) could not be determined " + + "uniquely. More than one encoding fits the bytes, and they produce " + + "different text, so converting would pick one reading without saying " + + "so. No changes will be made to these files.", + }; + + var list = new ListView + { + View = View.Details, + FullRowSelect = true, + Height = 120, + Dock = DockStyle.Top, + }; + + list.Columns.Add("File", 200); + list.Columns.Add("Detected", 110); + list.Columns.Add("Also fits, reading it differently", 300); + + foreach (PlannedFile file in refused.Take(200)) + { + list.Items.Add(new ListViewItem( + [ + file.RelativePath, + file.SourceEncoding, + string.Join(", ", file.CompetingEncodings.Take(6)) + + (file.CompetingEncodings.Count > 6 + ? $", and {file.CompetingEncodings.Count - 6} more" + : string.Empty), + ])); + } + + var chooser = new FlowLayoutPanel + { + AutoSize = true, + Dock = DockStyle.Top, + Padding = new Padding(0, 8, 0, 0), + }; + + chooser.Controls.Add(new Label + { + AutoSize = true, + Padding = new Padding(0, 6, 0, 0), + Text = "If you know which encoding these files are, choose it:", + }); + + _sourceChoice.DropDownStyle = ComboBoxStyle.DropDownList; + _sourceChoice.Width = 160; + _sourceChoice.Items.Add("(leave them alone)"); + + // The same set the classifier drew its candidates from, so anything it named as + // a competing reading can be chosen here. + foreach (string charset in TextEncoding.SupportedCharsets) + _sourceChoice.Items.Add(charset); + + _sourceChoice.SelectedIndex = 0; + _sourceChoice.SelectedIndexChanged += (_, _) => + _resolve.Enabled = _sourceChoice.SelectedIndex > 0; + + _resolve.Text = "Use this encoding"; + _resolve.AutoSize = true; + _resolve.Enabled = false; + _resolve.Click += (_, _) => + { + ChosenSourceEncoding = (string)_sourceChoice.SelectedItem!; + DialogResult = DialogResult.Retry; + Close(); + }; + + chooser.Controls.Add(_sourceChoice); + chooser.Controls.Add(_resolve); + + var note = new Label + { + AutoSize = true, + MaximumSize = new Size(620, 0), + ForeColor = SystemColors.GrayText, + Dock = DockStyle.Top, + Text = + "Choosing an encoding replaces detection for these files and nothing " + + "else: the bytes must still decode strictly as it, the result is still " + + "verified to hold exactly the same text, and a failed backup still " + + "stops the conversion.", + }; + + panel.Controls.Add(note); + panel.Controls.Add(chooser); + panel.Controls.Add(list); + panel.Controls.Add(explanation); + + explanation.Dock = DockStyle.Top; + + return panel; + } + + private Control BuildButtons() + { + var strip = new FlowLayoutPanel + { + Dock = DockStyle.Bottom, + FlowDirection = FlowDirection.RightToLeft, + Padding = new Padding(12), + AutoSize = true, + }; + + var cancel = new Button + { + Text = "Cancel", + DialogResult = DialogResult.Cancel, + AutoSize = true, + }; + + int convert = Count(PlannedAction.Convert); + + var proceed = new Button + { + Text = convert > 0 ? $"Convert {convert} file(s)" : "Nothing to convert", + DialogResult = DialogResult.OK, + AutoSize = true, + Enabled = convert > 0, + }; + + strip.Controls.Add(cancel); + strip.Controls.Add(proceed); + + AcceptButton = proceed; + CancelButton = cancel; + + return strip; + } + + private static Label Heading(string text) => new() + { + AutoSize = true, + Font = new Font(SystemFonts.MessageBoxFont!, FontStyle.Bold), + MaximumSize = new Size(620, 0), + Padding = new Padding(0, 0, 0, 8), + Text = text, + }; + + private static Control Rule() => new Label + { + BorderStyle = BorderStyle.Fixed3D, + Height = 2, + Margin = new Padding(0, 8, 0, 8), + }; + + private static Control Rows(IReadOnlyList<(string Label, int Count, string Note)> rows) + { + var table = new TableLayoutPanel + { + ColumnCount = 3, + RowCount = rows.Count, + AutoSize = true, + }; + + foreach ((string label, int count, string note) in rows) + { + // Zero-count categories are dropped: a list of noughts buries the lines that + // actually say something. + if (count == 0) + continue; + + table.Controls.Add(new Label + { + AutoSize = true, + Text = count.ToString(), + Font = new Font(SystemFonts.MessageBoxFont!, FontStyle.Bold), + TextAlign = ContentAlignment.MiddleRight, + Width = 44, + }); + + table.Controls.Add(new Label { AutoSize = true, Text = label }); + table.Controls.Add(new Label + { + AutoSize = true, + ForeColor = SystemColors.GrayText, + Text = note, + }); + } + + return table; + } + + private static Control Rows(IReadOnlyList<(string Label, string Value)> rows) + { + var table = new TableLayoutPanel + { + ColumnCount = 2, + RowCount = rows.Count, + AutoSize = true, + }; + + foreach ((string label, string value) in rows) + { + table.Controls.Add(new Label + { + AutoSize = true, + ForeColor = SystemColors.GrayText, + Text = label, + }); + + table.Controls.Add(new Label + { + AutoSize = true, + MaximumSize = new Size(480, 0), + Text = value, + }); + } + + return table; + } +} diff --git a/sources/EncodingChecker/ConversionPlan.cs b/sources/EncodingChecker/ConversionPlan.cs index 5188f82..de95bdd 100644 --- a/sources/EncodingChecker/ConversionPlan.cs +++ b/sources/EncodingChecker/ConversionPlan.cs @@ -207,13 +207,15 @@ internal static ConversionPlan FromEntries( foreach (ConversionReportEntry entry in entries) { - PlannedAction action = entry.Result switch - { - ConversionRowResult.Unchanged => PlannedAction.Unchanged, - ConversionRowResult.Skipped => PlannedAction.Skip, - ConversionRowResult.Error => PlannedAction.Refuse, - _ => PlannedAction.Convert, - }; + // Taken from the decision itself rather than re-derived from the row result, + // which cannot tell a refusal apart from a conversion that failed. An + // undecided entry is a caller that skipped the policy, and planning a + // conversion nobody decided on is the failure this whole mechanism exists to + // prevent - so it is raised, not defaulted. + PlannedAction action = entry.Action + ?? throw new InvalidOperationException( + $"'{entry.FilePath}' reached a conversion plan without a decision. " + + "Entries must go through a conversion pass before being planned."); string hash; long size; @@ -232,11 +234,18 @@ internal static ConversionPlan FromEntries( action = PlannedAction.Refuse; } + // What the conversion will actually read the file as, which is not always + // the scan's original answer: a user may have named the encoding since. + ScanEngine.ParseCharsetLabel( + entry.EffectiveSourceLabel, + out string sourceCharset, + out bool sourceHasBom); + int codePage = 0; try { - codePage = Encoding.GetEncoding(entry.SourceEncoding).CodePage; + codePage = Encoding.GetEncoding(sourceCharset).CodePage; } catch (ArgumentException) { @@ -249,9 +258,9 @@ internal static ConversionPlan FromEntries( Size = size, Sha256 = hash, Action = action, - SourceEncoding = entry.SourceEncoding, + SourceEncoding = sourceCharset, SourceCodePage = codePage, - SourceHasBom = entry.SourceHasBom, + SourceHasBom = sourceHasBom, SourceWasSpecified = entry.SourceEncodingWasSpecified, Ambiguity = entry.Ambiguity, AmbiguityReason = entry.AmbiguityReason, diff --git a/sources/EncodingChecker/ConversionPolicy.cs b/sources/EncodingChecker/ConversionPolicy.cs new file mode 100644 index 0000000..5f91985 --- /dev/null +++ b/sources/EncodingChecker/ConversionPolicy.cs @@ -0,0 +1,90 @@ +using System; +using System.Collections.Generic; + +namespace EncodingChecker; + +/// +/// The one place that decides what happens to a file. +/// +/// +/// Every surface - the CLI, a written plan, and the GUI - asks this and acts on the +/// answer, rather than each working out for itself what is safe. That is not tidiness. +/// The GUI previously reached its own conclusion by omission: ambiguity was classified +/// only during a Convert-mode scan, and the GUI scans in Detect mode, so every entry +/// arrived at conversion carrying the default "unambiguous" and the refusal that the +/// CLI applied never fired. The tool converted, on the strength of whatever detection +/// returned, the exact files it tells CLI users it will not convert. +/// +/// A safety rule that lives at one call site is a safety rule the next call site does +/// not have. +/// +/// +internal static class ConversionPolicy +{ + /// + /// Decides what to do with one file, given what is known about its encoding. + /// + /// + /// Why, in words a user can act on, when the answer is not a plain conversion. + /// + internal static PlannedAction Decide( + string sourceCharset, + bool sourceHasBom, + string targetCharset, + bool targetHasBom, + AmbiguityClass ambiguity, + IReadOnlyList competingEncodings, + out string? reason) + { + reason = null; + + if (string.Equals( + sourceCharset, ScanEngine.UNKNOWN_CHARSET, StringComparison.Ordinal)) + { + reason = "The file's encoding could not be identified from its contents."; + return PlannedAction.Skip; + } + + // Checked before ambiguity on purpose: a file already in the target encoding is + // not written at all, so there is no reading of it to get wrong. + if (string.Equals(sourceCharset, targetCharset, StringComparison.OrdinalIgnoreCase) + && sourceHasBom == targetHasBom) + { + return PlannedAction.Unchanged; + } + + // The bytes do not identify the encoding that wrote them, and the readings that + // fit disagree about the text. Detection still produced an answer; acting on it + // rewrites the file into one of several possible readings without saying so. + if (ambiguity == AmbiguityClass.TextChanging) + { + reason = AmbiguityAnalysis.DescribeRefusal(sourceCharset, competingEncodings); + return PlannedAction.Refuse; + } + + return PlannedAction.Convert; + } + + /// + /// How an action reads in a report row. + /// + internal static ConversionRowResult ToRowResult(PlannedAction action) => action switch + { + PlannedAction.Unchanged => ConversionRowResult.Unchanged, + PlannedAction.Skip => ConversionRowResult.Skipped, + PlannedAction.Refuse => ConversionRowResult.Error, + _ => ConversionRowResult.Converted, + }; + + /// + /// Whether converting this file could change its Unicode content, as opposed to only + /// re-labelling it. + /// + /// + /// The distinction a confirmation has to draw. A file whose encoding is undetermined + /// but whose candidate readings all agree on the text - plain ASCII being the common + /// case - is not something to warn about; a file whose candidates disagree is. + /// + internal static bool NeedsDisclosure(AmbiguityClass ambiguity) => + ambiguity == AmbiguityClass.TextEquivalent; +} diff --git a/sources/EncodingChecker/ConversionReport.cs b/sources/EncodingChecker/ConversionReport.cs index 47ac39b..71b3a59 100644 --- a/sources/EncodingChecker/ConversionReport.cs +++ b/sources/EncodingChecker/ConversionReport.cs @@ -87,6 +87,37 @@ internal sealed class ConversionReportEntry /// internal string? ExpectedSourceSha256 { get; set; } + /// + /// What decided for this file, or + /// while nothing has decided yet. + /// Internal state; not included in CSV output. + /// + /// + /// Nullable so that "nobody has decided" cannot be mistaken for "convert it". An + /// entry that reaches a plan undecided is a bug, and one that used to exist: the GUI + /// built conversions from entries whose ambiguity had never been classified. + /// + internal PlannedAction? Action { get; set; } + + /// + /// Whether converting this file could change its Unicode content rather than only + /// its encoding label. + /// + internal bool MayChangeText() => Ambiguity == AmbiguityClass.TextChanging; + + /// + /// The charset label the next conversion will read this file as. + /// + /// + /// when something has overridden or superseded the + /// original detection - a completed conversion, or a user naming the source encoding + /// - and the scan's own answer otherwise. Both the conversion engine and a written + /// plan use this, so what a plan says a file will be read as is what it is read as. + /// + internal string EffectiveSourceLabel => + CurrentCharsetLabel + ?? ScanEngine.FormatCharsetLabel(SourceEncoding, SourceHasBom); + /// Additional error detail; not included in CSV output. internal string? Diagnostic { get; set; } } diff --git a/sources/EncodingChecker/MainForm.cs b/sources/EncodingChecker/MainForm.cs index 80abdad..1e60913 100644 --- a/sources/EncodingChecker/MainForm.cs +++ b/sources/EncodingChecker/MainForm.cs @@ -63,6 +63,15 @@ private enum CurrentAction // actual conversion from preview ("would be converted"). private bool _convertWasPreview; + // A real conversion runs twice: once with WhatIf to decide what would happen, and + // again to carry out what the user confirmed. The second pass reuses the same entry + // objects, which already carry their decisions, so it does not classify anything a + // second time - the conversion that happens is the one that was shown. + private bool _convertWasPlanningPass; + + // Held between the two passes so the confirmed plan is what executes. + private List? _plannedEntries; + // Indices into imgsResults (see SetKeyName calls in MainForm.Designer.cs). // Reuses the existing Failed and Warning icons; the Warning icon carries the // preview/would-change state. @@ -513,12 +522,36 @@ private void OnConvert(object? sender, EventArgs e) entries.Add(entry); } + // A preview writes nothing, so it is its own answer and needs no confirmation. + // A real conversion is decided first and carried out second. + StartConvertPass( + entries, + itemsByPath, + targetLabel, + targetBaseCharset, + writeBom, + planningPass: !chkPreviewChanges.Checked); + } + + /// + /// Runs one conversion pass: the WhatIf pass that decides, or the pass that acts. + /// + private void StartConvertPass( + List entries, + Dictionary itemsByPath, + string targetLabel, + string targetBaseCharset, + bool targetWriteBom, + bool planningPass) + { var completed = new ConcurrentBag(); _convertItemsByPath = itemsByPath; _convertTargetLabel = targetLabel; _convertResults = completed; _convertWasPreview = chkPreviewChanges.Checked; + _convertWasPlanningPass = planningPass; + _plannedEntries = entries; _currentAction = CurrentAction.Convert; UpdateControlsOnActionStart(); @@ -530,8 +563,10 @@ private void OnConvert(object? sender, EventArgs e) { Entries = entries, TargetBaseCharset = targetBaseCharset, - TargetWriteBom = writeBom, - WhatIf = chkPreviewChanges.Checked, + TargetWriteBom = targetWriteBom, + + // The planning pass never writes, whatever the backup box says. + WhatIf = planningPass || chkPreviewChanges.Checked, Backup = chkCreateBackup.Checked, Completed = completed, CancellationToken = _actionCancellation.Token, @@ -540,6 +575,91 @@ private void OnConvert(object? sender, EventArgs e) _actionWorker.RunWorkerAsync(args); } + /// + /// Shows what the planning pass decided and, if the user agrees, carries it out. + /// + /// + /// when a second pass was started and the caller should leave + /// the results alone until it finishes. + /// + private bool ConfirmAndCarryOutPlan( + Dictionary itemsByPath, string targetLabel) + { + List entries = _plannedEntries ?? []; + + if (entries.Count == 0) + return false; + + ScanEngine.ParseCharsetLabel( + targetLabel, out string targetBaseCharset, out bool targetWriteBom); + + ConversionPlan plan; + + try + { + plan = ConversionPlan.FromEntries( + entries, + lstBaseDirectory.Text, + targetBaseCharset, + targetWriteBom, + chkCreateBackup.Checked, + explicitSource: entries.All(e => e.SourceEncodingWasSpecified) + ? entries[0].EffectiveSourceLabel + : null); + } + catch (InvalidOperationException ex) + { + // An entry that reached here without a decision is a bug, not a user error. + ShowWarning("The conversion could not be planned: {0}", ex.Message); + return false; + } + + using var confirmation = new ConversionConfirmationForm(plan); + DialogResult answer = confirmation.ShowDialog(this); + + // The user answered the refusal by naming the encoding. That replaces detection + // for those files and nothing else, so they go back through the same decision. + if (answer == DialogResult.Retry && + confirmation.ChosenSourceEncoding is { } chosen) + { + foreach (ConversionReportEntry entry in entries) + { + if (entry.Action != PlannedAction.Refuse || !entry.MayChangeText()) + continue; + + // The engine's existing override point. SourceEncoding keeps the + // scan's answer for the report; this is what the conversion reads. + entry.CurrentCharsetLabel = chosen; + entry.SourceEncodingWasSpecified = true; + entry.Ambiguity = AmbiguityClass.Unambiguous; + entry.AmbiguityReason = AmbiguityReason.ExplicitlySpecified; + entry.CompetingEncodings = []; + entry.Diagnostic = null; + + // Cleared so the policy decides again rather than reusing the refusal. + entry.Action = null; + } + + StartConvertPass( + entries, itemsByPath, targetLabel, + targetBaseCharset, targetWriteBom, planningPass: true); + + return true; + } + + if (answer != DialogResult.OK) + { + UpdateControlsOnActionDone("Conversion cancelled. No files were modified."); + return true; + } + + StartConvertPass( + entries, itemsByPath, targetLabel, + targetBaseCharset, targetWriteBom, planningPass: false); + + return true; + } + private void OnCancelAction(object? sender, EventArgs e) { if (_actionWorker.IsBusy) @@ -729,11 +849,22 @@ private void ConvertWorkerCompleted(RunWorkerCompletedEventArgs e) Dictionary itemsByPath = _convertItemsByPath!; ConcurrentBag completed = _convertResults ?? []; bool wasPreview = _convertWasPreview; + bool wasPlanningPass = _convertWasPlanningPass; _convertItemsByPath = null; _convertTargetLabel = null; _convertResults = null; _convertWasPreview = false; + _convertWasPlanningPass = false; + + // Nothing was written yet. Show what was decided and ask before anything is. + if (e.Error is null && !e.Cancelled && wasPlanningPass && + ConfirmAndCarryOutPlan(itemsByPath, targetLabel)) + { + return; + } + + _plannedEntries = null; if (e.Error != null) { diff --git a/sources/EncodingChecker/Program.cs b/sources/EncodingChecker/Program.cs index c319e5f..63c6f61 100644 --- a/sources/EncodingChecker/Program.cs +++ b/sources/EncodingChecker/Program.cs @@ -313,7 +313,9 @@ .. plan.Files TargetEncoding = plan.TargetEncoding, TargetHasBom = plan.TargetHasBom, // The plan already settled this. Re-deriving it would be the second - // detection pass the plan exists to avoid. + // detection pass the plan exists to avoid; carrying the decision + // across is what tells the engine not to classify again. + Action = f.Action, Ambiguity = f.Ambiguity, AmbiguityReason = f.AmbiguityReason, CompetingEncodings = f.CompetingEncodings, diff --git a/sources/EncodingChecker/ScanEngine.cs b/sources/EncodingChecker/ScanEngine.cs index 9fb188e..5f75ac1 100644 --- a/sources/EncodingChecker/ScanEngine.cs +++ b/sources/EncodingChecker/ScanEngine.cs @@ -239,17 +239,17 @@ internal static void ConvertFiles( // with the old encoding can silently produce mojibake that neither strict // decoding nor hash verification catches, since both would use the same // wrong encoding. - string effectiveLabel = - entry.CurrentCharsetLabel - ?? FormatCharsetLabel(entry.SourceEncoding, entry.SourceHasBom); - ParseCharsetLabel( - effectiveLabel, + entry.EffectiveSourceLabel, out string sourceCharset, out bool sourceHasBom); + // Stands in for ConversionPolicy's answer for an unidentified source, + // which has to be given before Encoding.GetEncoding is reached rather + // than inside ApplyConversion. Pinned as agreeing in ConversionPolicyTests. if (sourceCharset == UNKNOWN_CHARSET) { + entry.Action = PlannedAction.Skip; entry.Result = ConversionRowResult.Skipped; return entry; } @@ -357,19 +357,6 @@ internal static string FormatCharsetLabel( // gave. entry.SourceEncodingWasSpecified = sourceWasSpecified; - if (detected is not null && options.Action == ScanAction.Convert - && !sourceWasSpecified) - { - AmbiguityAnalysis? ambiguity = AnalyzeAmbiguity(path, detected); - - if (ambiguity is not null) - { - entry.Ambiguity = ambiguity.Class; - entry.AmbiguityReason = ambiguity.Reason; - entry.CompetingEncodings = ambiguity.CompetingCandidates; - } - } - switch (options.Action) { case ScanAction.Detect: @@ -443,33 +430,44 @@ private static void ApplyConversion( entry.TargetEncoding = targetCharset; entry.TargetHasBom = targetWriteBom; - // Compared against the file's current state, not entry.SourceEncoding, which - // stays at its original-scan value for reporting even after a conversion. - bool alreadyMatches = - string.Equals( - sourceCharset, - targetCharset, - StringComparison.OrdinalIgnoreCase) && - sourceHasBom == targetWriteBom; - - if (alreadyMatches) + // Classified here, at the point where the decision is actually made, so that + // every caller reaching a conversion has it - the CLI's Convert scan, a plan + // being written, and the GUI alike. Doing it during the scan instead meant the + // GUI, which scans in Detect mode, never had it. + // + // Skipped when the user named the source encoding: there is nothing ambiguous + // about an answer somebody gave. Skipped too when the entry already carries a + // decision, which means a plan made it earlier - classifying again there would + // be the second detection pass a plan exists to avoid, and its answer, not this + // one, is what the user approved. The policy below still re-asserts the gate + // from what the plan recorded. + if (entry.Action is null && !entry.SourceEncodingWasSpecified) { - entry.Result = ConversionRowResult.Unchanged; - return; + AmbiguityAnalysis? analysis = AnalyzeAmbiguity(path, sourceEncoding); + + if (analysis is not null) + { + entry.Ambiguity = analysis.Class; + entry.AmbiguityReason = analysis.Reason; + entry.CompetingEncodings = analysis.CompetingCandidates; + } } - // Refuse before touching anything when the file's bytes do not identify the - // encoding that wrote them and the rival readings disagree about the text. - // - // Detection still produces an answer for these; on short or ASCII-heavy input - // that answer is close to a guess, and acting on it rewrites the user's file - // into one of several possible readings without saying so. Skipped when the user - // named the source encoding, since then it is their answer, not a guess. - if (entry.Ambiguity == AmbiguityClass.TextChanging) + PlannedAction action = ConversionPolicy.Decide( + sourceCharset, + sourceHasBom, + targetCharset, + targetWriteBom, + entry.Ambiguity, + entry.CompetingEncodings, + out string? policyReason); + + entry.Action = action; + + if (action != PlannedAction.Convert) { - entry.Result = ConversionRowResult.Error; - entry.Diagnostic = AmbiguityAnalysis.DescribeRefusal( - sourceCharset, entry.CompetingEncodings); + entry.Result = ConversionPolicy.ToRowResult(action); + entry.Diagnostic = policyReason; return; } From a5b9d4ddc6bbd4f4f172e90bf93f2ed495116d8e Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 27 Aug 2026 05:31:56 +0300 Subject: [PATCH 2/2] Test the orchestration itself, and count every detection The defect this branch found was not in a component. Every component was correct and tested; the sequence between them was not, and the sequence lived in button handlers and background-worker callbacks where nothing could run it end to end. Fixing the defect without fixing that leaves the same gap open for the next one. So the sequence is a class. ConversionOrchestrator does the whole of it - decide, ask, carry out what was agreed - with the confirmation as a delegate and the real conversion engine underneath. MainForm keeps the thread marshalling and nothing else. Fourteen tests drive it against real files: each of the three classifications, an explicit source choice, a cancel, a stale plan, a failed backup, content the target cannot hold, a preview, and an undecided entry. Every case that must not modify anything reads the source bytes before and after. Two gaps that only showed up once the sequence was testable. The GUI's second pass did not carry the planned hashes, so it had no equivalent of -Apply's staleness check - a file could change while the user read the dialog and be converted anyway. It now runs the same all-or-nothing check and binds each entry to the bytes it was approved for. And an explicit encoding was applied to every refused file at once, which is wrong for the same reason applying it to the whole batch would be: a batch can hold refused files in different encodings. The choice now carries its scope, the dialog ticks the files it applies to, and the button says how many. "Detection happens once" was an architectural claim about all three surfaces. This project has already been caught by one of those. It is now counted: DetectionCounters records every time EC works out an encoding or classifies one, and the tests assert the counts. A GUI conversion detects once per file at View and never again - not while building the plan, not while the dialog is open, not while writing. A plan detects N and classifies N; applying it does neither. Reading a plan costs nothing. Test parallelism is disabled so those counts mean something; the suite runs in two seconds. The "no classification is not a safe state" rule now holds throughout rather than at the plan boundary: ConversionReportEntry.Ambiguity is nullable, an explicit source records Unambiguous rather than relying on a field default, and the policy refuses an unclassified file rather than converting it. It refuses instead of throwing because it runs inside a parallel conversion loop, where refusing leaves every file intact. 422 passing. Co-Authored-By: Claude Opus 5 --- README.md | 42 +- sources/EncodingChecker.Tests/AssemblyInfo.cs | 9 + .../ConversionConfirmationFormTests.cs | 51 ++- .../ConversionOrchestrationTests.cs | 416 ++++++++++++++++++ .../DetectionCountTests.cs | 233 ++++++++++ .../ConversionConfirmationForm.cs | 67 ++- .../EncodingChecker/ConversionOrchestrator.cs | 304 +++++++++++++ sources/EncodingChecker/ConversionPlan.cs | 5 +- sources/EncodingChecker/ConversionPolicy.cs | 19 +- sources/EncodingChecker/ConversionReport.cs | 14 +- sources/EncodingChecker/DetectionCounters.cs | 48 ++ sources/EncodingChecker/EncodingAmbiguity.cs | 2 + sources/EncodingChecker/MainForm.cs | 174 ++------ sources/EncodingChecker/ScanEngine.cs | 21 +- sources/EncodingChecker/TextEncoding.cs | 4 + 15 files changed, 1242 insertions(+), 167 deletions(-) create mode 100644 sources/EncodingChecker.Tests/AssemblyInfo.cs create mode 100644 sources/EncodingChecker.Tests/ConversionOrchestrationTests.cs create mode 100644 sources/EncodingChecker.Tests/DetectionCountTests.cs create mode 100644 sources/EncodingChecker/ConversionOrchestrator.cs create mode 100644 sources/EncodingChecker/DetectionCounters.cs diff --git a/README.md b/README.md index c543290..543a270 100644 --- a/README.md +++ b/README.md @@ -55,16 +55,38 @@ The conversion that runs is the one shown. Nothing is detected a second time bet confirmation and the writing, so the dialog cannot describe one set of conclusions while a different set is carried out — the same property `-Apply` has. -When files are refused, the dialog names the encodings actually in conflict and offers the -one thing that resolves it: saying which encoding they are. That selection is the GUI's -`-From`. It replaces detection for those files and nothing else — the bytes must still -decode strictly as the chosen encoding, the output is still verified to hold exactly the -same text, and a failed backup still stops the conversion. - -The GUI and the CLI ask the same question of the same code. There is one policy engine -([`ConversionPolicy`](sources/EncodingChecker/ConversionPolicy.cs)); detection or an -explicit source produces a classification, the classification produces an action, and -every surface acts on that action rather than reaching its own conclusion. +If the files change between the confirmation and the writing, **nothing is converted** — +the same all-or-nothing check `-Apply` makes, for the same reason: a person reading a +dialog takes time, and what they approved was the files as they were. + +When files are refused, the dialog lists them with the encodings actually in conflict and +offers the one thing that resolves it: saying which encoding they are. That selection is +the GUI's `-From`. It replaces detection for those files and nothing else — the bytes must +still decode strictly as the chosen encoding, the output is still verified to hold exactly +the same text, and a failed backup still stops the conversion. + +The choice applies only to the files you tick, and the button says how many. A batch can +easily hold refused files in different encodings — Cyrillic in koi8-r beside French in +windows-1252 — and one answer settles only the files it was given about. Imposing it on +the rest would repeat, one level up, the mistake the refusal exists to prevent. + +### One policy engine + +The GUI and the CLI ask the same question of the same code: + +``` +detection / explicit source → classification → PlannedAction → CLI, GUI, plan +``` + +[`ConversionPolicy`](sources/EncodingChecker/ConversionPolicy.cs) decides; every surface +acts on that decision rather than reaching its own. A missing classification is an +internal error, never a safe state: an entry that reaches a conversion or a plan without +one is refused or raises, rather than being treated as unambiguous. + +EC also counts how often it works out an encoding, and asserts in its test suite that a +file is never examined twice — once when scanned, and never again between a decision being +approved and carried out. Applying a plan, and confirming a GUI conversion, do no +detection at all. ## Command-line usage diff --git a/sources/EncodingChecker.Tests/AssemblyInfo.cs b/sources/EncodingChecker.Tests/AssemblyInfo.cs new file mode 100644 index 0000000..b356252 --- /dev/null +++ b/sources/EncodingChecker.Tests/AssemblyInfo.cs @@ -0,0 +1,9 @@ +using Xunit; + +// DetectionCountTests measures process-global counters to assert that EC never works out +// a file's encoding twice. Those counts are only meaningful if nothing else is detecting +// at the same time, and xUnit runs test classes in parallel by default. +// +// The whole suite runs in well under a second, so serialising it costs nothing worth +// weighing against being able to state that invariant as a test. +[assembly: CollectionBehavior(DisableTestParallelization = true)] diff --git a/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs index 55c37a8..1a7342b 100644 --- a/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs +++ b/sources/EncodingChecker.Tests/ConversionConfirmationFormTests.cs @@ -1,4 +1,4 @@ -using System.Text; +using System.Text; using System.Windows.Forms; namespace EncodingChecker.Tests; @@ -129,7 +129,7 @@ public void ItBuildsWhenNothingIsRefused() { using var form = new ConversionConfirmationForm(plan); - Assert.DoesNotContain("could not be determined", AllText(form)); + Assert.DoesNotContain("need an explicit source encoding", AllText(form)); }); } @@ -151,7 +151,7 @@ public void ItBuildsWhenEverythingIsRefused() string text = AllText(form); - Assert.Contains("could not be determined", text); + Assert.Contains("need an explicit source encoding", text); Assert.Contains("Nothing to convert", text); Assert.DoesNotContain("Convert 1 file", text); }); @@ -193,6 +193,51 @@ .. Descendants(form) }); } + [Fact] + public void ItMakesTheScopeOfAnEncodingChoiceUnmistakable() + { + // The button says how many files the choice would apply to, and the count moves + // with the ticks. A user must never have to infer how far their answer reaches. + Write("french.txt", "Le café était déjà prêt", "windows-1252"); + Write("russian.txt", "Привет мир, это русский текст", "koi8-r"); + + ConversionPlan plan = Plan(); + + Assert.Equal(2, plan.Files.Count(f => f.Action == PlannedAction.Refuse)); + + OnUiThread(() => + { + using var form = new ConversionConfirmationForm(plan); + + // ListView caches check state until it has a window handle, and only raises + // ItemChecked once it does. Nothing here pumps messages; the handle is enough. + form.CreateControl(); + _ = form.Handle; + + ListView list = Assert.Single(Descendants(form).OfType()); + + list.CreateControl(); + _ = list.Handle; + + // Everything the dialog asked about starts ticked, and the button says so. + Assert.Equal(2, list.CheckedItems.Count); + Assert.Contains("for 2 file(s)", AllText(form)); + + list.Items[0].Checked = false; + + Assert.Contains("for 1 file(s)", AllText(form)); + + // And with none ticked there is nothing to apply, so it cannot be pressed. + list.Items[1].Checked = false; + + Button apply = Assert.Single( + Descendants(form).OfType