Uh oh!
There was an error while loading. Please reload this page.
Extract timestamp lookup into TimestampLocator (LogWindow candidate 6) - #664
Merged
Conversation
Ticket 1 of the Timestamp Locator extraction (candidate 6, docs/improve/logwindow-architecture-review.html). Ports LogWindow's four timestamp-lookup methods (binary search + forward/backward scan) into a Core module tested against a fake ITimestampSource — no WinForms type involved. Lands unused; LogWindow is migrated in the next commit. FindForward returns a tuple instead of using a ref int (closes the referenced-int shape of issue #404 for this seam), and positions the Columnizer callback on every line it inspects in both directions — the original forward scan never did, unlike its backward counterpart. 32 tests cover both scan directions, the binary search, the negated near-miss return on a search miss (including the -0/hit-at-line-zero ambiguity, preserved rather than "fixed" since it's inherent to the ported algorithm), and cancellation.
…cator Tickets 2+3 of the Timestamp Locator extraction. LogWindow implements the new ITimestampSource seam and its timestamp methods become delegations to TimestampLocator; dataGridView.RowCount is replaced by _logFileReader.LineCount at the four lookup sites (it was always just a mirror of the reader's count). ColumnizerCallback now implements IPositionedColumnizerCallback so the locator can position it. TimeSpreadCalculator no longer holds an ILogWindow — the timestamp trio was the only reason it did (closes its own //TODO Refactor that it does not need LogWindow) — and now takes (TimestampLocator, ITimestampSource) instead. GetTimestampForLine/GetTimestampForLineForward/ FindTimestampLineInternal are removed from ILogWindow; the last of those turned out to have no remaining caller once TimeSpreadCalculator was rewired, so LogWindow's own copy is deleted rather than kept private. No ref int remains on the timestamp path. _isTimestampDisplaySyncing is deleted (write-only after the cancellation bail-out moved to the window's CancellationTokenSource, matching the _shouldCancel -> window CTS migration done for the filter path in b7a293a). Manual smoke (drag-to-scroll, time-sync between tabs, two-row time diff, the spread bar) is not yet verified against a live build — flagging per the ticket's acceptance criteria for a human pass.
Ticket 4 of the Timestamp Locator extraction: Timestamp Locator, Timestamp Source, and the negated-near-miss return convention (including the -0/line-zero ambiguity, a preserved quirk of the ported algorithm). Issue #404 (referenced int on ILogWindow) was already closed upstream; this branch removes its last actual occurrence in code. docs/improve/logwindow-architecture-review.html (gitignored, not part of this commit) is updated separately to mark candidate 6 shipped.
…urce Code review (Standards axis) on the Timestamp Locator extraction found a real shutdown-latency regression: GetTimestampForLine's scan and FindTimestampLine's binary search were wired to _windowCts, which is only cancelled late, in CloseLogWindow. The original code (and StopTimestampSyncThread) cancelled via _shouldTimestampDisplaySyncingCancel / _cts, well before JoinWorker blocks. Using _windowCts meant an in-progress scan on SyncTimestampDisplayWorker had no live cancellation signal for the whole of that teardown wait, riding out the full WORKER_SHUTDOWN_TIMEOUT instead of exiting promptly. Switched both call sites to _cts, matching the original bail-out exactly, with a comment explaining why. Also fills two test gaps the spec review found in the Ticket 1 test matrix: roundToSeconds=false preserving sub-second precision (both scan directions), a negative-lineNum case for FindForward (only the above-range case existed), and strengthens the cancellation test to assert the reader is never touched once the token is already cancelled.
…est line Manual smoke on the Timestamp Locator extraction found "Scroll all tabs to current timestamp" and cross-window time-sync doing nothing at all. Root cause: a sign error in the port. The original FindTimestampLine ended with `return -foundLine` — the internal binary search reports a miss as the negated near-miss line, and the public method flipped it BACK to a positive, scrollable line number. The port misread that as "the negation is the public contract" and passed the negative through, so ScrollToTimestampWorker's `foundLine >= 0` check skipped scrolling on every inexact timestamp. Cross-window sync compares timestamps at millisecond precision (roundToSeconds: false), so an exact hit in a different file essentially never happens — every sync path was a no-op. FindLine now flips a miss to the positive converged line, matching the original exactly. FindNearestLine (used by TimeSpreadCalculator, which flips the sign itself) keeps the raw negated convention. The two mis-pinned tests now encode scroll-to-nearest with exact expected lines traced from the original algorithm, and CONTEXT.md's "Negated near-miss" entry now states which method carries which convention.
Field report of dead time-sync on "yyyy-MM-dd HH:mm:ss.fff | LEVEL | thread | source | message" lines. The locator's test matrix so far used only a stub columnizer, so it could not distinguish "lookup broken" from "columnizer can't parse this format". Three integration tests now run the real TimestampColumnizer over lines in exactly that shape: FindBackward parses the millisecond timestamp, FindLine hits an exact millisecond, and a millisecond miss (the cross-window sync common case) still returns a positive, scrollable line. All pass — so a dead sync on this format points at the UI gates (timestamp-control preference, per-window columnizer selection, sync-group membership), not at the parse or lookup pipeline.
…ndows The two time-sync paths signalled differently: "Scroll all tabs to current timestamp" (coordinator) pinged the LED indicator service for every tab it scrolled, but selection-driven sync (worker -> SyncOtherWindows -> TimeSyncList.NavigateToTimestamp) scrolled silently, so users got no visual cue which tabs followed. Pre-existing asymmetry, surfaced while smoke-testing the Timestamp Locator branch. TimeSyncList.NavigateToTimestamp now takes an optional onScrolled callback invoked per window that accepted the scroll, and SyncOtherWindows routes it to the coordinator's new IndicateTimeSyncActivity - the same DIFF_MAX activity signal the menu path raises, so both paths are visually identical. The LED service is safe to call from the sync worker thread: it locks internally and marshals icon updates to the UI thread. Note: from the worker thread, ScrollToTimestamp dispatches via BeginInvoke and reports acceptance rather than completion, so the LED flashes for every sync-group member - documented on the callback.
Selection-driven time-sync (mouse click / arrow keys) only runs when Preferences.TimestampControl is on - and it defaults to off. Users who time-sync files with it off see "Scroll all tabs to current timestamp" work while selecting lines does nothing, which reads as a broken feature rather than a disabled setting; that exact confusion cost a real debugging session during the Timestamp Locator smoke test. When a window is added to a time-sync group and the setting is off, an information box now points at Settings > Timestamp features > "Show timestamp control". Shown once per application run, at the moment sync is switched on - the action whose outcome the setting silently changes. English and German resources.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… sync hint TimeSpreadCalculator's Enabled/TimeMode/Contrast drop their explicit backing fields for C# field-backed properties (matching the CurrentColumnizer idiom in LogWindow); initializers and the Contrast clamping are unchanged. Three FindForward line-number write-backs that were dead even in the pre-extraction ref-int code are now explicit discards; the two live write-backs in DoCalc's preamble stay. The time-sync settings hint flag becomes an instance field: shown once per window instead of once per application run, so a user who dismissed it in one window still gets it when enabling sync from another.
| } | ||
| var value = (int)(diffFromAverage / (timePerLine / TimeSpan.TicksPerMillisecond) * _contrast); | ||
| var value = (int)(diffFromAverage / (timePerLine / TimeSpan.TicksPerMillisecond) * Contrast); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts the Log Window's timestamp lookup — "what time is the line at N" and "which line carries time T" — into a pure, tested Core module (
TimestampLocatorover the narrowITimestampSourceseam). Worker threads, grid scrolling, and cross-window coordination stay in the control; only the lookup moved.Architecture
TimestampLocator(LogExpert.Core/Classes/Timestamp/):FindBackward/FindForward/FindLine/FindNearestLine, pure overILogfileReader+ the active Columnizer. 38 tests, including integration tests through the realTimestampColumnizeron pipe-separated production log lines.ITimestampSource: reader / columnizer / callback / columnizer-lock, read live (both the reader and the columnizer are replaced at runtime). Implemented byLogWindow.TimeSpreadCalculatorno longer needsILogWindow(closes its in-code TODO);ILogWindowsheds 3 members incl.FindTimestampLineInternal; theref intof refernced value types should not be part of an interface #404 is gone from this path.Bugs found and fixed along the way
FindLinepassed the internal negated near-miss through to callers that expect the nearest line — every sync path silently no-opped on inexact timestamps, which is the common case cross-file. Regression-pinned.GetFileName(); fixed in the port.TimeSyncListscrolls now raise the same LED activity signal as "Scroll all tabs to current timestamp".Preferences.TimestampControl(default off) — enabling time-sync with it off now shows a one-time hint pointing at the setting (EN + DE resources).Testing