Remove team color dependency from drawings#71
Merged
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
Persist drawing colors
- Introduce a reusable BetterColorPicker style and palette - Update color picker consumers to use the new custom styling
- Persist custom swatch colors in app preferences - Replace hardcoded color pickers with shared color library UI - Allow adding, editing, and deleting custom colors
- Persist neutral team colors in strategy settings and app defaults - Apply neutral marker styling across strategy pages and agent widgets - Add coverage for strategy settings JSON and shade conversion
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the drawing “team color” dependency by standardizing color persistence around ARGB integers (colorValue) across JSON + Hive, while adding UI/support for neutral team colors and a reusable/customizable color library.
Changes:
- Migrates drawing serialization/deserialization to persist ARGB ints (
colorValue) with backward compatibility for legacy string/Color payloads. - Adds neutral team color support plus strategy/global default settings (agent size, ability size, neutral colors) persisted via
AppPreferences. - Replaces
flutter_colorpickerusage with an in-repoBetterColorPickerand updates color-picking UI to use the new sharedColorLibrary.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/strategy_settings_provider_test.dart | Adds unit coverage for useNeutralTeamColors JSON default/persistence and neutral shading behavior. |
| test/drawing_provider_test.dart | Updates drawing JSON tests to assert colorValue is emitted and restored. |
| test/color_persistence_test.dart | Adds JSON + Hive regression tests for int color persistence and legacy payload compatibility. |
| pubspec.yaml | Removes flutter_colorpicker dependency. |
| pubspec.lock | Removes flutter_colorpicker lock entry and updates transitive versions. |
| lib/widgets/sidebar_widgets/text_tools.dart | Switches text tag color selection to shared ColorLibrary. |
| lib/widgets/sidebar_widgets/drawing_tools.dart | Switches pen color selection to shared ColorLibrary. |
| lib/widgets/sidebar_widgets/custom_shape_tools.dart | Switches custom shape color selection to shared ColorLibrary. |
| lib/widgets/sidebar_widgets/color_library.dart | Adds a reusable palette widget with custom color add/edit/delete via dialog. |
| lib/widgets/settings_tab.dart | Adds strategy vs global settings modes; introduces neutral team colors + new-strategy defaults UI. |
| lib/widgets/map_theme_settings_section.dart | Replaces flutter_colorpicker dialog content with BetterColorPicker. |
| lib/widgets/icarus_color_picker_style.dart | Defines shared styling for BetterColorPicker across dialogs. |
| lib/widgets/folder_edit_dialog.dart | Replaces folder color custom picker with BetterColorPicker. |
| lib/widgets/draggable_widgets/shared/framed_ability_icon_shell.dart | Applies neutral team shading to ally/enemy outlines when enabled. |
| lib/widgets/draggable_widgets/image/placed_image_builder.dart | Replaces hardcoded tag palette with colorLibraryProvider entries. |
| lib/widgets/draggable_widgets/agents/agent_widget.dart | Applies neutral team coloring to agent background/outline when enabled. |
| lib/widgets/draggable_widgets/agents/agent_feedback_widget.dart | Applies neutral team coloring to drag feedback background/outline when enabled. |
| lib/widgets/dialogs/upload_image_dialog.dart | Switches image tag color selection to shared ColorLibrary. |
| lib/widgets/better_color_picker.dart | Introduces in-repo color picker widget (replacing external dependency usage). |
| lib/providers/strategy_settings_provider.g.dart | Adds JSON field for useNeutralTeamColors. |
| lib/providers/strategy_settings_provider.dart | Adds useNeutralTeamColors to model/provider state. |
| lib/providers/strategy_provider.dart | Writes drawing elements using colorValue; applies neutral colors across pages; uses app defaults for new strategies. |
| lib/providers/map_theme_provider.dart | Extends AppPreferences to persist defaults + custom colors; adds setters. |
| lib/providers/drawing_provider.dart | Writes colorValue to JSON and reads legacy formats via _readDrawingColorValue. |
| lib/providers/color_library_provider.dart | Adds providers/controller for default + custom color library persisted in AppPreferences. |
| lib/hive/hive_registration.dart | Ensures new custom drawing adapters are registered for Hive + isolated Hive. |
| lib/hive/hive_registrar.g.dart | Removes drawing adapters from generated registrar (now registered explicitly). |
| lib/hive/hive_adapters.g.yaml | Updates Hive type schemas for new/changed fields and removes generated drawing adapters. |
| lib/hive/hive_adapters.g.dart | Updates Hive adapters for updated fields (StrategySettings/AppPreferences) and removes generated drawing adapters. |
| lib/hive/hive_adapters.dart | Adds explicit drawing adapters persisting int colors + legacy read support. |
| lib/const/settings.dart | Adds neutralTeamShade helper. |
| lib/const/drawing_element.g.dart | Updates JSON serialization to use colorValue. |
| lib/const/drawing_element.dart | Reworks drawing element color storage to colorValue with derived color getter + legacy JSON read support. |
| AGENTS.md | Adds guidance about not manually editing generated files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
21
to
31
| StrategySettings copyWith({ | ||
| double? agentSize, | ||
| double? abilitySize, | ||
| bool? useNeutralTeamColors, | ||
| bool? isOpen, | ||
| }) { | ||
| return StrategySettings( | ||
| agentSize: agentSize ?? this.agentSize, | ||
| abilitySize: abilitySize ?? this.abilitySize, | ||
| useNeutralTeamColors: useNeutralTeamColors ?? this.useNeutralTeamColors, | ||
| ); |
There was a problem hiding this comment.
StrategySettings.copyWith accepts an isOpen parameter but StrategySettings has no isOpen field and the returned instance never uses this value. This makes isOpen a no-op and potentially misleading. Either remove isOpen from copyWith (and any callers) or add a persisted isOpen field if it's intended to be part of the model/state.
| - **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.
AGENTS.md now says “No automated tests exist … flutter test will find nothing”, but the repo has a substantial test/ suite (and this PR adds/updates tests). This guidance is now misleading—please update/remove that bullet so contributors run the tests instead of skipping them.
Suggested change
| - **No automated tests exist** in this codebase. `flutter test` will find nothing. | |
| - **Tests.** Run `fvm flutter test` to execute the automated test suite. |
Contributor
Greptile SummaryThis PR migrates drawing element color storage from
Confidence Score: 3/5Not safe to merge as-is; the neutral-color setting silently reverts for non-active pages after a page switch. One clear P1 defect: lib/providers/strategy_provider.dart — missing Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SettingsTab (UI)
participant SSP as strategySettingsProvider
participant SP as StrategyProvider
participant Hive as Hive Box
UI->>SSP: updateNeutralTeamColors(value)
SSP->>SSP: state = state.copyWith(useNeutralTeamColors: value)
UI->>SP: applyNeutralTeamColorsToAllPages(value)
SP->>SP: _syncCurrentPageToHive()
SP->>Hive: box.get(state.id)
Hive-->>SP: StrategyData (all pages)
SP->>SP: newPages = pages.map(p => p.copyWith(settings(...value)))
SP->>Hive: box.put(updated.id, updated)
Note over SP: state NOT updated with newPages
SP->>SP: setUnsaved()
Note over UI,SP: User switches page
UI->>SSP: reads strategySettingsProvider
SSP->>SP: reads state.pages[newPage].settings
Note over SP,SSP: stale state — useNeutralTeamColors = old value
Reviews (1): Last reviewed commit: "Add neutral team color settings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
3672
to
3707
| setUnsaved(); | ||
| } | ||
|
|
||
| Future<void> applyNeutralTeamColorsToAllPages(bool value) async { | ||
| if (state.stratName == null) return; | ||
|
|
||
| await _syncCurrentPageToHive(); | ||
|
|
||
| final box = Hive.box<StrategyData>(HiveBoxNames.strategiesBox); | ||
| final strat = box.get(state.id); | ||
| if (strat == null || strat.pages.isEmpty) return; | ||
|
|
||
| final newPages = [ | ||
| for (final page in strat.pages) | ||
| page.copyWith( | ||
| settings: page.settings.copyWith(useNeutralTeamColors: value), | ||
| ), | ||
| ]; | ||
|
|
||
| final strategyTheme = ref.read(strategyThemeProvider); | ||
| final updated = strat.copyWith( | ||
| pages: newPages, | ||
| mapData: ref.read(mapProvider).currentMap, | ||
| themeProfileId: strategyTheme.profileId, | ||
| clearThemeProfileId: strategyTheme.profileId == null, | ||
| themeOverridePalette: strategyTheme.overridePalette, | ||
| clearThemeOverridePalette: strategyTheme.overridePalette == null, | ||
| lastEdited: DateTime.now(), | ||
| ); | ||
| await box.put(updated.id, updated); | ||
| setUnsaved(); | ||
| } | ||
|
|
||
| void moveToFolder({required String strategyID, required String? parentID}) { | ||
| final strategyBox = Hive.box<StrategyData>(HiveBoxNames.strategiesBox); | ||
| final strategy = strategyBox.get(strategyID); |
Contributor
There was a problem hiding this comment.
state not updated after writing to Hive
applyNeutralTeamColorsToAllPages writes updated pages to Hive but never updates the in-memory state. strategySettingsProvider is derived from the current page's settings in state, so switching to any other page after calling this method will cause strategySettingsProvider to re-initialize with the stale (un-updated) useNeutralTeamColors = false value from state.pages, silently reverting the setting for every non-active page.
The newPages list already has the correct values; the state just needs to be updated alongside Hive:
await box.put(updated.id, updated);
+ state = state.copyWith(/* rebuild state pages from newPages */);
setUnsaved();
Comment on lines
915
to
+924
| } | ||
| } | ||
|
|
||
| int _readDrawingColorValue(Map<String, dynamic> json) { | ||
| final value = json['colorValue'] ?? json['color']; | ||
| if (value is int) return value; | ||
| if (value is num) return value.toInt(); | ||
| if (value is String) { | ||
| return const ColorConverter().fromJson(value).toARGB32(); | ||
| } |
Contributor
There was a problem hiding this comment.
null color inconsistent with Hive path
When both 'colorValue' and 'color' keys are absent (e.g., a corrupt or partially-migrated JSON blob), value is null and throw UnsupportedError(...) is reached. The Hive path in _readDrawingHiveColorValue quietly falls through to the _ => 0xFFFFFFFF default rather than crashing. A defensive fallback here would match that behaviour and prevent a potential crash on malformed data.
int _readDrawingColorValue(Map<String, dynamic> json) {
final value = json['colorValue'] ?? json['color'];
if (value is int) return value;
if (value is num) return value.toInt();
if (value is String) {
return const ColorConverter().fromJson(value).toARGB32();
}
return 0xFFFFFFFF; // default to opaque white instead of throwing
}- Tighten sidebar and segmented tab corner radii - Adjust settings dialog padding and simplify mode switcher - Add a PowerShell watcher script for Flutter hot reload
- Render the active page name in the lower-left corner - Pass page names through the save/load screenshot flow - Add a widget test covering the new label
- Reduce bar and row corner radii for a cleaner fit - Adjust expanded panel spacing and resize handle height - Refresh row action buttons with compact icon spacing
- Store pages bar width in app preferences - Add horizontal resize handle and clamp width - Keep existing height resize behavior intact
- Track source and target page IDs through transition state - Ease the overlay progress curve and drive row fill indicators - Clear transition page IDs when the animation completes
- Extract lineup visuals into a reusable overlay - Show lineup overlay while page transitions hide the map - Use a consistent easing curve for transition progress
- Watch canvas resize changes in persistent lineup widgets - Add regression test for overlay repositioning and rescaling
- Remove extra button spacing in the collapsed and expanded bars - Hide row actions until hover or active state - Disable outer scrollbars for the reorder list - Centralize pages bar corner radii constants
- Read custom color library entries from `ColorLibraryController` - Stop watching preferences directly in the controller build - Keep controller state in sync before persisting preference updates
- Duplicate abilities on modifier-assisted drag - Track dragged IDs through drop handlers - Update ability widget tests for the new callback signature
- Add the impeccable skill docs, commands, and references - Update map theme and settings UI to match the new design flow - Refresh strategy folder import test expectations
- Replace the plain title/description block with a compact scope pill - Simplify the strategy object styling description copy
- Store custom keybinds in app preferences - Add a settings section to edit, reset, and search bindings - Scope text fields with shortcut overrides and add binding tests
- Move map theme state into `user_preferences_provider` - Update imports across app and tests
- Add spawn barrier, ult orb, and region name flags to app preferences - Sync map visibility state with saved preferences and archive exports
- Show draggable agent abilities in the context menu - Switch the marker sync banner action to a solid button
- Switch to a raw menu item with tighter spacing - Remove the tooltip wrapper from ability buttons
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
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
Testing