Skip to content

Fix configured encoding fallback and add Windows-1250 option - #671

Closed
Pr0metheus2 wants to merge 0 commit into
LogExperts:Developmentfrom
Pr0metheus2:Development
Closed

Fix configured encoding fallback and add Windows-1250 option#671
Pr0metheus2 wants to merge 0 commit into
LogExperts:Developmentfrom
Pr0metheus2:Development

Conversation

@Pr0metheus2

Copy link
Copy Markdown
Contributor

Before

  • When new file is opened and .lxp file does not exist, the .lxp is created, but it has always "UTF-8" encoding stored regardless application encoding setting.

  • BOM-less log files could bypass Preferences.DefaultEncoding in the memory-mapped reader and fall directly back to Encoding.Default (UTF-8).

  • Legacy configured encodings such as Windows-1252 could be rejected if a log was opened before the Settings dialog, because the code-page provider had not yet been registered.

  • Encoding precedence allowed an explicit/persisted value to override a file BOM.

  • Windows-1250 (Central and Eastern Europe) was not selectable in Preferences.

After

  • Encoding resolution follows the intended order:

    BOM → persisted/explicit encoding → Preferences.DefaultEncoding → Encoding.Default

  • Both stream and memory-mapped readers apply the configured default consistently.

  • Legacy Windows code pages are available when reading the preference, regardless of whether Settings was opened first.

  • Windows-1250 is available in the Preferences encoding dropdown.

This ensures a new BOM-less log uses the selected application default and saves that resolved encoding into its newly created .lxp file.

@Hirogen

Hirogen commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator
  1. [MemoryMappedFileReader.cs:13,115] has no BOM detection at all. So MMF implements Encoding ?? DefaultEncoding ?? Default while the stream path implements BOM ?? Encoding ?? DefaultEncoding ?? Default. They now disagree precisely on the leg the spec puts first — and MMF is what serves line text ([LogfileReader.cs:962].
  2. No tests. src/LogExpert.Tests untouched. No coverage for BOM-vs-preference precedence, the 1250 dropdown entry, or provider registration
  3. The .lxp value comes from [LogWindow.cs:5921] snapshot.Encoding = _logFileReader?.CurrentEncoding, set at [LogfileReader.cs:1309]. It only becomes correct transitively, via the root-cause fix (provider registration). DefaultEncoding was already in the old fallback chain, so for utf-8/iso-8859-1 the "always UTF-8" symptom was never reproducible; the actual bug was Encoding.GetEncoding("windows-125x") throwing and nulling DefaultEncoding.
  4. Encoding.RegisterProvider: Placing it inside FillDefaultEncodingFromSettings runs it on every file open. Idempotent, so harmless, but a per-open side effect where a one-time startup registration belongs. Other consumers of persisted names ([PersisterXML.cs:274], EncodingJsonConverter) still depend on one of the two constructor-sited registrations having run first
  5. Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); now appears twice: [SettingsDialog.cs:89] and the new [FileOperationService.cs:123]. A process-wide, idempotent registration belongs once at startup ([Program.cs], not at each entry point that happens to need it. (FillDefaultEncodingFromSettings now also mutates global process state. Nothing in the name reveals it registers an encoding provider, and it runs on every file open.)
  6. No test for the bug fix

BOM-first now silently defeats the explicit Encoding menu. DetermineEncoding has exactly one caller ([PositionAwareStreamReaderBase.cs:52], where detectedEncoding is a genuine BOM — so "BOM wins" is technically sound. But options.Encoding is not only the persisted value: [LogWindow.cs:7308]. ChangeEncoding sets it from the user's Encoding menu, then Reload()s. After this change, on any file with a BOM the reload discards the user's choice and reverts to the BOM. The spec bullet justifying this ("Encoding precedence allowed an explicit/persisted value to override a file BOM") is written about opening a file; applying it to the manual-override path makes the Encoding menu a no-op for BOM'd files — and combined with 1. , leaves MMF decoding with the user's pick while the stream path and the saved .lxp record the BOM.

The Problem why the release build fails is, that you try to merge from a protected branch into a protected branch Dev => Dev, but its better to branch from a _featureBranch into the development branch! So github actions have the ability to commit changes

Pr0metheus2 pushed a commit to Pr0metheus2/LogExpert that referenced this pull request Jul 29, 2026
…irst
.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 LogExperts#673 and LogExperts#671.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Pr0metheus2@Hirogen