fix: made the scrolling feature in snowflake effect #1502
Conversation
Reviewer's GuideEnhanced SnowFlakeAnimation to support horizontal scrolling by partitioning the input grid into bounded frame cycles with clamp-based offsets and boundary checks; also updated the Hindi translation for the “animation” label. Sequence diagram for horizontal scrolling in SnowFlakeAnimationsequenceDiagram
participant "processAnimation()"
participant "processGrid"
participant "canvas"
"processAnimation()"->>"processGrid": Partition grid into frames
"processAnimation()"->>"canvas": Apply frame data with horizontal offset and boundary checks
"processAnimation()"->>"canvas": Update canvas for each animation phase
Class diagram for updated SnowFlakeAnimation scrolling logicclassDiagram
class BadgeAnimation
class SnowFlakeAnimation {
+void processAnimation(int badgeHeight, int badgeWidth, int animationIndex, List<List<bool>> processGrid, List<List<bool>> canvas)
-int newWidth
-int newHeight
-int framesCount
-int snowflakeCycleLength
-int maxFrames
-int effectiveFramesCount
-int totalCycleLength
-int cyclePosition
-int currentFrame
-int startCol
-int frame
-int horizontalOffset
}
BadgeAnimation <|-- SnowFlakeAnimation
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 there - I've reviewed your changes - here's some feedback:
- The cycle and frame‐position calculations in processAnimation are quite dense—consider refactoring into smaller helper methods or better‐named variables for clarity.
- Verify that clamping horizontalOffset and effectiveFramesCount covers edge cases (e.g. when newWidth > badgeWidth) to avoid unexpected scroll offsets.
- Changing the ‘animation’ translation to 'डॉट-मैट्रिक्स' shifts the meaning—ensure this matches the UI context or introduce a new L10n key instead of repurposing 'animation'.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The cycle and frame‐position calculations in processAnimation are quite dense—consider refactoring into smaller helper methods or better‐named variables for clarity.
- Verify that clamping horizontalOffset and effectiveFramesCount covers edge cases (e.g. when newWidth > badgeWidth) to avoid unexpected scroll offsets.
- Changing the ‘animation’ translation to 'डॉट-मैट्रिक्स' shifts the meaning—ensure this matches the UI context or introduce a new L10n key instead of repurposing 'animation'.
## Individual Comments
### Comment 1
<location> `lib/badge_animation/ani_snowflake.dart:10-11` </location>
<code_context>
- int horizontalOffset = (badgeWidth - newWidth) ~/ 2;
+ // Calculate the total number of frames that fit the badge width
+ int framesCount = (newWidth / badgeWidth).ceil();
+
+ // Calculate the total animation length for one complete snowflake cycle
</code_context>
<issue_to_address>
**suggestion:** Consider using integer division for frame count calculation.
Floating-point division with ceil can cause off-by-one errors when newWidth isn't a multiple of badgeWidth. Integer division with rounding up, such as (newWidth + badgeWidth - 1) ~/ badgeWidth, is clearer and avoids this issue.
```suggestion
// Calculate the total number of frames that fit the badge width using integer division rounding up
int framesCount = (newWidth + badgeWidth - 1) ~/ badgeWidth;
```
</issue_to_address>
### Comment 2
<location> `lib/badge_animation/ani_snowflake.dart:35` </location>
<code_context>
+ // Get the frame within the current snowflake cycle
+ int frame = cyclePosition % snowflakeCycleLength;
+
+ int horizontalOffset = (badgeWidth - newWidth).clamp(0, badgeWidth) ~/ 2;
bool phase1 = frame < badgeHeight * 4;
</code_context>
<issue_to_address>
**question:** Clamping horizontalOffset may mask layout issues.
Clamping prevents negative offsets but may conceal cases where newWidth exceeds badgeWidth, which could cause rendering issues. Please handle these cases explicitly or clarify the intended behavior in documentation.
</issue_to_address>
### Comment 3
<location> `lib/badge_animation/ani_snowflake.dart:51` </location>
<code_context>
+ int sourceCol = startCol + col - horizontalOffset;
bool isWithinNewGrid = sourceCol >= 0 && sourceCol < newWidth;
- if (isWithinNewGrid) {
+ if (isWithinNewGrid && row < newHeight) {
canvas[fallPosition][col] = processGrid[row][sourceCol];
}
</code_context>
<issue_to_address>
**nitpick:** Redundant row bounds check in inner loop.
If badgeHeight can exceed newHeight, validate badgeHeight against newHeight earlier to avoid redundant checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nope3472
changed the title
Build StatusBuild workflow failed. Please check the logs for more information. ScreenshotsNot able to fetch screenshots. |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Hi, I’ve reviewed this PR with the help of AI to speed up code analysis and ensure we catch technical issues early — please use AI tools as well (e.g., ChatGPT with code input or GitHub Copilot Chat) to check your code and reasoning next time. It really helps to find small mistakes faster and improves the review cycle.
⚠️ Must Fix Before Merge
1. Duplicate variable declarations
In ani_snowflake.dart, there are duplicate local variable declarations:
int frame = animationIndex % totalAnimationLength;
int horizontalOffset = (badgeWidth - newWidth) ~/ 2;
...
int frame = cyclePosition % snowflakeCycleLength;
int horizontalOffset = (badgeWidth - newWidth).clamp(0, badgeWidth) ~/ 2;➡️ Fix: Remove the int keyword from the second declarations and just reuse the same variables:
frame = cyclePosition % snowflakeCycleLength;
horizontalOffset = (badgeWidth - newWidth).clamp(0, badgeWidth) ~/ 2;Otherwise, this will not compile or could lead to unexpected shadowing.
2. Inefficient bounds checks and nested loops
The nested for loops use repetitive boundary checks (if (row < newHeight && col < newWidth) inside the inner loop).
➡️ Fix suggestion:
Clamp dimensions before looping:
final height = min(badgeHeight, newHeight);
final width = min(badgeWidth, newWidth);
for (int row = 0; row < height; row++) {
for (int col = 0; col < width; col++) {
...
}
}This makes the loop more efficient and easier to read.
3. Horizontal offset logic may be hiding layout issues
You clamp horizontalOffset to [0, badgeWidth].
If newWidth > badgeWidth, this hides the fact that the image is wider and should scroll.
➡️ Suggestion: handle it explicitly:
if (newWidth > badgeWidth) {
horizontalOffset = 0; // scrolling case
} else {
horizontalOffset = (badgeWidth - newWidth) ~/ 2; // centering case
}Add a short comment to document why clamping is not needed here.
4. Off-by-one risk in frame count
int framesCount = (newWidth / badgeWidth).ceil();
For safety and to stay in integer math, use:
int framesCount = (newWidth + badgeWidth - 1) ~/ badgeWidth;This avoids rounding inconsistencies for narrow widths.
5. Clarify variables and add inline comments
Variables like totalCycleLength, cyclePosition, framesCount, and startCol interact in non-trivial ways.
Please add a short explanatory block at the top of the method describing:
- how these values are derived,
- how the cycle length is determined,
- and why
maxFrames = 8is chosen.
Example:
// The snowflake animation scrolls horizontally across the display.
// totalCycleLength: number of frames per scroll cycle
// cyclePosition: current offset within the cycle
// maxFrames: upper bound to limit animation memory footprint🧩 Internationalization
6. Manual edits in generated localization file
lib/l10n/app_localizations_hi.dart appears manually changed.
That file is auto-generated from .arb sources and will be overwritten.
➡️ Fix:
Revert manual edits, keep changes in .arb files only, then run:
flutter gen-l10n
to regenerate localized Dart files.
7. Inconsistent label rename
The key animation now maps to "Dot-Matrix" in English and Hindi, but not in other locales.
If it’s a global rename, update all .arb files.
If it’s local to one screen, use a new key (e.g. "dotMatrixLabel") instead of overwriting "animation" globally.
🧪 Minimal Test Plan
-
Short and long text preview
- Test texts that produce
newWidthsmaller, equal, and greater thanbadgeWidth. - Verify correct centering and horizontal scroll movement without clipping.
- Test texts that produce
-
Regression
- Check that other effects (Laser, Picture, etc.) behave as before.
-
Localization
- Switch to Hindi and English; verify label shows as “Dot-Matrix”.
- Re-run
flutter gen-l10nand confirm no warnings.
-
Badge parity
- Send the same text to a physical badge and compare scroll speed and smoothness.
|
Looks Good to me! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the scrolling feature in the snowflake animation effect by implementing proper horizontal scrolling with frame cycling and adding bounds checks to prevent grid access errors.
Key changes include:
- Enhanced snowflake animation with dynamic horizontal scrolling and frame management
- Added bounds checking to prevent out-of-bounds grid access errors
- Updated Hindi localization to change 'animation' translation from 'एनिमेशन' to 'डॉट-मैट्रिक्स'
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/badge_animation/ani_snowflake.dart | Implements scrolling logic with frame cycling, dynamic offsets, and bounds checking |
| lib/l10n/app_localizations_hi.dart | Updates Hindi translation for 'animation' |
| lib/l10n/app_hi.arb | Updates Hindi localization resource for 'animation' |
| lib/l10n/app_en.arb | Updates English localization from 'Animation' to 'Dot-Matrix' |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "down": "Down", | ||
| "fixed": "Fixed", | ||
| "animation": "Animation", | ||
| "animation": "Dot-Matrix", |
There was a problem hiding this comment.
Duplicate key 'animation' found in the localization file. This key appears at lines 65, 134, and 161. In JSON/ARB files, duplicate keys can cause unpredictable behavior as only the last occurrence will be used. Each key should be unique or the keys should be renamed to be more specific (e.g., 'animationMode', 'animationType', 'animationLabel').
| "save": "सेव करें", | ||
| "speed": "स्पीड", | ||
| "animation": "एनिमेशन", | ||
| "animation": "डॉट-मैट्रिक्स", |
There was a problem hiding this comment.
Duplicate key 'animation' found in the localization file. This key appears at lines 58, 123, and 150. In JSON/ARB files, duplicate keys can cause unpredictable behavior as only the last occurrence will be used. Each key should be unique or the keys should be renamed to be more specific (e.g., 'animationMode', 'animationType', 'animationLabel').
| // Get the current position in the overall cycle | ||
| int cyclePosition = animationIndex % totalCycleLength; | ||
|
|
||
| // Determine which text section we're currently showing |
There was a problem hiding this comment.
The comment mentions 'text section' but this is a snowflake animation. The comment should be updated to reflect the actual purpose, such as 'Determine which content frame we're currently showing'.
| // Determine which text section we're currently showing | |
| // Determine which content frame we're currently showing |
3 participants
Fixes #1470
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Refine the snowflake animation to include dynamic horizontal scrolling with frame cycling and optimized frame limits, add bounds checks to avoid grid errors, and update the Hindi localization for 'animation'.
Bug Fixes:
Enhancements:
Documentation: