feat: created animations that can be transferred to the badge #1381
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 GuideIntroduces four new badge animations (Pacman, Chevron, Diamond, Broken Hearts) with frame‐by‐frame transfer functions, refactors and unifies animation transfer logic in providers, updates UI to support new animations in a 3-per-row grid with icons, and improves speed mapping, state reset, and mouth-closing final frame for smooth looping. Sequence diagram for unified animation transfer logicsequenceDiagram
actor User
participant HomeScreen
participant AnimationBadgeProvider
participant BadgeMessageProvider
participant Badge
User->>HomeScreen: Tap 'Transfer' button
HomeScreen->>AnimationBadgeProvider: handleAnimationTransfer(...)
alt Special animation (Pacman, Chevron, Diamond, Broken Hearts)
AnimationBadgeProvider->>BadgeMessageProvider: transfer[Animation]Animation(...)
BadgeMessageProvider->>Badge: transferData(...)
else Standard animation
AnimationBadgeProvider->>BadgeMessageProvider: checkAndTransfer(...)
BadgeMessageProvider->>Badge: transferData(...)
end
Class diagram for new and updated animation classesclassDiagram
class BadgeAnimation {
<<abstract>>
+processAnimation(badgeHeight, badgeWidth, animationIndex, processGrid, canvas)
}
class PacmanClassicAnimation {
+processAnimation(...)
}
class LeftChevronAnimation {
+processAnimation(...)
}
class DiamondAnimation {
+processAnimation(...)
}
class BrokenHeartsAnimation {
+processAnimation(...)
}
BadgeAnimation <|-- PacmanClassicAnimation
BadgeAnimation <|-- LeftChevronAnimation
BadgeAnimation <|-- DiamondAnimation
BadgeAnimation <|-- BrokenHeartsAnimation
Class diagram for updated AnimationBadgeProvider and transfer logicclassDiagram
class AnimationBadgeProvider {
- int? _animationIndex
- int _animationSpeed
- Timer? _timer
+ bool isSpecialAnimationSelected()
+ void resetToTextAnimation()
+ void calculateDuration(int speed)
+ Future<void> handleAnimationTransfer(...)
}
class BadgeMessageProvider {
+ Future<void> checkAndTransfer(...)
+ Future<void> transferData(...)
+ Future<Data> generateData(...)
}
class SpeedDialProvider
class InlineImageProvider
AnimationBadgeProvider --> BadgeMessageProvider : uses
AnimationBadgeProvider --> SpeedDialProvider : uses
AnimationBadgeProvider --> InlineImageProvider : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nope3472 - I've reviewed your changes - here's some feedback:
- BadgeMessageProvider has grown very large with animation-specific transfer code — consider moving each animation's frame generation into its own class or helper (e.g. reuse BadgeAnimation implementations) to keep the provider focused on transfer logic
- You have duplicated bitmap drawing routines (_drawFilledCircle, _drawPacman, boolToIntBitmap, _drawDestroyEffect) across files — extract these into shared utilities to reduce code duplication and ease maintenance
- The Broken Hearts animation uses index 12 but there’s no modeValueMap or Mode enum entry for 12 — add a mapping (and enum) or otherwise handle index 12 to avoid null lookups during transfer
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- BadgeMessageProvider has grown very large with animation-specific transfer code — consider moving each animation's frame generation into its own class or helper (e.g. reuse BadgeAnimation implementations) to keep the provider focused on transfer logic
- You have duplicated bitmap drawing routines (_drawFilledCircle, _drawPacman, boolToIntBitmap, _drawDestroyEffect) across files — extract these into shared utilities to reduce code duplication and ease maintenance
- The Broken Hearts animation uses index 12 but there’s no modeValueMap or Mode enum entry for 12 — add a mapping (and enum) or otherwise handle index 12 to avoid null lookups during transfer
## Individual Comments
### Comment 1
<location> `lib/providers/badge_message_provider.dart:123` </location>
<code_context>
+ Data data;
+ if (jsonData != null) {
+ data = fileHelper.jsonToData(jsonData);
+ if (isSavedBadge && data.messages.isNotEmpty) {
+ final old = data.messages[0];
+ final newMessage = Message(
+ text: old.text, // use the already-padded hex string
+ flash: old.flash,
+ marquee: old.marquee,
+ speed: old.speed,
+ mode: Mode.animation, // Force seamless marquee
+ );
+ data = Data(messages: [newMessage, ...data.messages.skip(1)]);
</code_context>
<issue_to_address>
Forcing Mode.animation for saved badges may not always be appropriate.
Currently, loading a saved badge always sets the first message's mode to Mode.animation, which may not reflect the user's original choice. Please consider making this behavior conditional based on the saved mode or user intent.
</issue_to_address>
### Comment 2
<location> `lib/providers/badge_message_provider.dart:415` </location>
<code_context>
+ const int badgeHeight = 11;
+ const int badgeWidth = 44;
+ const int spawnInterval = 4; // frames between new diamonds
+ final Speed selectedSpeed = Speed.eight; // Use max speed
+ final logger = Logger();
+ logger.i(
</code_context>
<issue_to_address>
Hardcoding Speed.eight for diamond and broken hearts animations.
Diamond and broken hearts animations override the user-selected speed by always using Speed.eight. Please confirm if this is intentional or if user-configurable speed should be supported here as with other animations.
</issue_to_address>
### Comment 3
<location> `lib/providers/badge_message_provider.dart:593` </location>
<code_context>
+ mouthDirection, badgeWidth, badgeHeight);
+ }
+
+ void _drawFilledCircle(
+ List<List<bool>> canvas, int cx, int cy, int radius, int w, int h) {
+ for (int y = -radius; y <= radius; y++) {
</code_context>
<issue_to_address>
Helper drawing functions are duplicated in animation and provider files.
Consider moving these helper functions to a shared utility to reduce duplication and maintain consistency.
Suggested implementation:
```
import 'package:your_project/utils/drawing_utils.dart';
// (Remove the local _drawFilledCircle implementation.)
// Now use drawFilledCircle from the shared utility wherever _drawFilledCircle was used.
```
1. Create a new file at `lib/utils/drawing_utils.dart` with the following content:
```dart
void drawFilledCircle(
List<List<int>> canvas, int cx, int cy, int radius, int w, int h) {
for (int y = -radius; y <= radius; y++) {
for (int x = -radius; x <= radius; x++) {
if (x * x + y * y <= radius * radius) {
int px = cx + x;
int py = cy + y;
if (py >= 0 && py < h && px >= 0 && px < w) {
canvas[py][px] = 1;
}
}
}
}
}
```
2. Update all usages of `_drawFilledCircle` in this file to use `drawFilledCircle` instead.
3. Replace `'package:your_project/utils/drawing_utils.dart'` with the actual import path for your project.
4. Repeat similar changes in the animation file where the helper is duplicated: remove the local implementation and import/use the shared utility.
</issue_to_address>
### Comment 4
<location> `lib/providers/animation_badge_provider.dart:69` </location>
<code_context>
List<List<bool>> getPaintGrid() => _paintGrid;
+ // Helper: returns true if a special animation (custom) is selected
+ bool isSpecialAnimationSelected() {
+ int idx = getAnimationIndex() ?? 0;
+ return idx == 9 || idx == 10 || idx == 11 || idx == 12;
</code_context>
<issue_to_address>
Special animation detection is hardcoded.
Checking for specific indices (9–12) is not scalable. Consider using tags or an enum to identify special animations for easier future updates.
Suggested implementation:
```
// Helper: returns true if a special animation (custom) is selected
bool isSpecialAnimationSelected() {
final idx = getAnimationIndex() ?? 0;
final animation = animationMap[idx];
return animation?.type == BadgeAnimationType.special;
}
```
```
10: LeftChevronAnimation(type: BadgeAnimationType.special), // Chevron left
11: DiamondAnimation(type: BadgeAnimationType.special), // Diamond
12: BrokenHeartsAnimation(type: BadgeAnimationType.special), // Broken Hearts
};
```
1. Define the `BadgeAnimationType` enum at the top of the file or in a shared location:
```dart
enum BadgeAnimationType { normal, special }
```
2. Update all animation classes (e.g., `LeftChevronAnimation`, `DiamondAnimation`, etc.) to accept a `type` parameter in their constructors and expose a `type` property.
For example:
```dart
class LeftChevronAnimation extends Animation {
final BadgeAnimationType type;
LeftChevronAnimation({this.type = BadgeAnimationType.normal});
// ...
}
```
Do this for all animation classes used in `animationMap`.
3. Update all other entries in `animationMap` to specify their type (default to `BadgeAnimationType.normal` if not special).
</issue_to_address>
### Comment 5
<location> `lib/providers/animation_badge_provider.dart:231` </location>
<code_context>
+ }) async {
+ final int aniIndex = getAnimationIndex() ?? 0;
+ final int selectedSpeed = speedDialProvider.getOuterValue();
+ if (aniIndex == 9) {
+ // Pacman
+ await transferPacmanAnimation(badgeData, selectedSpeed);
+ } else if (aniIndex == 10) {
+ await transferChevronAnimation(badgeData, selectedSpeed);
+ } else if (aniIndex == 11) {
+ await transferDiamondAnimation(badgeData, selectedSpeed);
+ } else if (aniIndex == 12) {
+ await transferBrokenHeartsAnimation(badgeData, selectedSpeed);
+ } else {
</code_context>
<issue_to_address>
handleAnimationTransfer uses index-based dispatch for special animations.
Hardcoding indices for special animations may lead to maintenance issues as new animations are added. Recommend switching to a more flexible dispatch method.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+123
to
+151
| if (isSavedBadge && data.messages.isNotEmpty) { | ||
| final old = data.messages[0]; | ||
| final newMessage = Message( | ||
| text: old.text, // use the already-padded hex string | ||
| flash: old.flash, | ||
| marquee: old.marquee, | ||
| speed: old.speed, | ||
| mode: Mode.animation, // Force seamless marquee |
There was a problem hiding this comment.
question: Forcing Mode.animation for saved badges may not always be appropriate.
Currently, loading a saved badge always sets the first message's mode to Mode.animation, which may not reflect the user's original choice. Please consider making this behavior conditional based on the saved mode or user intent.
| const int badgeHeight = 11; | ||
| const int badgeWidth = 44; | ||
| const int spawnInterval = 4; // frames between new diamonds | ||
| final Speed selectedSpeed = Speed.eight; // Use max speed |
There was a problem hiding this comment.
question: Hardcoding Speed.eight for diamond and broken hearts animations.
Diamond and broken hearts animations override the user-selected speed by always using Speed.eight. Please confirm if this is intentional or if user-configurable speed should be supported here as with other animations.
| return bitmap.map((row) => row.map((b) => b ? 1 : 0).toList()).toList(); | ||
| } | ||
|
|
||
| void _drawFilledCircle( |
There was a problem hiding this comment.
suggestion: Helper drawing functions are duplicated in animation and provider files.
Consider moving these helper functions to a shared utility to reduce duplication and maintain consistency.
Suggested implementation:
import 'package:your_project/utils/drawing_utils.dart';
// (Remove the local _drawFilledCircle implementation.)
// Now use drawFilledCircle from the shared utility wherever _drawFilledCircle was used.
- Create a new file at
lib/utils/drawing_utils.dartwith the following content:
void drawFilledCircle(
List<List<int>> canvas, int cx, int cy, int radius, int w, int h) {
for (int y = -radius; y <= radius; y++) {
for (int x = -radius; x <= radius; x++) {
if (x * x + y * y <= radius * radius) {
int px = cx + x;
int py = cy + y;
if (py >= 0 && py < h && px >= 0 && px < w) {
canvas[py][px] = 1;
}
}
}
}
}-
Update all usages of
_drawFilledCirclein this file to usedrawFilledCircleinstead. -
Replace
'package:your_project/utils/drawing_utils.dart'with the actual import path for your project. -
Repeat similar changes in the animation file where the helper is duplicated: remove the local implementation and import/use the shared utility.
| @@ -54,9 +65,32 @@ class AnimationBadgeProvider extends ChangeNotifier { | |||
| //function to get the state of the cell | |||
| List<List<bool>> getPaintGrid() => _paintGrid; | |||
|
|
|||
| // Helper: returns true if a special animation (custom) is selected | |||
| bool isSpecialAnimationSelected() { | |||
There was a problem hiding this comment.
suggestion: Special animation detection is hardcoded.
Checking for specific indices (9–12) is not scalable. Consider using tags or an enum to identify special animations for easier future updates.
Suggested implementation:
// Helper: returns true if a special animation (custom) is selected
bool isSpecialAnimationSelected() {
final idx = getAnimationIndex() ?? 0;
final animation = animationMap[idx];
return animation?.type == BadgeAnimationType.special;
}
10: LeftChevronAnimation(type: BadgeAnimationType.special), // Chevron left
11: DiamondAnimation(type: BadgeAnimationType.special), // Diamond
12: BrokenHeartsAnimation(type: BadgeAnimationType.special), // Broken Hearts
};
- Define the
BadgeAnimationTypeenum at the top of the file or in a shared location:enum BadgeAnimationType { normal, special }
- Update all animation classes (e.g.,
LeftChevronAnimation,DiamondAnimation, etc.) to accept atypeparameter in their constructors and expose atypeproperty.
For example:Do this for all animation classes used inclass LeftChevronAnimation extends Animation { final BadgeAnimationType type; LeftChevronAnimation({this.type = BadgeAnimationType.normal}); // ... }
animationMap. - Update all other entries in
animationMapto specify their type (default toBadgeAnimationType.normalif not special).
Comment on lines
+231
to
+275
| if (aniIndex == 9) { | ||
| // Pacman | ||
| await transferPacmanAnimation(badgeData, selectedSpeed); | ||
| } else if (aniIndex == 10) { | ||
| await transferChevronAnimation(badgeData, selectedSpeed); | ||
| } else if (aniIndex == 11) { | ||
| await transferDiamondAnimation(badgeData, selectedSpeed); | ||
| } else if (aniIndex == 12) { |
There was a problem hiding this comment.
suggestion: handleAnimationTransfer uses index-based dispatch for special animations.
Hardcoding indices for special animations may lead to maintenance issues as new animations are added. Recommend switching to a more flexible dispatch method.
nope3472
changed the title
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/16860897315/artifacts/3728747677. Screenshots (Android)
Screenshots (iPhone)
Screenshots (iPad)
|
||||||||||||||||||||||||
-1_home_screen.png?raw=true)
-2_text_badge.png?raw=true){kind=icon}
-3_emoji_badge.png?raw=true){kind=icon}
-4_inverted_emoji_badge.png?raw=true){kind=icon}
-5_saved_badges.png?raw=true)
-6_saved_badges_clicked.png?raw=true)
-7_draw_badge.png?raw=true){kind=icon}
|
@nope3472 According to https://github.com/fossasia/badgemagic-firmware/blob/master/BadgeBLE.md, the device only supports 8 types of animations that are already included in BadgeMagic app. |
|
Please implement the following:
|
samruddhi-Rahegaonkar
suggested changes
Jul 25, 2025
There was a problem hiding this comment.
@nope3472 Please Address all the issue given by sourcery they are completely valid and also changes what we have discussed in yesterdays meeting.
if you need any help you can ping me.
|
@nope3472 You could make this as a draft PR so i could give you some early inputs. |
|
@nope3472 it seems there is no progress on this PR since our last discussion |
|
@hpdang will raise a pr today |
samruddhi-Rahegaonkar
suggested changes
Jul 31, 2025
There was a problem hiding this comment.
@nope3472 Please change the tabs name from animation to tansition and vice-versa and any possible changes you see.
please open a new issue for back and forth animation of pac man
samruddhi-Rahegaonkar
suggested changes
Aug 4, 2025
samruddhi-Rahegaonkar
suggested changes
Aug 4, 2025
There was a problem hiding this comment.
@nope3472 Animations should be Saved as Transitions Please change it!
|
@samruddhi-Rahegaonkar these new animations should be under animation and the previous ones should be under transition i think that was discussed |
|
@nope3472 I am saying that The new Animations such as pacman, cupid should saved in savedBadges similar to new Transition |
|
@nope3472 please
|
|
@samruddhi-Rahegaonkar @hpdang @mariobehling i am first creating all the animations and then i will update the save button issue and the tests |
|
@nope3472 If you are done with all the changes needed discussed in meeting if yes then we are good to go with this? |
|
@nope3472 status please |
mariobehling
approved these changes
Aug 7, 2025
added 7 commits
August 9, 2025 20:41
added 7 commits
August 9, 2025 20:42
nope3472
force-pushed
the
moreanimations
branch
from
053bfa5 to
8d1a4a0
Compare
|
@hpdang @mariobehling @samruddhi-Rahegaonkar the tests are resolved please review the pr |
samruddhi-Rahegaonkar
approved these changes
Aug 10, 2025
|
@nope3472 Please resolve conflicts. |
4 tasks
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 #641
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Introduce four new custom badge animations (Pacman, Chevron, Diamond, Broken Hearts) with seamless looping, state reset, and dedicated transfer routines, unify animation selection and transfer logic, update the animation UI to display custom animations with icons in a 3-column grid, and improve text animation handling by auto-switching modes on typing.
New Features:
Bug Fixes:
Enhancements: