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
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. |
|
@mariobehling please approve this |
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