Persist drawing colors#69
Conversation
- Migrate drawing elements and serializers from `Color` strings to `colorValue` - Preserve legacy JSON and Hive payloads during read - Add regression tests for JSON and Hive color persistence
- Register source-owned adapters for free, line, rectangle, and ellipse drawings - Preserve legacy color fields while writing the current colorValue field - Update generated Hive metadata and registrar outputs
SunkenInTime
merged commit e6c8e78
into
feature/custom-color-palette
There was a problem hiding this comment.
Pull request overview
This PR standardizes drawing color persistence by storing colors as ARGB int values across JSON exports and Hive storage, while maintaining backwards compatibility with legacy payload formats.
Changes:
- Persist drawing element colors via
colorValue(ARGBint) in JSON serialization/deserialization, with legacy stringcolorsupport. - Replace generated Hive adapters for drawing elements with source-owned custom adapters that store integer color fields and can interpret legacy payloads.
- Add test coverage for JSON + Hive color persistence (drawings, tags, utilities, and circle agent colors).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/drawing_provider_test.dart | Extends JSON round-trip assertions to include colorValue for drawings. |
| test/color_persistence_test.dart | Adds comprehensive JSON + Hive color persistence tests (including legacy cases). |
| lib/providers/strategy_provider.dart | Updates drawing migration to use colorValue when recreating shifted elements. |
| lib/providers/drawing_provider.dart | Switches drawing JSON encoding to colorValue and adds legacy-aware color parsing helper. |
| lib/hive/hive_registration.dart | Registers custom drawing adapters explicitly (idempotently) in addition to generated registrar. |
| lib/hive/hive_registrar.g.dart | Stops auto-registering drawing adapters (now owned/registered in source). |
| lib/hive/hive_adapters.g.yaml | Removes generated adapter specs for drawing element types (now custom). |
| lib/hive/hive_adapters.g.dart | Removes generated drawing adapters (replaced by custom ones in source). |
| lib/hive/hive_adapters.dart | Introduces custom drawing adapters that persist integer color values and read legacy fields. |
| lib/const/drawing_element.g.dart | Updates JSON serialization codegen to use colorValue for drawings. |
| lib/const/drawing_element.dart | Refactors drawing elements to store colorValue and expose color as a derived getter; adds legacy JSON parsing hooks. |
| AGENTS.md | Adds guidance not to manually edit generated files. |
Comments suppressed due to low confidence (1)
lib/providers/strategy_provider.dart:841
shiftDrawingsshifts Line/FreeDrawing/RectangleDrawing when migrating to 16:9, but it falls through toreturn element;for EllipseDrawing. That means ellipse drawings will keep pre-migration coordinates and end up misaligned relative to everything else. Add an EllipseDrawing branch that constructs a shifted ellipse (including boundingBox).
if (element is RectangleDrawing) {
return RectangleDrawing(
start: shift(element.start),
end: shift(element.end),
colorValue: element.colorValue,
thickness: element.thickness,
boundingBox: shiftBoundingBox(element.boundingBox),
isDotted: element.isDotted,
hasArrow: element.hasArrow,
id: element.id,
);
}
return element;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final int colorValue => colorValue, | ||
| final num colorValue => colorValue.toInt(), | ||
| final Color color => color.toARGB32(), | ||
| _ => 0xFFFFFFFF, |
There was a problem hiding this comment.
_readDrawingHiveColorValue silently falls back to 0xFFFFFFFF when it encounters an unexpected value type. This can mask corrupted/unsupported Hive payloads and make debugging data issues harder. Consider throwing (or at least logging) on unknown types, or using a clearly documented default derived from app settings.
| _ => 0xFFFFFFFF, | |
| _ => throw StateError( | |
| 'Unsupported Hive drawing color value: $value ' | |
| '(type: ${value.runtimeType}) for colorFieldIndex=$colorFieldIndex ' | |
| 'legacyColorValueFieldIndex=$legacyColorValueFieldIndex', | |
| ), |
| if (!Hive.isAdapterRegistered(20)) { | ||
| registerIcarusAdapters(Hive); | ||
| } |
There was a problem hiding this comment.
_ensureAdaptersRegistered checks Hive.isAdapterRegistered(20) as a proxy for “icarus adapters are registered”. This is brittle (typeIds can change as models evolve) and is unnecessary now that registerIcarusAdapters itself is idempotent. Prefer calling registerIcarusAdapters(Hive) unconditionally here, or checking a well-known adapter id constant used by this test (e.g., one of the drawing adapter typeIds).
| if (!Hive.isAdapterRegistered(20)) { | |
| registerIcarusAdapters(Hive); | |
| } | |
| registerIcarusAdapters(Hive); |
| - **Linux build deps.** `clang cmake ninja-build pkg-config libgtk-3-dev liblzma-dev libstdc++-14-dev` must be installed for Linux desktop builds. | ||
| - **Code generation.** After changing Hive models, Riverpod providers, or JSON-serializable classes, regenerate with: `fvm flutter pub run build_runner build --delete-conflicting-outputs`. | ||
| - **Generated files are outputs, not edit targets.** Never manually edit generated files like `*.g.dart`, `*.g.yaml`, or registrar outputs. Make changes in the source files that drive generation, or replace generated behavior with explicit source-owned code such as a custom adapter, then regenerate. | ||
| - **No automated tests exist** in this codebase. `flutter test` will find nothing. |
There was a problem hiding this comment.
The docs now say generated files shouldn’t be edited (good), but the next bullet still claims “No automated tests exist”. This is incorrect (the repo has many test/ suites, and this PR adds more), and it can mislead contributors into skipping flutter test. Please update/remove that bullet to reflect the current test setup.
| - **No automated tests exist** in this codebase. `flutter test` will find nothing. | |
| - **Automated tests exist.** Run `fvm flutter test` to execute the current test suites. |
Greptile SummaryThis PR normalizes drawing colors to ARGB Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/clarity issues with no correctness impact The core migration logic is correct: field indices match the old generated adapters, both new int and legacy Color/string formats are handled at every read path, and the new tests verify round-trips and backward compatibility. The three P2 comments do not affect runtime behavior. AGENTS.md (stale test documentation) and lib/hive/hive_adapters.dart (legacyColorValueFieldIndex clarity, silent white fallback) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Write["Serialize (new)"]
W1["DrawingElement\ncolorValue: int"] -->|"Hive write\nfield 2 = int"| W2["CustomAdapter\n(FreeDrawing/Line/Rect/Ellipse)"]
W1 -->|"JSON toJson\n'colorValue': int"| W3["Map String dynamic"]
end
subgraph Read["Deserialize (legacy-safe)"]
R1["BinaryReader\nfield 2 = Color OR int"] -->|"_readDrawingHiveColorValue\nswitch handles both"| R2["colorValue: int"]
R3["JSON 'colorValue' OR 'color' string"] -->|"_readDrawingColorJsonValue\n+ fromJson helpers"| R4["colorValue: int"]
end
R2 --> DE["DrawingElement\ncolorValue: int\nColor get color"]
R4 --> DE
Reviews (1): Last reviewed commit: "Persist drawing colors with custom Hive ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - **Code generation.** After changing Hive models, Riverpod providers, or JSON-serializable classes, regenerate with: `fvm flutter pub run build_runner build --delete-conflicting-outputs`. | ||
| - **Generated files are outputs, not edit targets.** Never manually edit generated files like `*.g.dart`, `*.g.yaml`, or registrar outputs. Make changes in the source files that drive generation, or replace generated behavior with explicit source-owned code such as a custom adapter, then regenerate. | ||
| - **No automated tests exist** in this codebase. `flutter test` will find nothing. | ||
| - **Lint.** `fvm flutter analyze` — expect ~70 pre-existing warnings/infos (unused imports, deprecated APIs). No errors. |
There was a problem hiding this comment.
This line still says flutter test will find nothing, but this PR introduces test/color_persistence_test.dart and extends test/drawing_provider_test.dart. Leaving this note as-is will cause future contributors and automated agents to skip running or writing tests entirely.
| - **Lint.** `fvm flutter analyze` — expect ~70 pre-existing warnings/infos (unused imports, deprecated APIs). No errors. | |
| - **Tests.** Run `fvm flutter test` to execute the test suite. Add new tests under `test/` for persistence or provider logic changes. |
| } | ||
| } | ||
|
|
||
| class EllipseDrawingAdapter extends TypeAdapter<EllipseDrawing> { | ||
| @override | ||
| final typeId = ellipseDrawingAdapterTypeId; | ||
|
|
||
| @override | ||
| EllipseDrawing read(BinaryReader reader) { | ||
| final numOfFields = reader.readByte(); | ||
| final fields = <int, dynamic>{ | ||
| for (int i = 0; i < numOfFields; i++) reader.readByte(): reader.read(), | ||
| }; | ||
|
|
||
| return EllipseDrawing( | ||
| start: fields[0] as Offset, |
There was a problem hiding this comment.
legacyColorValueFieldIndex always points to a non-existent field
For every adapter (FreeDrawing: 12, Line: 10, RectangleDrawing: 8, EllipseDrawing: 8), the legacyColorValueFieldIndex value is the nextIndex from the old .g.yaml — a slot that was never written by the old generated adapter. fields[thatIndex] is always null, so the fallback to fields[colorFieldIndex: 2] always fires. The new int and the old Color value both live at the same field slot (2), and the switch in _readDrawingHiveColorValue already handles both types at that slot.
The parameter effectively does nothing in all current usages, but its misleading name implies there was once a separate "legacy" slot. Consider simplifying by always passing legacyColorValueFieldIndex: null (or removing the parameter) and relying on the switch to differentiate Color from int at colorFieldIndex.
| final numOfFields = reader.readByte(); | ||
| final fields = <int, dynamic>{ | ||
| for (int i = 0; i < numOfFields; i++) reader.readByte(): reader.read(), | ||
| }; | ||
|
|
||
| return EllipseDrawing( |
There was a problem hiding this comment.
The wildcard arm _ => 0xFFFFFFFF silently replaces any value that isn't an int, num, or Color with opaque white. This means corrupt or unexpectedly-typed Hive data would render as invisible-on-light-background white lines with no diagnostic signal. Throwing (or at least logging a warning) would make data issues far easier to detect during development.
Summary
Testing