From 704cf5a2a29070fb9cadb01f96975dd30fdb57a1 Mon Sep 17 00:00:00 2001 From: BRUNER Patrick Date: Tue, 28 Jul 2026 16:30:42 +0200 Subject: [PATCH 1/2] fix: resolve legacy code-page encodings without opening Preferences first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .NET does not ship the legacy Windows code pages; Encoding.GetEncoding throws for them until CodePagesEncodingProvider is registered. Registration lived only in the SettingsDialog constructor, and every site that resolves an encoding *name* swallows the ArgumentException and falls back to Encoding.Default. So a user who picked Windows-1252 in Preferences had that choice silently discarded on every restart in which they did not reopen Preferences. Same for a code page persisted per file in a .lxp, and for the settings JSON. Add EncodingRegistry (LogExpert.Core/Helpers) and route all four resolve sites through it: FileOperationService.FillDefaultEncodingFromSettings, EncodingJsonConverter.ReadJson, PersisterXML.ReadEncoding and the Preferences dropdown. Every method registers the provider before it resolves, so correctness does not depend on one entry point having run first — Program.cs is untouched. Registration uses Lazy with ExecutionAndPublication because files load under Task.Run: the flag must not be observable before registration has completed, or a concurrent first resolve hits the original bug. Also add Windows-1250 to the Preferences encoding dropdown, and extract SettingsDialog.GetAvailableEncodings so the offered set is assertable without building the dialog. DetermineEncoding is deliberately unchanged. The precedence chain (explicit encoding, then BOM, then Preferences default, then machine default) is the intended behaviour; it gains regression tests, not edits. PositionAwareReader is not touched. Delete the memory-mapped read path ---------------------------------- MemoryMappedFileReader and LineOffsetIndex were unreachable. LogfileReader sets IsMultiFile = multiFile || fileNames.Length == 1, so a single file makes IsMultiFile true and the `if (!IsMultiFile && ...)` guard never fires; the multi-file ctor passes multiFile: true. _mmfReader was always null, so BuildIndex, ExtendIndex, GetLine and the fast path in GetLogLineMemoryInternal never ran. Deleting them changes no reachable behaviour. It also could not simply be switched on. MemoryMappedFile.CreateFromFile opens with FileShare.Read, so while a view is mapped the process producing the log cannot append — a test appending to a mapped file fails with IOException even when the writer requests FileShare.ReadWrite. A read path that locks out the writer is incompatible with tail mode, so fixing IsMultiFile would not have produced a working memory-mapped reader but a regression. Same rationale as ADR 0006: the reader folder holds only code something can reach. IsMultiFile itself is left exactly as it is; changing it moves every single-file open onto a different name-resolution path and belongs in its own change. Tests: the four resolve sites are pinned at the site, not just on the helper (FileOperationServiceTests, EncodingJsonConverterTests, PersisterXmlEncodingTests, SettingsDialogEncodingListTests), plus the full DetermineEncoding precedence chain in LogfileReaderEncodingTests. Supersedes #673 and #671. --- .../JsonConverters/EncodingJsonConverter.cs | 17 +- .../Classes/Log/LineOffsetIndex.cs | 53 ------- .../Classes/Log/LogfileReader.cs | 24 --- .../Classes/Log/MemoryMappedFileReader.cs | 146 ------------------ .../Classes/Persister/PersisterXML.cs | 33 ++-- .../Helpers/EncodingRegistry.cs | 115 ++++++++++++++ .../ColumnizerTests/CSVColumnizerTest.cs | 7 +- .../SettingsDialogEncodingListTests.cs | 47 ++++++ .../Encodings/EncodingJsonConverterTests.cs | 62 ++++++++ .../Encodings/EncodingRegistryTests.cs | 60 +++++++ .../Encodings/PersisterXmlEncodingTests.cs | 90 +++++++++++ .../Services/FileOperationServiceTests.cs | 27 ++++ .../LogfileReaderEncodingTests.cs | 135 ++++++++++++++++ src/LogExpert.UI/Dialogs/SettingsDialog.cs | 46 ++++-- .../FileOperationService.cs | 24 +-- 15 files changed, 603 insertions(+), 283 deletions(-) delete mode 100644 src/LogExpert.Core/Classes/Log/LineOffsetIndex.cs delete mode 100644 src/LogExpert.Core/Classes/Log/MemoryMappedFileReader.cs create mode 100644 src/LogExpert.Core/Helpers/EncodingRegistry.cs create mode 100644 src/LogExpert.Tests/Dialogs/SettingsDialogEncodingListTests.cs create mode 100644 src/LogExpert.Tests/Encodings/EncodingJsonConverterTests.cs create mode 100644 src/LogExpert.Tests/Encodings/EncodingRegistryTests.cs create mode 100644 src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs create mode 100644 src/LogExpert.Tests/StreamReaderTests/LogfileReaderEncodingTests.cs diff --git a/src/LogExpert.Core/Classes/JsonConverters/EncodingJsonConverter.cs b/src/LogExpert.Core/Classes/JsonConverters/EncodingJsonConverter.cs index 6d716de1e..47a6892c8 100644 --- a/src/LogExpert.Core/Classes/JsonConverters/EncodingJsonConverter.cs +++ b/src/LogExpert.Core/Classes/JsonConverters/EncodingJsonConverter.cs @@ -1,6 +1,8 @@ using System; using System.Text; +using LogExpert.Core.Helpers; + using Newtonsoft.Json; namespace LogExpert.Core.Classes.JsonConverters; @@ -53,19 +55,6 @@ public override void WriteJson (JsonWriter writer, object? value, JsonSerializer return null; } - var encodingName = reader.Value?.ToString(); - if (string.IsNullOrEmpty(encodingName)) - { - return Encoding.Default; - } - - try - { - return Encoding.GetEncoding(encodingName); - } - catch (ArgumentException) - { - return Encoding.Default; - } + return EncodingRegistry.GetEncoding(reader.Value?.ToString(), Encoding.Default); } } diff --git a/src/LogExpert.Core/Classes/Log/LineOffsetIndex.cs b/src/LogExpert.Core/Classes/Log/LineOffsetIndex.cs deleted file mode 100644 index 34c9024f6..000000000 --- a/src/LogExpert.Core/Classes/Log/LineOffsetIndex.cs +++ /dev/null @@ -1,53 +0,0 @@ -namespace LogExpert.Core.Classes.Log; - -/// -/// Stores byte offsets for each line start in a file. Supports incremental appending for tail mode. -/// -internal sealed class LineOffsetIndex (int initialCapacity = 4096) -{ - private long[] _offsets = new long[initialCapacity]; - - public int LineCount { get; private set; } - - /// - /// Appends a line-start offset. - /// - public void Add (long offset) - { - if (LineCount == _offsets.Length) - { - Array.Resize(ref _offsets, _offsets.Length * 2); - } - - _offsets[LineCount++] = offset; - } - - /// - /// Returns the byte offset of the start of the given line. - /// - public long GetOffset (int lineNum) - { - return (uint)lineNum < (uint)LineCount ? _offsets[lineNum] : -1; - } - - /// - /// Returns the byte length of the given line (from its start to the next line's start). - /// For the last line, returns -1 (unknown length, read to end or newline). - /// - public long GetLineLength (int lineNum) - { - return (uint)lineNum >= (uint)LineCount - ? -1 - : lineNum + 1 < LineCount - ? _offsets[lineNum + 1] - _offsets[lineNum] - : -1; - } - - /// - /// Removes all offsets, resetting the index. - /// - public void Clear () - { - LineCount = 0; - } -} diff --git a/src/LogExpert.Core/Classes/Log/LogfileReader.cs b/src/LogExpert.Core/Classes/Log/LogfileReader.cs index a0c1fab79..9e2495d9e 100644 --- a/src/LogExpert.Core/Classes/Log/LogfileReader.cs +++ b/src/LogExpert.Core/Classes/Log/LogfileReader.cs @@ -37,8 +37,6 @@ public partial class LogfileReader : ILogfileReader, IMultiFileNavigation, ILogf private readonly ILoadProgressReporter _progressReporter; - private readonly MemoryMappedFileReader _mmfReader; - private const int WAIT_TIME = 1000; private bool _contentDeleted; @@ -186,18 +184,6 @@ private LogfileReader ( _watchedILogFileInfo = fileInfo; - if (!IsMultiFile && _watchedILogFileInfo.Uri?.Scheme is null or "file") - { - try - { - _mmfReader = new MemoryMappedFileReader(_watchedILogFileInfo.FullName, EncodingOptions.Encoding ?? Encoding.Default); - } - catch (IOException) - { - _mmfReader = null; // fallback to buffer path - } - } - StartGCThread(); } @@ -959,12 +945,6 @@ private ValueTask GetLogLineMemoryInternal (int lineNum) return default; } - if (_mmfReader != null && lineNum < _mmfReader.LineCount) - { - var line = _mmfReader.GetLine(lineNum); - return new ValueTask(line); - } - using var readLock = BufferIndex.AcquireReadLock(); { var logBufferEntry = BufferIndex.GetBufferForLineWithIndex(lineNum); @@ -1568,8 +1548,6 @@ private void FileChanged () _logger.Info(CultureInfo.InvariantCulture, "file size changed. new size={0}, file: {1}", newSize, _fileName); FireChangeEvent(); } - - _mmfReader?.ExtendIndex(); } /// @@ -1874,8 +1852,6 @@ protected virtual void Dispose (bool disposing) _cts.Dispose(); BufferIndex.Dispose(); _progressReporter.Dispose(); - _mmfReader?.Dispose(); - } _disposed = true; diff --git a/src/LogExpert.Core/Classes/Log/MemoryMappedFileReader.cs b/src/LogExpert.Core/Classes/Log/MemoryMappedFileReader.cs deleted file mode 100644 index 73f784553..000000000 --- a/src/LogExpert.Core/Classes/Log/MemoryMappedFileReader.cs +++ /dev/null @@ -1,146 +0,0 @@ -using System.Buffers; -using System.IO.MemoryMappedFiles; -using System.Text; - -using ColumnizerLib; - -namespace LogExpert.Core.Classes.Log; - -/// -/// Reads log lines via memory-mapped file access. Builds a line-offset index on load. -/// Supports tail mode by re-mapping when the file grows. -/// -internal sealed class MemoryMappedFileReader (string filePath, Encoding encoding) : IDisposable -{ - private readonly Encoding _encoding = encoding; - private readonly LineOffsetIndex _lineIndex = new(); - private MemoryMappedFile _mmf; - private MemoryMappedViewAccessor _accessor; - private long _mappedLength; - private readonly string _filePath = filePath; - - public int LineCount => _lineIndex.LineCount; - - /// - /// Builds (or rebuilds) the line-offset index by scanning for newline characters. - /// In tail mode, call with startOffset = previously mapped length to index only new content. - /// - public void BuildIndex (long startOffset = 0) - { - using var fs = new FileStream(_filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); - var fileLength = fs.Length; - - if (startOffset == 0) - { - _lineIndex.Clear(); - _lineIndex.Add(0); // first line starts at offset 0 - } - - fs.Position = startOffset; - var buffer = ArrayPool.Shared.Rent(81920); - try - { - int bytesRead; - var position = startOffset; - while ((bytesRead = fs.Read(buffer, 0, buffer.Length)) > 0) - { - for (var i = 0; i < bytesRead; i++) - { - if (buffer[i] == (byte)'\n') - { - _lineIndex.Add(position + i + 1); - } - } - - position += bytesRead; - } - } - finally - { - ArrayPool.Shared.Return(buffer); - } - - // Re-map the file - RemapFile(fileLength); - } - - /// - /// Extends the mapping to cover new file content (for tail mode). - /// - public void ExtendIndex () - { - BuildIndex(_mappedLength); - } - - /// - /// Reads a single line by its zero-based line number. - /// Returns the line as an ILogLineMemory. - /// - public ILogLineMemory GetLine (int lineNum) - { - var offset = _lineIndex.GetOffset(lineNum); - if (offset < 0 || _accessor == null) - { - return null; - } - - var length = _lineIndex.GetLineLength(lineNum); - if (length < 0) - { - // Last line — read to the end of file or a reasonable limit - length = Math.Min(_mappedLength - offset, 1024 * 1024); - } - - if (length <= 0) - { - return new LogLine(ReadOnlyMemory.Empty, lineNum); - } - - // Read bytes from the mapped view - var bytes = new byte[length]; - _ = _accessor.ReadArray(offset, bytes, 0, (int)length); - - // Trim trailing \r\n - var end = (int)length; - if (end > 0 && bytes[end - 1] == '\n') - { - end--; - } - - if (end > 0 && bytes[end - 1] == '\r') - { - end--; - } - - var text = _encoding.GetString(bytes, 0, end); - return new LogLine(text, lineNum); - } - - private void RemapFile (long fileLength) - { - _accessor?.Dispose(); - _mmf?.Dispose(); - - if (fileLength == 0) - { - _mappedLength = 0; - return; - } - - _mmf = MemoryMappedFile.CreateFromFile( - _filePath, - FileMode.Open, - mapName: null, - capacity: fileLength, - MemoryMappedFileAccess.Read); - - _accessor = _mmf.CreateViewAccessor(0, fileLength, MemoryMappedFileAccess.Read); - _mappedLength = fileLength; - } - - public void Dispose () - { - _accessor?.Dispose(); - _mmf?.Dispose(); - } -} diff --git a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs index c570cd5bf..60b122fe5 100644 --- a/src/LogExpert.Core/Classes/Persister/PersisterXML.cs +++ b/src/LogExpert.Core/Classes/Persister/PersisterXML.cs @@ -6,6 +6,7 @@ using LogExpert.Core.Classes.Filter; using LogExpert.Core.Entities; +using LogExpert.Core.Helpers; using NLog; @@ -266,26 +267,24 @@ private static PersistenceData ReadPersistenceDataFromNode (XmlNode node) private static Encoding ReadEncoding (XmlElement fileElement) { XmlNode encodingNode = fileElement.SelectSingleNode("encoding"); - if (encodingNode != null) + if (encodingNode == null) { - XmlAttribute encAttr = encodingNode.Attributes["name"]; - try - { - return encAttr == null ? null : Encoding.GetEncoding(encAttr.Value); - } - catch (ArgumentException e) - { - _logger.Error(e); - return Encoding.Default; - } - catch (NotSupportedException e) - { - _logger.Error(e); - return Encoding.Default; - } + return null; + } + + XmlAttribute encAttr = encodingNode.Attributes["name"]; + if (encAttr == null) + { + return null; + } + + if (EncodingRegistry.TryGetEncoding(encAttr.Value, out var encoding)) + { + return encoding; } - return null; + _logger.Error($"Persisted encoding '{encAttr.Value}' is not supported, falling back to the default encoding"); + return Encoding.Default; } /// diff --git a/src/LogExpert.Core/Helpers/EncodingRegistry.cs b/src/LogExpert.Core/Helpers/EncodingRegistry.cs new file mode 100644 index 000000000..716e4f67c --- /dev/null +++ b/src/LogExpert.Core/Helpers/EncodingRegistry.cs @@ -0,0 +1,115 @@ +using System.Diagnostics.CodeAnalysis; +using System.Text; + +namespace LogExpert.Core.Helpers; + +/// +/// Resolves encoding names and code pages, including the legacy Windows code pages that .NET does not +/// ship with by default. +/// +/// +/// .NET only knows Unicode, ASCII and latin1 out of the box; anything else — windows-1250, +/// windows-1252, … — requires to be registered first, and +/// throws until it is. +/// +/// Registration used to happen as a side effect of constructing the Preferences dialog. Everything +/// that resolves an encoding name runs earlier than that or never opens the dialog at all — the +/// Preferences default encoding, the per-file encoding in a .lxp, the settings JSON — and every one of +/// those call sites swallows the exception and falls back to . The result +/// was that a code page the user had picked was silently discarded on the next start. +/// +/// +/// Resolving through this class removes the ordering problem: every method here registers the provider +/// before it resolves, so no caller has to run after some other component. Callers should not use +/// or directly; +/// the Encoding.Ascii-style static properties are fine, since .NET always has those. +/// +/// +public static class EncodingRegistry +{ + /// + /// Registers on first use. + /// + /// + /// is the point: registration must have + /// *completed* before any thread is allowed past, otherwise a second thread resolving concurrently + /// would call too early, catch the + /// and silently fall back — the exact bug this class exists to + /// prevent. Files load under Task.Run, so concurrent first resolves do happen. + /// + private static readonly Lazy _provider = new( + () => + { + Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); + return true; + }, + LazyThreadSafetyMode.ExecutionAndPublication); + + private static void EnsureRegistered () + { + _ = _provider.Value; + } + + /// + /// Resolves a code page number. + /// + /// The code page number, e.g. 1252. + /// The for . + /// + /// is not a supported code page. Intended for hard-coded code pages, + /// where an unsupported value is a programming error rather than bad user input; use + /// for values that come from a file. + /// + public static Encoding GetEncoding (int codePage) + { + EnsureRegistered(); + return Encoding.GetEncoding(codePage); + } + + /// + /// Resolves an encoding name, falling back when it cannot be resolved. + /// + /// An encoding name such as "windows-1252", possibly null or empty. + /// The encoding to return when is unusable. + /// The resolved encoding, or . + public static Encoding GetEncoding (string? name, Encoding fallback) + { + return TryGetEncoding(name, out var encoding) ? encoding : fallback; + } + + /// + /// Attempts to resolve an encoding name. + /// + /// An encoding name such as "windows-1252", possibly null or empty. + /// The resolved encoding, or null when the name is unusable. + /// + /// true when names a supported encoding; false when it is + /// null, blank or unknown. + /// + public static bool TryGetEncoding (string? name, [NotNullWhen(true)] out Encoding? encoding) + { + encoding = null; + + if (string.IsNullOrWhiteSpace(name)) + { + return false; + } + + EnsureRegistered(); + + try + { + encoding = Encoding.GetEncoding(name); + return true; + } + catch (ArgumentException) + { + return false; + } + catch (NotSupportedException) + { + // Thrown for code pages the provider knows of but cannot instantiate. + return false; + } + } +} diff --git a/src/LogExpert.Tests/ColumnizerTests/CSVColumnizerTest.cs b/src/LogExpert.Tests/ColumnizerTests/CSVColumnizerTest.cs index 5706eb4a9..0a2561b74 100644 --- a/src/LogExpert.Tests/ColumnizerTests/CSVColumnizerTest.cs +++ b/src/LogExpert.Tests/ColumnizerTests/CSVColumnizerTest.cs @@ -524,9 +524,8 @@ public void LogfileReader_CommaCsv_ReloadWithPreProcess_DataLineNotEmpty () } /// - /// Tests the exact GUI scenario: single file (not multi), which enables the MemoryMappedFileReader. - /// The MMF reader reads raw lines without PreProcess, which can conflict with the buffer system - /// where lines are dropped. + /// Tests the exact GUI scenario: a single file opened with multiFile false, with a PreProcess + /// columnizer that drops the header line. /// [Test] public void LogfileReader_CommaCsv_SingleFile_WithPreProcess_DataLineNotEmpty () @@ -537,7 +536,7 @@ public void LogfileReader_CommaCsv_SingleFile_WithPreProcess_DataLineNotEmpty () using ManualResetEventSlim loadingDone = new(false); - // multiFile=FALSE — this enables the MemoryMappedFileReader path (like the real GUI) + // multiFile=FALSE, like the real GUI LogfileReader reader = new(path, new EncodingOptions(), false, 40, 50, new MultiFileOptions(), ReaderType.System, PluginRegistry.PluginRegistry.Instance, 500); reader.PreProcessColumnizer = csvColumnizer; reader.LoadingFinished += (_, _) => loadingDone.Set(); diff --git a/src/LogExpert.Tests/Dialogs/SettingsDialogEncodingListTests.cs b/src/LogExpert.Tests/Dialogs/SettingsDialogEncodingListTests.cs new file mode 100644 index 000000000..8b90ab4d7 --- /dev/null +++ b/src/LogExpert.Tests/Dialogs/SettingsDialogEncodingListTests.cs @@ -0,0 +1,47 @@ +using System.Text; + +using LogExpert.Core.Helpers; +using LogExpert.Dialogs; + +using NUnit.Framework; + +namespace LogExpert.Tests.Dialogs; + +/// +/// The Preferences encoding dropdown is the only way to set Preferences.DefaultEncoding, so the +/// list is asserted directly — building the dialog is not needed to know what it offers. +/// +[TestFixture] +public class SettingsDialogEncodingListTests +{ + [Test] + [TestCase(1250, TestName = "GetAvailableEncodings_OffersWindows1250")] + [TestCase(1252, TestName = "GetAvailableEncodings_OffersWindows1252")] + public void GetAvailableEncodings_OffersLegacyCodePage (int codePage) + { + var encodings = SettingsDialog.GetAvailableEncodings(); + + Assert.That(encodings.Select(encoding => encoding.CodePage), Does.Contain(codePage)); + } + + /// + /// Every offered encoding is saved as its name and resolved from that name on the next start, so a + /// name that cannot be resolved again would silently degrade to . + /// + [Test] + public void GetAvailableEncodings_EveryEntryResolvesByItsPersistedName () + { + var encodings = SettingsDialog.GetAvailableEncodings(); + + Assert.Multiple(() => + { + foreach (var encoding in encodings) + { + Assert.That( + EncodingRegistry.TryGetEncoding(encoding.HeaderName, out _), + Is.True, + $"'{encoding.HeaderName}' cannot be resolved back from a saved preference"); + } + }); + } +} diff --git a/src/LogExpert.Tests/Encodings/EncodingJsonConverterTests.cs b/src/LogExpert.Tests/Encodings/EncodingJsonConverterTests.cs new file mode 100644 index 000000000..5db2cc6a5 --- /dev/null +++ b/src/LogExpert.Tests/Encodings/EncodingJsonConverterTests.cs @@ -0,0 +1,62 @@ +using System.Text; + +using LogExpert.Core.Classes.JsonConverters; +using LogExpert.Core.Helpers; + +using Newtonsoft.Json; + +using NUnit.Framework; + +namespace LogExpert.Tests.Encodings; + +/// +/// The converter used for every in the settings JSON. It runs during +/// ConfigManager initialisation — long before any dialog exists — so it must be able to resolve a +/// legacy Windows code page on its own. +/// +[TestFixture] +public class EncodingJsonConverterTests +{ + [Test] + [TestCase("windows-1250", 1250)] + [TestCase("windows-1252", 1252)] + public void ReadJson_LegacyCodePageName_ResolvesInsteadOfFallingBack (string encodingName, int expectedCodePage) + { + var encoding = Deserialize($"\"{encodingName}\""); + + Assert.Multiple(() => + { + Assert.That(encoding, Is.Not.Null); + Assert.That(encoding.CodePage, Is.EqualTo(expectedCodePage)); + }); + } + + [Test] + public void ReadJson_Null_ReturnsNull () + { + Assert.That(Deserialize("null"), Is.Null); + } + + [Test] + [TestCase("\"\"")] + [TestCase("\"not-a-real-encoding-xxxxx\"")] + public void ReadJson_UnusableName_ReturnsDefaultEncoding (string json) + { + Assert.That(Deserialize(json), Is.EqualTo(Encoding.Default)); + } + + [Test] + public void WriteJson_RoundTripsALegacyCodePage () + { + // Resolve through the registry, not Encoding.GetEncoding — otherwise this test would depend on + // some earlier test having registered the provider. + var written = JsonConvert.SerializeObject(EncodingRegistry.GetEncoding(1252), new EncodingJsonConverter()); + + Assert.That(Deserialize(written)?.CodePage, Is.EqualTo(1252)); + } + + private static Encoding? Deserialize (string json) + { + return JsonConvert.DeserializeObject(json, new EncodingJsonConverter()); + } +} diff --git a/src/LogExpert.Tests/Encodings/EncodingRegistryTests.cs b/src/LogExpert.Tests/Encodings/EncodingRegistryTests.cs new file mode 100644 index 000000000..0ff4fcef7 --- /dev/null +++ b/src/LogExpert.Tests/Encodings/EncodingRegistryTests.cs @@ -0,0 +1,60 @@ +using System.Text; + +using LogExpert.Core.Helpers; + +using NUnit.Framework; + +namespace LogExpert.Tests.Encodings; + +/// +/// The legacy Windows code pages used to become resolvable only as a side effect of constructing +/// the Preferences dialog. Anything that resolved an encoding name earlier — Preferences, a .lxp, +/// the settings JSON — silently fell back to . These tests pin the +/// guarantee that resolving goes through instead. +/// +[TestFixture] +public class EncodingRegistryTests +{ + [Test] + [TestCase(1250)] + [TestCase(1252)] + public void GetEncoding_LegacyCodePage_Resolves (int codePage) + { + var encoding = EncodingRegistry.GetEncoding(codePage); + + Assert.That(encoding.CodePage, Is.EqualTo(codePage)); + } + + [Test] + [TestCase("windows-1250")] + [TestCase("windows-1252")] + [TestCase("utf-8")] + [TestCase("iso-8859-1")] + public void TryGetEncoding_SupportedName_ReturnsTrueAndEncoding (string name) + { + var resolved = EncodingRegistry.TryGetEncoding(name, out var encoding); + + Assert.Multiple(() => + { + Assert.That(resolved, Is.True); + Assert.That(encoding.WebName, Is.EqualTo(name)); + }); + } + + [Test] + [TestCase(null)] + [TestCase("")] + [TestCase(" ")] + [TestCase("not-a-real-encoding-xxxxx")] + public void TryGetEncoding_UnusableName_ReturnsFalse (string? name) + { + var resolved = EncodingRegistry.TryGetEncoding(name, out var encoding); + + Assert.Multiple(() => + { + Assert.That(resolved, Is.False); + Assert.That(encoding, Is.Null); + }); + } + +} diff --git a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs new file mode 100644 index 000000000..d4594761d --- /dev/null +++ b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs @@ -0,0 +1,90 @@ +using System.Text; + +using LogExpert.Core.Classes.Persister; + +using NUnit.Framework; + +namespace LogExpert.Tests.Encodings; + +/// +/// The per-file encoding stored in a .lxp. It is read while a file is being opened, with no dialog +/// involved, so a legacy Windows code page has to resolve on its own here too — otherwise the encoding +/// the user chose for that specific file is silently replaced by . +/// +[TestFixture] +#pragma warning disable CS0618 // PersisterXML is the deprecated fallback format, and still loads old .lxp files. +public class PersisterXmlEncodingTests +{ + private string _lxpFile = null!; + + [SetUp] + public void Setup () + { + _lxpFile = Path.Combine(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp"); + } + + [TearDown] + public void Cleanup () + { + if (File.Exists(_lxpFile)) + { + File.Delete(_lxpFile); + } + } + + [Test] + [TestCase("windows-1250", 1250)] + [TestCase("windows-1252", 1252)] + [TestCase("utf-8", 65001)] + public void Load_PersistedEncoding_Resolves (string encodingName, int expectedCodePage) + { + WriteLxp($""); + + var data = PersisterXML.Load(_lxpFile); + + Assert.Multiple(() => + { + Assert.That(data.Encoding, Is.Not.Null, "the persisted encoding was discarded"); + Assert.That(data.Encoding.CodePage, Is.EqualTo(expectedCodePage)); + }); + } + + [Test] + public void Load_NoEncodingElement_LeavesEncodingNull () + { + WriteLxp(string.Empty); + + var data = PersisterXML.Load(_lxpFile); + + // Null means "nothing was persisted for this file", which lets the BOM and the Preferences + // default decide. It must not be conflated with an unresolvable name. + Assert.That(data.Encoding, Is.Null); + } + + [Test] + public void Load_UnresolvableEncodingName_FallsBackToDefault () + { + WriteLxp(""); + + var data = PersisterXML.Load(_lxpFile); + + Assert.That(data.Encoding, Is.EqualTo(Encoding.Default)); + } + + private void WriteLxp (string encodingElement) + { + // has to be present: PersisterXML.ReadOptions dereferences it unconditionally. + File.WriteAllText( + _lxpFile, + $""" + + + + + {encodingElement} + + + """); + } +} +#pragma warning restore CS0618 diff --git a/src/LogExpert.Tests/Services/FileOperationServiceTests.cs b/src/LogExpert.Tests/Services/FileOperationServiceTests.cs index 52b9a07f6..f00aa7d85 100644 --- a/src/LogExpert.Tests/Services/FileOperationServiceTests.cs +++ b/src/LogExpert.Tests/Services/FileOperationServiceTests.cs @@ -303,6 +303,33 @@ public void AddFileTab_ValidDefaultEncoding_SetsDefaultEncoding () Assert.That(_factoryCalls[0].Encoding.DefaultEncoding.WebName, Is.EqualTo("utf-8")); } + /// + /// The headline bug: .NET does not ship the legacy Windows code pages, and registration of + /// CodePagesEncodingProvider used to happen only in the Preferences dialog constructor. A user + /// who picked Windows-1252 and restarted without reopening Preferences got their choice silently + /// discarded here, because this method swallows the resolve failure and leaves DefaultEncoding null. + /// + [Test] + [TestCase("windows-1250", 1250)] + [TestCase("windows-1252", 1252)] + public void AddFileTab_LegacyCodePageDefaultEncoding_ResolvesWithoutOpeningPreferences (string encodingName, int expectedCodePage) + { + // Arrange + _settings.Preferences.DefaultEncoding = encodingName; + _ = _tabControllerMock + .Setup(tc => tc.FindWindowByFileName(It.IsAny())) + .Returns((LogWindow)null!); + + var request = new FileTabRequest { FileName = "test.log" }; + + // Act + _ = _sut.AddFileTab(request); + + // Assert + Assert.That(_factoryCalls[0].Encoding.DefaultEncoding, Is.Not.Null, "the configured code page was discarded"); + Assert.That(_factoryCalls[0].Encoding.DefaultEncoding.CodePage, Is.EqualTo(expectedCodePage)); + } + [Test] public void AddFileTab_InvalidDefaultEncoding_DefaultEncodingRemainsNull () { diff --git a/src/LogExpert.Tests/StreamReaderTests/LogfileReaderEncodingTests.cs b/src/LogExpert.Tests/StreamReaderTests/LogfileReaderEncodingTests.cs new file mode 100644 index 000000000..63b19ab29 --- /dev/null +++ b/src/LogExpert.Tests/StreamReaderTests/LogfileReaderEncodingTests.cs @@ -0,0 +1,135 @@ +using System.Text; + +using LogExpert.Core.Classes.Log; +using LogExpert.Core.Classes.Log.ProgressReporters; +using LogExpert.Core.Entities; +using LogExpert.Core.Enums; +using LogExpert.Core.Helpers; + +using NUnit.Framework; + +namespace LogExpert.Tests.StreamReaderTests; + +/// +/// Pins the encoding a ends up using — the value that reaches the grid, +/// the Encoding menu and the persisted .lxp. +/// +[TestFixture] +public class LogfileReaderEncodingTests +{ + private const string EURO_LINE = "Euro: €"; + + private string _tempFile = null!; + + [SetUp] + public void Setup () + { + _tempFile = Path.GetTempFileName(); + _ = PluginRegistry.PluginRegistry.Create(Path.GetDirectoryName(_tempFile)!, 500); + } + + [TearDown] + public void Cleanup () + { + if (File.Exists(_tempFile)) + { + File.Delete(_tempFile); + } + } + + [Test] + public void ReadFiles_BomlessFile_UsesConfiguredDefaultEncoding () + { + var configuredEncoding = EncodingRegistry.GetEncoding(1252); + File.WriteAllText(_tempFile, EURO_LINE + "\n", configuredEncoding); + + using var reader = CreateReader(new EncodingOptions { DefaultEncoding = configuredEncoding }); + reader.ReadFiles(); + + Assert.Multiple(() => + { + Assert.That(reader.CurrentEncoding.CodePage, Is.EqualTo(configuredEncoding.CodePage)); + Assert.That(LineText(reader, 0), Is.EqualTo(EURO_LINE)); + }); + } + + [Test] + public void ReadFiles_PreamblePresent_OverridesConfiguredDefaultEncoding () + { + File.WriteAllText(_tempFile, EURO_LINE + "\n", new UTF8Encoding(encoderShouldEmitUTF8Identifier: true)); + + using var reader = CreateReader(new EncodingOptions { DefaultEncoding = EncodingRegistry.GetEncoding(1252) }); + reader.ReadFiles(); + + Assert.Multiple(() => + { + Assert.That(reader.CurrentEncoding.WebName, Is.EqualTo(Encoding.UTF8.WebName)); + Assert.That(LineText(reader, 0), Is.EqualTo(EURO_LINE)); + }); + } + + /// + /// Deliberate precedence: an explicit is either a choice + /// from the Encoding menu or one persisted per file in the .lxp, so it outranks the file's BOM. + /// Only (the Preferences value) yields to a BOM. + /// + [Test] + public void ReadFiles_ExplicitEncoding_TakesPrecedenceOverPreamble () + { + var explicitEncoding = EncodingRegistry.GetEncoding(1252); + File.WriteAllText(_tempFile, EURO_LINE + "\n", new UTF8Encoding(encoderShouldEmitUTF8Identifier: true)); + + using var reader = CreateReader(new EncodingOptions { Encoding = explicitEncoding }); + reader.ReadFiles(); + + Assert.That(reader.CurrentEncoding.CodePage, Is.EqualTo(explicitEncoding.CodePage)); + } + + /// + /// The last link of the chain: nothing explicit, no BOM, no Preferences default — the machine + /// default is what remains. + /// + [Test] + public void ReadFiles_NoExplicitEncodingNoPreambleNoConfiguredDefault_UsesTheMachineDefault () + { + File.WriteAllText(_tempFile, "plain ascii\n", Encoding.ASCII); + + using var reader = CreateReader(new EncodingOptions()); + reader.ReadFiles(); + + Assert.That(reader.CurrentEncoding.CodePage, Is.EqualTo(Encoding.Default.CodePage)); + } + + [Test] + public void ChangeEncoding_SwitchesTheReportedEncoding () + { + File.WriteAllText(_tempFile, "plain ascii\n", Encoding.ASCII); + + using var reader = CreateReader(new EncodingOptions { Encoding = Encoding.ASCII }); + reader.ReadFiles(); + + reader.ChangeEncoding(Encoding.Latin1); + + Assert.That(reader.CurrentEncoding.CodePage, Is.EqualTo(Encoding.Latin1.CodePage)); + } + + private static string? LineText (LogfileReader reader, int lineNum) + { + return reader.GetLogLineMemory(lineNum)?.FullLine.Span.ToString(); + } + + private LogfileReader CreateReader (EncodingOptions encodingOptions) + { + return new LogfileReader( + _tempFile, + encodingOptions, + multiFile: false, + bufferCount: 100, + linesPerBuffer: 500, + new MultiFileOptions(), + ReaderType.SystemDirect, + PluginRegistry.PluginRegistry.Instance, + maximumLineLength: 500, + progressReporter: NullProgressReporter.Instance); + } +} diff --git a/src/LogExpert.UI/Dialogs/SettingsDialog.cs b/src/LogExpert.UI/Dialogs/SettingsDialog.cs index 8207efdfb..de9804533 100644 --- a/src/LogExpert.UI/Dialogs/SettingsDialog.cs +++ b/src/LogExpert.UI/Dialogs/SettingsDialog.cs @@ -9,6 +9,7 @@ using LogExpert.Core.Config; using LogExpert.Core.Entities; using LogExpert.Core.Enums; +using LogExpert.Core.Helpers; using LogExpert.Core.Interfaces; using LogExpert.UI.ControlCharDisplay; using LogExpert.UI.Controls.LogTabWindow; @@ -86,8 +87,6 @@ private SettingsDialog (Preferences prefs, LogTabWindow logTabWin) LoadResources(); - Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); - ResumeLayout(); } @@ -281,7 +280,7 @@ private void FillDialog () FillReaderTypeList(); FillControlCharsTab(); - comboBoxEncoding.SelectedItem = Encoding.GetEncoding(Preferences.DefaultEncoding); + comboBoxEncoding.SelectedItem = EncodingRegistry.GetEncoding(Preferences.DefaultEncoding, Encoding.Default); comboBoxLanguage.SelectedItem = CultureInfo.GetCultureInfo(Preferences.DefaultLanguage).Name; switch (Preferences.ColumnizerSelectionPriority) @@ -689,27 +688,44 @@ private void DisplayCurrentIcon () } /// - /// Populates the encoding list in the combo box with a predefined set of character encodings. + /// Populates the encoding list in the combo box from . The value + /// member of the combo box is set to a specific header name defined in the resources. /// - /// - /// This method clears any existing items in the combo box and adds a selection of common encodings, including - /// ASCII, Default (UTF-8), ISO-8859-1, UTF-8, Unicode, and Windows-1252. The value member of the combo box is set - /// to a specific header name defined in the resources. - /// private void FillEncodingList () { comboBoxEncoding.Items.Clear(); - _ = comboBoxEncoding.Items.Add(Encoding.ASCII); - _ = comboBoxEncoding.Items.Add(Encoding.Default); - _ = comboBoxEncoding.Items.Add(Encoding.GetEncoding("iso-8859-1")); - _ = comboBoxEncoding.Items.Add(Encoding.UTF8); - _ = comboBoxEncoding.Items.Add(Encoding.Unicode); - _ = comboBoxEncoding.Items.Add(CodePagesEncodingProvider.Instance.GetEncoding(1252)); + foreach (var encoding in GetAvailableEncodings()) + { + _ = comboBoxEncoding.Items.Add(encoding); + } comboBoxEncoding.ValueMember = Resources.SettingsDialog_UI_ComboBox_Encoding_ValueMember_HeaderName; } + /// + /// The encodings offered as the default encoding: ASCII, Default (UTF-8), ISO-8859-1, UTF-8, + /// Unicode, Windows-1250 and Windows-1252. + /// + /// + /// Separate from so the offered set can be asserted without building + /// the dialog. The selected entry is saved by name, so every entry has to be resolvable by name on + /// the next start — which is why the code pages go through . + /// + internal static IReadOnlyList GetAvailableEncodings () + { + return + [ + Encoding.ASCII, + Encoding.Default, + Encoding.Latin1, + Encoding.UTF8, + Encoding.Unicode, + EncodingRegistry.GetEncoding(1250), + EncodingRegistry.GetEncoding(1252) + ]; + } + /// /// Populates the language selection list with available language options. /// diff --git a/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs b/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs index a8a77d51b..3f5d511b0 100644 --- a/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs +++ b/src/LogExpert.UI/Services/FileOperationService/FileOperationService.cs @@ -6,6 +6,7 @@ using LogExpert.Core.Classes.Filter; using LogExpert.Core.Classes.Persister; using LogExpert.Core.Entities; +using LogExpert.Core.Helpers; using LogExpert.Core.Interfaces; using LogExpert.UI.Controls.LogWindow; using LogExpert.UI.Interface; @@ -120,17 +121,20 @@ public void AddToFileHistory (string fileName) private void FillDefaultEncodingFromSettings (EncodingOptions encodingOptions) { - if (_configManager.Settings.Preferences.DefaultEncoding != null) + var configuredEncoding = _configManager.Settings.Preferences.DefaultEncoding; + if (configuredEncoding == null) { - try - { - encodingOptions.DefaultEncoding = Encoding.GetEncoding(_configManager.Settings.Preferences.DefaultEncoding); - } - catch (ArgumentException) - { - _logger.Warn($"### FillDefaultEncodingFromSettings: Encoding {_configManager.Settings.Preferences.DefaultEncoding} is not a valid encoding"); - encodingOptions.DefaultEncoding = null; - } + return; + } + + if (EncodingRegistry.TryGetEncoding(configuredEncoding, out var encoding)) + { + encodingOptions.DefaultEncoding = encoding; + } + else + { + _logger.Warn($"### FillDefaultEncodingFromSettings: Encoding {configuredEncoding} is not a valid encoding"); + encodingOptions.DefaultEncoding = null; } } From e3ae13e26ef36bba8d43b0a745ce0c580075ad41 Mon Sep 17 00:00:00 2001 From: BRUNER Patrick Date: Tue, 28 Jul 2026 17:49:44 +0200 Subject: [PATCH 2/2] Path.Join is better --- src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs index d4594761d..a2a1e0444 100644 --- a/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs +++ b/src/LogExpert.Tests/Encodings/PersisterXmlEncodingTests.cs @@ -20,7 +20,7 @@ public class PersisterXmlEncodingTests [SetUp] public void Setup () { - _lxpFile = Path.Combine(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp"); + _lxpFile = Path.Join(Path.GetTempPath(), $"{Guid.NewGuid()}.lxp"); } [TearDown]