fix: Incorrect animation screen options #1325
Conversation
Reviewer's GuideThis PR enhances the Bluetooth activation and data-transfer flow in BadgeMessageProvider with robust error handling, platform-specific enable routines, and logging, and refactors the SnowflakeAnimation module by consolidating frame calculations and simplifying grid updates for smoother rendering. Class Diagram for Modified BadgeMessageProvider and SnowFlakeAnimationclassDiagram
class BadgeMessageProvider {
+sendMessage(String text, bool flash, bool marq, bool isInverted, String speed, String mode, Map<String, dynamic>? jsonData, bool isSavedBadge) async void
}
class BadgeAnimation {
+processFrame(int animationIndex, List<List<bool>> processGrid, List<List<bool>> canvas) void
}
class SnowFlakeAnimation {
+processFrame(int animationIndex, List<List<bool>> processGrid, List<List<bool>> canvas) void
}
SnowFlakeAnimation --|> BadgeAnimation : extends
BadgeMessageProvider ..> FlutterBluePlus : uses
BadgeMessageProvider ..> ToastUtils : 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 @samruddhi-Rahegaonkar - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/providers/badge_message_provider.dart:111` </location>
<code_context>
+ BluetoothAdapterState adapterState =
+ await FlutterBluePlus.adapterState.first;
+
+ if (adapterState != BluetoothAdapterState.on) {
if (Platform.isAndroid) {
- ToastUtils().showToast('Turning on Bluetooth...');
</code_context>
<issue_to_address>
Consider extracting Bluetooth enable logic into a helper
Extract the repeated Bluetooth enable logic for Android and iOS into a shared helper method to reduce duplication and improve code clarity.
Suggested implementation:
```
if (!await _ensureBluetoothEnabled()) {
return;
}
```
```
Future<bool> _ensureBluetoothEnabled() async {
BluetoothAdapterState adapterState =
await FlutterBluePlus.adapterState.first;
if (adapterState == BluetoothAdapterState.on) {
return true;
}
if (Platform.isAndroid) {
ToastUtils().showToast('Please enable Bluetooth...');
try {
await FlutterBluePlus.turnOn();
return true;
} catch (e) {
ToastUtils().showErrorToast('Failed to enable Bluetooth: $e');
logger.e('Bluetooth turnOn() failed: $e');
return false;
}
}
// Add iOS logic here if needed in the future
return false;
}
```
</issue_to_address>
### Comment 2
<location> `lib/providers/badge_message_provider.dart:126` </location>
<code_context>
+ adapterState = await FlutterBluePlus.adapterState
+ .where((state) => state == BluetoothAdapterState.on)
+ .first
+ .timeout(
+ const Duration(seconds: 10),
+ onTimeout: () {
</code_context>
<issue_to_address>
Extract timeout duration to a constant
Defining a named constant improves clarity and makes future adjustments easier.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
adapterState = await FlutterBluePlus.adapterState
.where((state) => state == BluetoothAdapterState.on)
.first
.timeout(
const Duration(seconds: 10),
onTimeout: () {
ToastUtils().showErrorToast('Bluetooth did not turn on in time.');
throw Exception('Bluetooth enable timeout');
},
);
=======
const Duration _bluetoothEnableTimeout = Duration(seconds: 10);
adapterState = await FlutterBluePlus.adapterState
.where((state) => state == BluetoothAdapterState.on)
.first
.timeout(
_bluetoothEnableTimeout,
onTimeout: () {
ToastUtils().showErrorToast('Bluetooth did not turn on in time.');
throw Exception('Bluetooth enable timeout');
},
);
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `lib/badge_animation/ani_snowflake.dart:12` </location>
<code_context>
- for (int i = 0; i < badgeHeight; i++) {
- for (int j = 0; j < badgeWidth; j++) {
- // Calculate the total number of frames that fit the badge width
- int framesCount = (newGridWidth / badgeWidth).ceil();
-
- // Determine the current frame based on the animation value
</code_context>
<issue_to_address>
Guard against zero framesCount to avoid divide/modulo by zero
Clamping framesCount to at least 1 ensures animationIndex % framesCount won't cause a crash when newGridWidth is 0.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| BluetoothAdapterState adapterState = | ||
| await FlutterBluePlus.adapterState.first; | ||
|
|
||
| if (adapterState != BluetoothAdapterState.on) { |
There was a problem hiding this comment.
suggestion: Consider extracting Bluetooth enable logic into a helper
Extract the repeated Bluetooth enable logic for Android and iOS into a shared helper method to reduce duplication and improve code clarity.
Suggested implementation:
if (!await _ensureBluetoothEnabled()) {
return;
}
Future<bool> _ensureBluetoothEnabled() async {
BluetoothAdapterState adapterState =
await FlutterBluePlus.adapterState.first;
if (adapterState == BluetoothAdapterState.on) {
return true;
}
if (Platform.isAndroid) {
ToastUtils().showToast('Please enable Bluetooth...');
try {
await FlutterBluePlus.turnOn();
return true;
} catch (e) {
ToastUtils().showErrorToast('Failed to enable Bluetooth: $e');
logger.e('Bluetooth turnOn() failed: $e');
return false;
}
}
// Add iOS logic here if needed in the future
return false;
}
| adapterState = await FlutterBluePlus.adapterState | ||
| .where((state) => state == BluetoothAdapterState.on) | ||
| .first | ||
| .timeout( | ||
| const Duration(seconds: 10), | ||
| onTimeout: () { | ||
| ToastUtils().showErrorToast('Bluetooth did not turn on in time.'); | ||
| throw Exception('Bluetooth enable timeout'); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
suggestion: Extract timeout duration to a constant
Defining a named constant improves clarity and makes future adjustments easier.
| adapterState = await FlutterBluePlus.adapterState | |
| .where((state) => state == BluetoothAdapterState.on) | |
| .first | |
| .timeout( | |
| const Duration(seconds: 10), | |
| onTimeout: () { | |
| ToastUtils().showErrorToast('Bluetooth did not turn on in time.'); | |
| throw Exception('Bluetooth enable timeout'); | |
| }, | |
| ); | |
| const Duration _bluetoothEnableTimeout = Duration(seconds: 10); | |
| adapterState = await FlutterBluePlus.adapterState | |
| .where((state) => state == BluetoothAdapterState.on) | |
| .first | |
| .timeout( | |
| _bluetoothEnableTimeout, | |
| onTimeout: () { | |
| ToastUtils().showErrorToast('Bluetooth did not turn on in time.'); | |
| throw Exception('Bluetooth enable timeout'); | |
| }, | |
| ); |
| // Calculate the starting column for the current frame in newGrid | ||
| int startCol = currentcountFrame * badgeWidth; | ||
| // Calculate the total number of frames based on overflow | ||
| int framesCount = (newGridWidth / badgeWidth).ceil(); | ||
|
|
There was a problem hiding this comment.
issue: Guard against zero framesCount to avoid divide/modulo by zero
Clamping framesCount to at least 1 ensures animationIndex % framesCount won't cause a crash when newGridWidth is 0.
samruddhi-Rahegaonkar
force-pushed
the
issue#1248
branch
from
4951560 to
90a491f
Compare
samruddhi-Rahegaonkar
changed the title
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/16652499339/artifacts/3659326012. 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}
|
@adityastic @Jhalakupadhyay Please review this too. |
|
@nope3472 Will you please review it i will add recording of output? |
samruddhi-Rahegaonkar
force-pushed
the
issue#1248
branch
2 times, most recently
from
7f25deb to
bddc6f8
Compare
|
@samruddhi-Rahegaonkar what is the status here? |
|
@hpdang This feature has been completed from my side also added recordings regarding it and updated the codes according to the review. |
samruddhi-Rahegaonkar
force-pushed
the
issue#1248
branch
from
b8cdc76 to
5f629b0
Compare
|
@nope3472 Review the Latest changes! |
|
@samruddhi-Rahegaonkar the Animation effect works as expected on the physical LED badge. However, the in-app preview (what users see on their phone screen before sending it to the badge) does not accurately reflect the actual animation that plays on the badge. This is confusing for users, because they expect the preview to show exactly what will appear on the badge. Please test it again and fix it. |
|
@hpdang we could only show in app preview if user set three messages in textfield and saved as animation. if we saved each word as individual animation then in saved badge preview it will show still image because we cannot create animation with one word. |
|
@hpdang in the Homescreen preview they see what animation looks like. |
|
@samruddhi-Rahegaonkar users should be able to understand how their text will appear on the badge without having to save anything first. If the user’s input is just a single word that fits within one screen, the animation remains fixed on that screen, and the preview should reflect this behavior. In short: the in-app preview must always accurately represent what will actually happen on the badge, regardless of the input length or animation type. Maybe you can try:
|
mariobehling
changed the title
fix: Guard against zero framesCount to avoid modulo by zero in SnowFlakeAnimation fix: blicking illusion back fix: added input validation fix: fixed the interchanges animations fix: animation and picture swapped with correct demo animation style: format animation_badge_provider fix: fixed animation preview and usage fix: shows selected animation in preview of saved badges fix: animation transfers to the badge succesfully as animation fix: test fail fix: failed flutter test fixed fix: sync the actual badge and preview
samruddhi-Rahegaonkar
force-pushed
the
issue#1248
branch
from
2e4f731 to
d5eb5fc
Compare
|
@samruddhi-Rahegaonkar the preview doesnt not correctly reflect what is on the physical badge. video6325424209149302949.mp4video6325424209149302947.mp4 |
|
@samruddhi-Rahegaonkar what is the status here? |
|
@hpdang The badge and preview use completely separate rendering systems, and unless both are driven by the exact same data stream and logic, 100% visual sync in full-width word-switching is not feasible. |
|
@nope3472 I have done with this feature. Please review and if there are any changes suggest the changes or please aprove it. so i can move to another issue. |
There was a problem hiding this comment.
-There is repeated logic for mapping animation indices to modes and vice versa across multiple files (providers/animation_badge_provider.dart, providers/badge_message_provider.dart, providers/saved_badge_provider.dart).
-Suggestion: Create a central utility/helper for animation/mode/index mapping. This makes future changes less error-prone and the codebase easier to maintain.
APART FROM THIS THE LOGIC IS TECHNICALLY CORRECT.
|
@nope3472 I think all have there seperate working functionality making single utility will be hard to manage isn't it ? AnimationBadgeProvider handles grid updates, animation rendering, timer lifecycle. |
|
@nope3472 Please Aprove the changes as discussed. |
Fixes #1248
Fixes #1370
Changes
-Updated Animation according to proprietary app.
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Improve Bluetooth handling in
BadgeMessageProviderby automating enablement on Android, adding platform-specific prompts, timeouts, and error logging, and streamline the data transfer flow; refactorSnowFlakeAnimationto simplify frame computation and prevent rendering glitches by aligning animation slices and bounding checks.Enhancements:
BadgeMessageProviderto unify adapter state checks and streamline data generation and transfer