feat: added emoji rendering #1200 #1213
Open
Conversation
This file contains 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 Guide by SourceryThis pull request introduces enhanced text and emoji rendering capabilities to the badge, including font selection, a marquee effect, and improved clipping prevention. It also refactors the rendering pipeline to support these new features. Sequence diagram for message rendering with text stylesequenceDiagram
participant User
participant HomeScreen
participant AnimationBadgeProvider
participant Converters
participant BadgePaint
User->>HomeScreen: Types message and selects font
HomeScreen->>AnimationBadgeProvider: badgeAnimation(message, isInverted, textStyle)
AnimationBadgeProvider->>Converters: messageTohex(message, isInverted, textStyle)
Converters->>Converters: renderTextToMatrix(message, textStyle)
Converters-->>AnimationBadgeProvider: hexStrings
AnimationBadgeProvider->>AnimationBadgeProvider: setNewGrid(binaryArray)
AnimationBadgeProvider->>BadgePaint: getPaintGrid()
BadgePaint->>BadgePaint: paint(canvas, size)
File-Level Changes
Assessment against 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:
Overall Comments:
- Consider extracting the font selection logic into a separate widget for better organization.
- The animation provider is initialized twice, once as a field and again in initState; prefer initializing it only once.
Here's what I looked at during the review
- 🟡 General issues: 4 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 and I'll use the feedback to improve your reviews.
| @@ -92,17 +124,23 @@ class _HomeScreenState extends State<HomeScreen> | |||
| } else { | |||
| previousText = currentText; | |||
| } | |||
| _controllerListner(); | |||
| } | |||
|
|
|||
| void _controllerListner() { | |||
There was a problem hiding this comment.
nitpick (typo): Consider renaming '_controllerListner' to '_controllerListener'.
This minor naming adjustment would improve code readability and reduce potential confusion.
Suggested implementation:
_controllerListener();
void _controllerListener() {
Comment on lines
+181
to
+184
| final ui.Picture picture = recorder.endRecording(); | ||
| final ui.Image image = await picture.toImage(canvasWidth, canvasHeight); | ||
| final ByteData? byteData = | ||
| await image.toByteData(format: ui.ImageByteFormat.rawRgba); |
There was a problem hiding this comment.
suggestion: Consider handling the null case from toByteData more gracefully.
Instead of throwing an exception when byteData is null, you might want to consider a fallback mechanism or clearer logging for better error tracing.
Suggested change
| final ui.Picture picture = recorder.endRecording(); | |
| final ui.Image image = await picture.toImage(canvasWidth, canvasHeight); | |
| final ByteData? byteData = | |
| await image.toByteData(format: ui.ImageByteFormat.rawRgba); | |
| final ui.Picture picture = recorder.endRecording(); | |
| final ui.Image image = await picture.toImage(canvasWidth, canvasHeight); | |
| final ByteData? rawByteData = | |
| await image.toByteData(format: ui.ImageByteFormat.rawRgba); | |
| final ByteData byteData = rawByteData ?? (() { | |
| debugPrint("Warning: image.toByteData returned null. Using fallback empty byte data."); | |
| final Uint8List fallback = Uint8List(canvasWidth * canvasHeight * 4); | |
| return ByteData.sublistView(fallback); | |
| })(); |
|
|
||
| // Draw the outer rectangle | ||
| double horizontalPadding = 8.0; | ||
| badgeWidth -= 1.0 * horizontalPadding; | ||
| final Paint rectPaint = Paint() | ||
| ..style = PaintingStyle.fill | ||
| ..color = Colors.black | ||
| ..strokeWidth = 2.0; | ||
|
|
||
| final RRect gridRect = RRect.fromLTRBR( |
There was a problem hiding this comment.
suggestion: Extract layout magic numbers into named constants.
The expressions like 'offsetWidthBadgeBackground + horizontalPadding - 10' are hard to follow. Abstracting these values as well-named constants can improve maintainability and readability.
Suggested implementation:
double horizontalPadding = 8.0;
const double leftOffsetAdjustment = 10.0;
const double badgeCornerRadius = 10.0;
final RRect gridRect = RRect.fromLTRBR(
offsetWidthBadgeBackground + horizontalPadding - leftOffsetAdjustment,
offsetHeightBadgeBackground,
offsetWidthBadgeBackground + badgeWidth + horizontalPadding,
offsetHeightBadgeBackground + badgeHeight,
Radius.circular(badgeCornerRadius),
);
Ensure that if the extracted constants are used elsewhere in the file, they are consistently named and placed in a suitable location for shared layout constants.
| String message, Converters converters, bool isInverted) async { | ||
| if (message == "") { | ||
| //geerate a 2d list with all values as 0 | ||
| void badgeAnimation(String message, Converters converters, bool isInverted, |
There was a problem hiding this comment.
suggestion: Review the default TextStyle fallback in badgeAnimation.
The current fallback uses 'TextStyle(fontSize: 11, color: Colors.black)'. Ensure that this default aligns with overall design expectations or consider defining it as a class-level constant.
Suggested implementation:
class AnimationBadgeProvider extends ChangeNotifier {
static const TextStyle defaultTextStyle = TextStyle(fontSize: 11, color: Colors.black);
void badgeAnimation(String message, Converters converters, bool isInverted,
{TextStyle? textStyle}
) async {
if (message.isEmpty) {
List<List<bool>> image =
List.generate(16, (i) => List.generate(45, (j) => false));
setNewGrid(image);
} else {
final effectiveTextStyle = textStyle ?? defaultTextStyle;
Please ensure that the rest of the badgeAnimation function uses effectiveTextStyle where a TextStyle is needed.
nope3472
changed the title
|
Build successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/13549723972/artifacts/2657698104 |
adityastic
reviewed
Feb 27, 2025
| List<List<bool>> _paintGrid = | ||
| List.generate(11, (i) => List.generate(44, (j) => false)); | ||
| List.generate(16, (i) => List.generate(45, (j) => false)); |
There was a problem hiding this comment.
Why are we changing the badge size?
There was a problem hiding this comment.
The hardware badge is 11x44
adityastic
reviewed
Feb 27, 2025
| @@ -4,7 +4,10 @@ import 'package:flutter/material.dart'; | |||
| import 'package:provider/provider.dart'; | |||
|
|
|||
| class AnimationBadge extends StatefulWidget { | |||
| const AnimationBadge({super.key}); | |||
| final String text; | |||
There was a problem hiding this comment.
Animation badge should have no relation to the text. It just displays whatever binary data is passed on
adityastic
reviewed
Feb 27, 2025
| import 'package:badgemagic/bademagic_module/utils/badge_utils.dart'; | ||
| import 'package:flutter/material.dart'; | ||
|
|
||
| class BadgePaint extends CustomPainter { | ||
| BadgeUtils badgeUtils = BadgeUtils(); | ||
| final List<List<bool>> grid; | ||
| final TextStyle? textStyle; |
There was a problem hiding this comment.
There should be no changes to animation and badge view
adityastic
requested changes
Feb 27, 2025
adityastic
force-pushed
the
flutter_app
branch
2 times, most recently
from
d14b7e1 to
381b8c8
Compare
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 #1200
Changes
Checklist:
flutter analyzeand tests run withflutter test.Summary by Sourcery
Improves the badge's text rendering capabilities by adding font selection and marquee effect, and fixes clipping issues with emojis and text.
New Features:
Bug Fixes:
Enhancements:
Summary by Sourcery
Improves the badge's text rendering capabilities by adding font selection and marquee effect, and fixes clipping issues with emojis and text.
New Features:
Bug Fixes:
Enhancements: