fix: added canvas color change support according to the display's colors using ColorPaletteProvider#45
Merged
AsCress merged 2 commits intofossasia:mainfrom Jun 5, 2025
Conversation
Contributor
Reviewer's GuideImplements dynamic canvas color cycling by sourcing the display’s palette from ColorPaletteProvider and corrects the ordering of color arrays in EPD utility classes. Sequence Diagram: Dynamic Canvas Color UpdatesequenceDiagram
actor User
participant MBIES as MovableBackgroundImageExampleState
participant GSL as getItLocator
participant CPP as ColorPaletteProvider
%% Initialization of availableCanvasColors (typically happens once)
MBIES->>GSL: getIt<ColorPaletteProvider>()
activate GSL
GSL-->>MBIES: ColorPaletteProvider instance
deactivate GSL
MBIES->>CPP: .colors (access property)
activate CPP
CPP-->>MBIES: availableCanvasColors
deactivate CPP
%% User triggers the canvas color change
User->>MBIES: Triggers changeCanvasColor()
activate MBIES
MBIES->>MBIES: currentCanvasColorIndex = (idx + 1) % availableCanvasColors.length
MBIES->>MBIES: newColor = availableCanvasColors[currentCanvasColorIndex]
MBIES->>MBIES: _currentCanvasColor = _getCanvasColorName(newColor)
MBIES->>MBIES: setState()
deactivate MBIES
Class Diagram: Updates for Dynamic Canvas Color PaletteclassDiagram
class _MovableBackgroundImageExampleState {
+List~Color~ availableCanvasColors
+int currentCanvasColorIndex
-_getCanvasColorName(Color color) String
+_changeCanvasColor() void
}
class ColorPaletteProvider {
+List~Color~ colors
}
class Gdey037z03 {
+List~Color~ colors
}
class Gdey037z03BW {
+List~Color~ colors
}
_MovableBackgroundImageExampleState ..> ColorPaletteProvider : uses colors from
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey @Vishveshwara - I've reviewed your changes - here's some feedback:
- Rather than mapping Colors to hard-coded string names in
_getCanvasColorName, consider storing the current canvas color as a Color (or using a name–color pair from your provider) to eliminate the manual mapping and support future palette changes. - Since
availableCanvasColorsis captured once viagetIt, any runtime updates to the ColorPaletteProvider won’t be reflected—consider listening to the provider or rebuilding that list inbuild/initStateso it stays in sync.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Add this method to handle canvas color changes | ||
| final List<Color> availableCanvasColors = | ||
| getIt<ColorPaletteProvider>().colors; | ||
| int currentCanvasColorIndex = 0; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Initial canvas color state may be out of sync
Initialize _currentCanvasColor in initState using currentCanvasColorIndex to keep the displayed name and actual color in sync.
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.
Fixes #44
Before the Fix(The black/white display should not switch to the red canvas)
canvas_color_issue.mp4
After The Fix
canvas_color_fixed.mp4
Summary by Sourcery
Enable dynamic cycling of canvas background colors based on each display’s supported color palette and prevent unsupported red canvas on black/white e-paper displays.
New Features:
Bug Fixes:
Enhancements: