chore(loggers): extract PlotModelFactory from DatabaseLogger (#592)#608
Merged
Conversation
Next behavior-preserving extraction in the #592 effort to break up the app's two largest classes (after SessionSampleWriter #604 and SessionDataRepository #607). Moves the pure OxyPlot *construction* out of DatabaseLogger into a new sealed PlotModelFactory, leaving the logger as the orchestration + live-model mutation/binding surface. Moved into PlotModelFactory (pure construction only — no EF, threading, Dispatcher, InvalidatePlot, or axis mutation): - The main plot model + Analog/Digital/Time LinearAxis config, dark-theme application, axis Add, and IsLegendVisible=false (was the constructor's axis-setup block). - The minimap model, its two non-interactive axes, and the three RectangleAnnotations (selection rect + two dim overlays). The factory returns the annotation handles so the logger keeps its field references to mutate. - CreateChannelSeries: a channel's LineSeries + LoggedSeriesLegendItem, with the Analog/Digital YAxisKey mapping. The legend item still receives the live PlotModel and the DatabaseLogger so its visibility toggle invalidates the plot and mirrors onto the minimap exactly as before. - CreateMinimapSeries: the per-channel minimap LineSeries construction only — the apply-to-live half of SetupMinimapSeries (Series.Clear/Add, the _minimapSeries map, axis.Zoom, annotation bounds, InvalidatePlot) stays in the logger. Also moves LoggedSeriesLegendItem to its own file (one-main-class-per-file per the style guide) and annotates its optional logger as nullable. Kept in DatabaseLogger (viewport/live-model coupled, untouched): every viewport/minimap-sync method, the throttle/settle DispatcherTimers, the timeAxis.AxisChanged subscription, the MinimapInteractionController wiring, the annotation fields, and every InvalidatePlot/Zoom/axis-mutation call. The ADR-001 viewport controller remains a future, separate extraction. The shared axis-key strings ("Analog"/"Digital"/"Time"/"MinimapTime"/ "MinimapY") become public consts on PlotModelFactory — the single source of truth the factory builds from and the logger's viewport lookups key on. Invariants (ADR-001 / CLAUDE.md "Plot Rendering") preserved: InvalidatePlot(true) on every ItemsSource change, no ResetAllAxes() with downsampled data, the IsSyncingFromMinimap guard, _downsampledCache reuse, and ClearPlot()'s per-axis reset all stay in the logger. No viewport machinery moved. DatabaseLogger.cs: 1,376 -> 1,119 lines. PlotModelFactory is unit-testable with no WPF runtime, DB, or threads — a new PlotModelFactoryTests (13 tests) asserts axis keys/positions, the channel YAxisKey mapping, color parsing, minimap annotation/axis config, annotation z-order, and returned-handle identity. Updates the CLAUDE.md plot-gotchas "Key files" list to name PlotModelFactory.cs. Tests: unit gate green (434/434, incl. the 13 new). Both logged-data-plot FlaUI scenarios pass on the attached device (LoggingSessionTests.StartLoggingSession_RendersLivePlot_RunsStopsAndDeletesSession and ViewLoggedSession_LoadsStoredSessionOntoLoggedDataPlot). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
PR Summary by QodoExtract PlotModelFactory from DatabaseLogger for pure OxyPlot construction Description
Diagram
High-Level Assessment
Files changed (5)
|
Contributor
Code Review by Qodo
Context used✅ Tickets:
🎫 Break up the two god classes: DaqifiViewModel (2,229 lines) and DatabaseLogger (1,820 lines)✅ Compliance rules (platform):
48 rules 1. PlotModelFactory not injected
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Pure OxyPlot construction (axes, theme, minimap model + annotations, series) lives in | ||
| // PlotModelFactory (issue #592). The logger keeps every live mutation: the axis subscription | ||
| // below and all viewport/minimap-sync machinery. | ||
| _plotModelFactory = new PlotModelFactory(); |
Contributor
There was a problem hiding this comment.
1. plotmodelfactory not injected 📘 Rule violation ⌂ Architecture
DatabaseLogger directly instantiates PlotModelFactory, introducing a hard dependency that reduces testability and violates the dependency-injection requirement.
Agent Prompt
## Issue description
`DatabaseLogger` constructs `PlotModelFactory` internally (`new PlotModelFactory()`), instead of receiving it via dependency injection.
## Issue Context
This PR introduces `PlotModelFactory` as a collaborator. Per the compliance rule, collaborators/services should be injected (constructor parameter or DI container) rather than instantiated inside the class.
## Fix Focus Areas
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[131-140]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
I'm going to respectfully push back on this one. PlotModelFactory is a stateless, dependency-free pure factory — no EF, logger, or external services, so there's nothing to mock or substitute — and it's constructed exactly the way the two already-merged #592 collaborators are in this same constructor: _sampleWriter = new SessionSampleWriter(...) and _sessionDataRepository = new SessionDataRepository(...).
DatabaseLogger is the composition root here: it's instantiated directly by DaqifiViewModel (new DatabaseLogger(GetLoggingContextFactory())), not resolved from a DI container. The established pattern injects only the external dependency (the IDbContextFactory) and constructs the internal collaborators inline. Injecting a zero-dependency factory would add ceremony without any testability benefit (the factory's methods are pure and already unit-tested directly) and would diverge from the two merged extractions this PR is modeled on.
Happy to add an internal injection seam if/when these loggers move to the DI container, but as-is new PlotModelFactory() matches the current playbook. Leaving as-is.
#592) Applies the actionable Qodo findings on PR #608: - Single-main-class-per-file: move the public `MinimapPlotComponents` record out of PlotModelFactory.cs into its own MinimapPlotComponents.cs (same rule that motivated splitting out LoggedSeriesLegendItem in this PR). - Constant naming: rename the public axis-key consts to SCREAMING_SNAKE_CASE (ANALOG_AXIS_KEY, DIGITAL_AXIS_KEY, TIME_AXIS_KEY, MINIMAP_TIME_AXIS_KEY, MINIMAP_Y_AXIS_KEY) to match CLAUDE.md's stated convention and the existing Loggers/ consts (MINIMAP_BUCKET_COUNT, INITIAL_LOAD_POINTS, ...). Updates the references in DatabaseLogger and the tests. - Null-dispatcher reliability: LoggedSeriesLegendItem.IsVisible no longer dereferences Application.Current.Dispatcher unconditionally. In the live app Application.Current is always set, so this is the same UI-thread dispatch as before; in a headless/unit-test host (where it is null) the work runs inline instead of throwing, keeping the extracted construction exercisable without a WPF runtime. Adds a test that toggles legend visibility headless. Test-fixture constants (Serial/Color) keep their PascalCase names to stay consistent with the sibling tests (SessionDataRepositoryTests, SessionSampleWriterTests), which use the same convention. Unit gate green: 435/435. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Replies to the Qodo summary findingsThe two action-required (inline) findings are addressed in their threads: #2 (two public types) is fixed; #1 (PlotModelFactory not injected) I've replied to with a rationale for keeping it consistent with the two merged #592 collaborators. For the summary-only findings: 5. Null Dispatcher dereference (🐞 Bug) — Fixed in 3. Constant casing (📘 Rule) — Partially addressed in 4. CLAUDE.md line exceeds 120 (📘 Rule) — Disagreed. The entire "Common Tasks" section is written as long, unwrapped single-line markdown bullets — the pre-existing version of this exact bullet already exceeded 120, and the neighboring bullet (line 331) is longer still. Wrapping only this one bullet would make it inconsistent with the section's established style, and the base content can't fit under 120 without restructuring the bullet in a way no other bullet in the file uses. Left as a single line to match the surrounding prose. |
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 42%
Daqifi.Desktop.Common - 40.5%
Daqifi.Desktop.IO - 100%
Coverage report generated by ReportGenerator • View full report in build artifacts |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What & why
Next behavior-preserving extraction for #592 (breaking up the app's two largest
classes, one extraction per PR, ~800-line target). Follows the playbook from the
merged
SessionSampleWriter(#604) andSessionDataRepository(#607): a sealedcollaborator, constructor-only dependencies, no reach into desktop singletons,
unit-testable in isolation.
This PR moves the pure OxyPlot construction out of
DatabaseLoggerinto a newPlotModelFactory. The logger stays the orchestration + live-modelmutation/binding surface (the
LoggedDataPanePrototype.xamlbinding contract —PlotModel,MinimapPlotModel,DeviceLegendGroups,HasSessionData,IsRefiningData,IsLegendPanelVisible,CurrentSession.*, and theZoom*/ResetZoom/SaveGraph/ToggleLegendPanelcommands — is unchanged).DatabaseLogger.cs: 1,376 → 1,119 lines.Moved into
PlotModelFactory(construction only — no EF, threading, Dispatcher,InvalidatePlot, or axis mutation)CreateMainPlotModel— the main model + Analog/Digital/TimeLinearAxisconfig,
OxyPlotDarkTheme.ApplyTo, axisAdd,IsLegendVisible=false.CreateMinimapPlotModel— the minimap model, its two non-interactive axes,and the three
RectangleAnnotations (selection rect + two dim overlays).Returns a
MinimapPlotComponentsrecord so the logger keeps the annotationhandles it mutates.
CreateChannelSeries— a channel'sLineSeries+LoggedSeriesLegendItemwith the Analog/Digital
YAxisKeymapping. The legend item still gets the livePlotModel+DatabaseLogger, so its visibility toggle invalidates the plotand syncs the minimap exactly as before.
CreateMinimapSeries— the per-channel minimapLineSeriesconstructiononly; the apply-to-live half of
SetupMinimapSeries(Series.Clear/Add, the_minimapSeriesmap,axis.Zoom, annotation bounds,InvalidatePlot) stays inthe logger.
LoggedSeriesLegendIteminto its own file (one-main-class-per-fileper the style guide) and annotates its optional logger as nullable.
The shared axis-key strings (
Analog/Digital/Time/MinimapTime/MinimapY)become
public consts onPlotModelFactory— the single source of truth thefactory builds from and the logger's viewport lookups key on.
Kept in
DatabaseLogger(viewport/live-model coupled — untouched)Every viewport/minimap-sync method (
OnMainTimeAxisChanged, throttle/settleticks,
UpdateMainPlotViewport,UpdateSeriesFromMemory,FetchViewportDataFromDb,OnMinimap*,SetMinimapSeriesVisibility), the twoDispatcherTimers, theAxisChangedsubscription, theMinimapInteractionControllerwiring, the annotation fields, and everyInvalidatePlot/Zoom/axis-mutation call. The ADR-001 viewport controller is adeliberately separate, future extraction — not this PR.
Invariants preserved (ADR-001 / CLAUDE.md "Plot Rendering")
InvalidatePlot(true)on everyItemsSourcechange — all stay in the logger.ResetAllAxes()with downsampled data;ResetZoomstill does per-axisReset()+ explicittimeAxis.Zoom(...).IsSyncingFromMinimapguard still wraps programmatic axis changes._downsampledCachestill reused — no new per-frame allocations.ClearPlot()still resets every axis on session switch.The factory holds zero
InvalidatePlot/Zoom/axis-mutation calls.Tests
PlotModelFactoryTests(13 tests, no DB / WPF runtime / threads — a realtest seam where none existed): axis keys/positions, the Analog→
Analog/Digital→
DigitalYAxisKeymapping, color parsing, minimap annotation/axisconfig, annotation z-order, and returned-handle identity.
dotnet test --filter "TestCategory!=Ui&FullyQualifiedName!~WindowsFirewallWrapperTests").scenarios pass:
LoggingSessionTests.StartLoggingSession_RendersLivePlot_RunsStopsAndDeletesSessionand
LoggingSessionTests.ViewLoggedSession_LoadsStoredSessionOntoLoggedDataPlot.Also updates the CLAUDE.md plot-gotchas Key files list to name
PlotModelFactory.cs.🤖 Generated with Claude Code