feat: Added the right scroll animation #1000
Conversation
…badge. feat: Added the modes support for left right top down fixed and animation support for flash and marquee. animation right
Reviewer's Guide by SourceryThis pull request implements a right scroll animation for a badge display, along with several utility functions for converting between different data representations. It also refactors the badge view and animation logic, introducing a new provider for managing the badge state and animations. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Jhalakupadhyay - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider breaking down the BadgeViewProvider into smaller, more focused classes to improve maintainability. The current implementation is handling too many responsibilities (animation logic, timer management, state updates).
- Implement the remaining animation types (Up, Down, Fixed, Snowflake) before considering this feature complete. The TODO comments in the code indicate these are still missing.
Here's what I looked at during the review
- 🟡 General issues: 7 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| return binaryString; | ||
| } | ||
|
|
||
| List<List<int>> binaryStringTo2DList(String binaryString) { |
There was a problem hiding this comment.
suggestion: Consider refactoring binaryStringTo2DList for improved readability and efficiency
The current implementation uses nested loops with single-letter variables, which can be hard to follow. Consider using more descriptive variable names and simplifying the logic. You might also want to add error handling for cases where the input string length doesn't match the expected size.
List<List<int>> binaryStringTo2DList(String binaryString) {
const int expectedLength = 11 * 7;
if (binaryString.length != expectedLength) {
throw ArgumentError('Invalid binary string length. Expected $expectedLength, got ${binaryString.length}');
}
final int maxHeight = 11;
final List<List<int>> binary2DList = List.generate(maxHeight, (_) => []);
| return binaryArray; | ||
| } | ||
|
|
||
| String hexToBin(String hex) { |
There was a problem hiding this comment.
suggestion (performance): Optimize hexToBin function for better performance
The current implementation using BigInt might be inefficient for large inputs. Consider implementing a direct hex to binary conversion using bitwise operations or a lookup table for better performance.
String hexToBin(String hex) {
final lookup = {
'0': '0000', '1': '0001', '2': '0010', '3': '0011',
'4': '0100', '5': '0101', '6': '0110', '7': '0111',
'8': '1000', '9': '1001', 'a': '1010', 'b': '1011',
'c': '1100', 'd': '1101', 'e': '1110', 'f': '1111'
};
return hex.toLowerCase().split('').map((c) => lookup[c]!).join();
}
| @@ -43,6 +43,22 @@ class Converters { | |||
| return hexStrings; | |||
| } | |||
|
|
|||
| void badgeAnimation(String message) async { | |||
There was a problem hiding this comment.
suggestion: Separate concerns in badgeAnimation function
This function is mixing data processing with UI updates. Consider splitting it into two functions: one for data conversion and another for updating the UI. This will improve maintainability and adhere better to the principle of separation of concerns.
Future<List<List<int>>> convertMessageToMatrix(String message) async {
// Implementation for converting message to matrix
}
void updateBadgeUI(List<List<int>> matrix) async {
// Implementation for updating the UI with the matrix
}
void badgeAnimation(String message) async {
final matrix = await convertMessageToMatrix(message);
updateBadgeUI(matrix);
}
| _startImageCaching(); | ||
| super.initState(); | ||
|
|
||
| _tabController = TabController(length: 3, vsync: this); | ||
| } | ||
|
|
||
| void _controllerListner() { |
There was a problem hiding this comment.
suggestion (typo): Fix typo in function name and consider breaking down if complexity grows
The function name has a typo - it should be '_controllerListener'. Also, if this function grows in complexity, consider breaking it down into smaller, more focused functions.
void _controllerListener() {
| const Duration aniFlashSpeed = Duration(microseconds: 500000); // in uS | ||
|
|
||
| // Function to calculate animation speed based on speed level | ||
| int aniSpeedStrategy(int speedLevel) { |
There was a problem hiding this comment.
suggestion: Consider using an enum for speed levels
Instead of using raw integers for speed levels, consider defining an enum. This would make the code more type-safe and self-documenting.
enum SpeedLevel { slow, medium, fast, veryFast }
int aniSpeedStrategy(SpeedLevel level) {
final int speedFactor = level.index;
return aniBaseSpeed.inMicroseconds - (speedFactor * aniBaseSpeed.inMicroseconds ~/ 8);
}
| } | ||
| } | ||
|
|
||
| void changeGridValue(List<List<int>> newGrid) { |
There was a problem hiding this comment.
suggestion: Break down changeGridValue function into smaller, more manageable functions
This function is quite long and complex. Consider breaking it down into smaller, more focused functions. This will improve readability and make the code easier to maintain and test.
void changeGridValue(List<List<int>> newGrid) {
_validateGridDimensions(newGrid);
_updateGrid(newGrid);
_notifyListeners();
}
void _validateGridDimensions(List<List<int>> grid) {
int expectedWidth = homeViewGrid[0].length;
int expectedHeight = homeViewGrid.length;
// Add validation logic here
}
void _updateGrid(List<List<int>> newGrid) {
// Add grid update logic here
}
| @@ -0,0 +1,33 @@ | |||
| import 'package:badgemagic/badge_animation/animation_abstract.dart'; | |||
|
|
|||
| class LeftAnimation extends BadgeAnimation { | |||
There was a problem hiding this comment.
suggestion: Consider unifying animation logic to reduce code duplication
The LeftAnimation and RightAnimation classes have very similar logic. Consider creating a base class or a utility function that both can use, parameterizing the differences. This would reduce code duplication and make it easier to maintain and extend the animation system.
abstract class DirectionalAnimation extends BadgeAnimation {
final bool isLeftDirection;
DirectionalAnimation({required this.isLeftDirection});
// Common animation logic goes here
}
class LeftAnimation extends DirectionalAnimation {
LeftAnimation() : super(isLeftDirection: true);
Summary by Sourcery
Add new badge animation capabilities with left and right scrolling animations, refactor the badge drawing logic to use a new provider, and introduce utility functions for data conversion. Implement a dynamic animation speed strategy and replace the existing provider with a new one for better architecture.
New Features:
Enhancements:
BadgeViewProvider, improving the separation of concerns and maintainability.Chores:
DrawBadgeProviderwithBadgeViewProvideracross the codebase, aligning with the new architecture for badge animations.