feat: Support for Multiple LED Badge sizes #1347
Open
Conversation
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
Reviewer's GuideThis PR adds dynamic support for multiple LED badge sizes by introducing a ScreenSize model and replacing all hard-coded grid dimensions with runtime-configurable values, propagating the selected size through UI widgets, providers, converters, custom painters, utilities, and tests. Sequence diagram for badge size selection and propagationsequenceDiagram
actor User
participant HomeScreen
participant AnimationBadge
participant DrawBadge
participant SaveBadgeDialog
participant SaveBadgeProvider
User->>HomeScreen: Selects badge size from dropdown
HomeScreen->>AnimationBadge: Passes selected ScreenSize
User->>DrawBadge: Navigates to draw badge
DrawBadge->>DrawBadge: Initializes grid with selected ScreenSize
User->>SaveBadgeDialog: Opens save dialog
SaveBadgeDialog->>SaveBadgeProvider: Saves badge with selected ScreenSize
SaveBadgeProvider->>SaveBadgeProvider: Stores badge data with size info
Class diagram for ScreenSize and supportedScreenSizesclassDiagram
class ScreenSize {
+int width
+int height
+String name
+operator ==(Object other)
+int hashCode
}
class supportedScreenSizes {
<<constant>>
+List<ScreenSize>
}
Class diagram for updated AnimationBadgeProvider and DrawBadgeProviderclassDiagram
class AnimationBadgeProvider {
-List<List<bool>> _paintGrid
-List<List<bool>> _newGrid
-List<List<List<bool>>> _frames
-int _currentFrame
+void initGrids(ScreenSize size)
+void badgeAnimation(String, Converters, bool, ScreenSize)
+List<List<bool>> getPaintGrid()
+List<List<bool>> getNewGrid()
}
class DrawBadgeProvider {
-List<List<bool>> _drawViewGrid
-ScreenSize _currentSize
+void initGridWithSize(ScreenSize size)
+List<List<bool>> getDrawViewGrid()
+ScreenSize getCurrentSize()
}
Class diagram for updated UI widgets with ScreenSize supportclassDiagram
class AnimationBadge {
+ScreenSize selectedSize
}
class BMBadge {
+ScreenSize selectedSize
}
class EffectContainer {
+ScreenSize selectedSize
}
class EffectTab {
+ScreenSize selectedSize
}
class SaveBadgeCard {
+ScreenSize selectedSize
}
class BadgeListView {
+ScreenSize selectedSize
}
class SavedClipartListView {
+ScreenSize selectedSize
}
class SaveBadgeDialog {
+ScreenSize selectedSize
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
samruddhi-Rahegaonkar
force-pushed
the
issue1344
branch
12 times, most recently
from
fc0ac96 to
74a1dda
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
- This change spreads the new ScreenSize parameters into almost every widget and provider method; consider using a single Provider or InheritedWidget to hold the selected badge size so you don’t have to modify every signature and widget.
- The Converters.messageTohex method has become extremely large with mixed responsibilities (emoji tags, char scaling, bitmap scaling); refactor by extracting emoji, character, and bitmap conversion into separate helper classes or private services to improve readability and testability.
- BadgePaint (and other rendering code) still uses magic scaling factors like 0.93 and 0.5—extract these into named constants or compute them dynamically from width/height ratios to avoid unexpected layout issues on non-standard sizes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- This change spreads the new ScreenSize parameters into almost every widget and provider method; consider using a single Provider or InheritedWidget to hold the selected badge size so you don’t have to modify every signature and widget.
- The Converters.messageTohex method has become extremely large with mixed responsibilities (emoji tags, char scaling, bitmap scaling); refactor by extracting emoji, character, and bitmap conversion into separate helper classes or private services to improve readability and testability.
- BadgePaint (and other rendering code) still uses magic scaling factors like 0.93 and 0.5—extract these into named constants or compute them dynamically from width/height ratios to avoid unexpected layout issues on non-standard sizes.
## Individual Comments
### Comment 1
<location> `lib/providers/animation_badge_provider.dart:110` </location>
<code_context>
}
void startTimer() {
+ if (_newGrid.isEmpty || _newGrid[0].isEmpty) {
+ logger.w("Cannot start animation timer: _newGrid is empty");
+ return;
</code_context>
<issue_to_address>
startTimer silently returns if _newGrid is empty, which may mask upstream issues.
Consider surfacing this condition as an error or notifying the caller, so they are aware the animation did not start.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Hey @sourcery-ai ,
Isn't It ? |
samruddhi-Rahegaonkar
force-pushed
the
issue1344
branch
3 times, most recently
from
1038c33 to
e334dbe
Compare
samruddhi-Rahegaonkar
force-pushed
the
issue1344
branch
2 times, most recently
from
4d3d10a to
856c79e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
- Consider centralizing the selected ScreenSize in a provider or InheritedWidget so you don’t have to prop-drill it through every widget and method call.
- The Converters class now mixes text parsing, bitmap scaling, emoji handling, and hex formatting—splitting it into focused services (e.g. TextConverter, ImageConverter) will improve readability and testability.
- There are multiple bitmap scaling and trimming routines (_scaleBitmapToBadgeSize, _scaleTextCharacterToBadgeSize, convertBitmapToLEDHex, etc.); consolidating them into shared utilities will reduce duplication and ensure consistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the selected ScreenSize in a provider or InheritedWidget so you don’t have to prop-drill it through every widget and method call.
- The Converters class now mixes text parsing, bitmap scaling, emoji handling, and hex formatting—splitting it into focused services (e.g. TextConverter, ImageConverter) will improve readability and testability.
- There are multiple bitmap scaling and trimming routines (_scaleBitmapToBadgeSize, _scaleTextCharacterToBadgeSize, convertBitmapToLEDHex, etc.); consolidating them into shared utilities will reduce duplication and ensure consistent behavior.
## Individual Comments
### Comment 1
<location> `lib/providers/draw_badge_provider.dart:25` </location>
<code_context>
- }
+ final rows = _drawViewGrid.length;
+ final cols = _drawViewGrid.isNotEmpty ? _drawViewGrid[0].length : 0;
+ for (int i = 0; i < rows && i < badgeData.length; i++) {
+ for (int j = 0; j < cols && j < badgeData[0].length; j++) {
+ _drawViewGrid[i][j] = badgeData[i][j];
</code_context>
<issue_to_address>
updateDrawViewGrid only copies overlapping regions, which may leave parts of the grid unchanged.
If badgeData is smaller than the grid, leftover cells may show outdated data. Please clear or reset all grid cells before copying new values.
</issue_to_address>
### Comment 2
<location> `lib/virtualbadge/view/badge_paint.dart:50` </location>
<code_context>
+ final int rows = grid.length;
+
+ // Adjust cell size to fit all pixels inside the badge area
+ final double cellWidth = badgeWidth / (cols * 0.93);
+ final double cellHeight = badgeHeight / rows;
+ final double cellSize = math.min(cellWidth, cellHeight);
</code_context>
<issue_to_address>
The cell width uses a 0.93 scaling factor, which may not be intuitive.
Please document the choice of 0.93 or make it a configurable parameter to improve clarity and adaptability.
</issue_to_address>
### Comment 3
<location> `lib/virtualbadge/view/badge_paint.dart:97` </location>
<code_context>
- bool shouldRepaint(covariant CustomPainter oldDelegate) {
- return true;
- }
+ bool shouldRepaint(covariant CustomPainter oldDelegate) => true;
}
</code_context>
<issue_to_address>
shouldRepaint always returns true, which may cause unnecessary repaints.
Compare the old and new grid values in shouldRepaint to avoid unnecessary repaints and improve performance.
Suggested implementation:
```
@override
bool shouldRepaint(covariant CustomPainter oldDelegate) {
if (oldDelegate is! YourPainterClassName) return true;
return oldDelegate.grid != grid;
}
}
```
- Replace `YourPainterClassName` with the actual name of your CustomPainter subclass.
- Ensure that `grid` is a field of your painter class and that it implements `==` properly (e.g., use `ListEquality` from `collection` if it's a List).
- If `grid` is a complex object, you may need to implement or use a deep equality check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Multiple.badge.size.mp4 |
|
The screen size is relevant to the screen, but the spacing to the above screen is larger than the spacing to the text box. Please make it the other way around. 
|
|
@mariobehling We can maximum have the above one or the centered one because the widget takes its space too. |
|
@mariobehling I think the current one is better than to be centered with the white space above and below. |
Please see image. Do not change the color of the button, keep it white as before. Do not introduce a new gray color into the app.
|
No, I had made a grey color to show the space taking by the widget. |
|
|
@mariobehling This is the maxiumum closer we can make to the Badge preview further more it will overlay on the badge. |
|
@mariobehling Can we go with this ? |
|
@samruddhi-Rahegaonkar why there is changes in actions. and this is a really long PR 30 files changes it becomes impossible to review. |
I have attached Video and changes are not that much just passed the Screen size parameters to evry file and tests thats why file changed are very large. |
|
@Jhalakupadhyay This PR was too old and after resolving cinflicts it has grown. |
|
hey @samruddhi-Rahegaonkar can you resolve the conflicts. |
|
@nope3472 I will resolve it by end of the day. |
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 #1314
Changes
Screenshots / Recordings
Screen.Recording.2025-06-26.at.6.49.21.PM.mov
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Introduce dynamic support for multiple LED badge sizes by adding a ScreenSize model and propagating variable dimensions throughout the UI, providers, converters, painting logic, and tests, while enabling users to select their badge size at runtime.
New Features:
Enhancements:
CI:
Tests:
Summary by Sourcery
Support multiple LED badge sizes by introducing a ScreenSize model and wiring dynamic width/height parameters throughout the app to enable scalable layouts in all drawing, animation, effect, saving, and transfer features.
New Features:
Enhancements:
CI:
Tests: