feat: adding different font styles #1351
Conversation
Reviewer's GuideThis PR integrates a Google Fonts–based font picker into the HomeScreen text field for real-time style updates, extends badge editing by loading and updating saved badge files, and strengthens file I/O and data persistence through synchronization and utility classes. Sequence diagram for editing a saved badge with font and effect restorationsequenceDiagram
actor User
participant HomeScreen
participant FileHelper
participant BadgeTextStorage
participant AnimationBadgeProvider
participant SpeedDialProvider
User->>HomeScreen: Open HomeScreen with savedBadgeFilename
HomeScreen->>FileHelper: Load badge JSON from disk
FileHelper-->>HomeScreen: Return badge data
HomeScreen->>BadgeTextStorage: Get original text for badge
BadgeTextStorage-->>HomeScreen: Return original text
HomeScreen->>AnimationBadgeProvider: Set effects and animation mode
HomeScreen->>SpeedDialProvider: Set speed dial value
HomeScreen->>HomeScreen: Set text field to original text
User->>HomeScreen: Edit and save badge
HomeScreen->>SavedBadgeProvider: updateBadgeData(...)
SavedBadgeProvider->>FileHelper: Save updated badge data
SavedBadgeProvider->>BadgeTextStorage: Save updated original text
FileHelper-->>SavedBadgeProvider: Confirm save
BadgeTextStorage-->>SavedBadgeProvider: Confirm save
SavedBadgeProvider-->>HomeScreen: Confirm update
HomeScreen-->>User: Show "Badge Updated Successfully"
Class diagram for new and updated types: FontPickerDialog, BadgeTextStorage, SavedBadgeProviderclassDiagram
class FontPickerDialog {
+String? selectedFont
+ValueChanged<String?> onFontSelected
+build(BuildContext context)
}
class BadgeTextStorage {
<<static>>
+saveOriginalText(String badgeFilename, String originalText)
+getOriginalText(String badgeFilename)
+deleteOriginalText(String badgeFilename)
}
class SavedBadgeProvider {
+applySavedBadgeDataToUI(...)
+updateBadgeData(...)
+getBadgeData(...)
+savedBadgeAnimation(...)
+isSavedBadgeData: bool
+setSavedBadgeDataMap(...)
+setIsSavedBadgeData(...)
}
FontPickerDialog --|> StatelessWidget
SavedBadgeProvider --|> ChangeNotifier
File-Level Changes
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 @nope3472 - 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/view/homescreen.dart:540` </location>
<code_context>
+ if (state == AppLifecycleState.resumed) {
+ inlineimagecontroller.clear();
+ previousText = '';
+ animationProvider.stopAllAnimations.call(); // If method exists
+ animationProvider.initializeAnimation.call(); // If method exists
+ if (mounted) setState(() {});
+ } else if (state == AppLifecycleState.paused ||
</code_context>
<issue_to_address>
Calling methods with .call() may cause runtime errors if not callable.
Using `.call()` here will throw if these are standard methods. Invoke them directly as methods instead.
</issue_to_address>
### Comment 2
<location> `lib/view/homescreen.dart:270` </location>
<code_context>
+ onChanged: (value) {},
+ controller: inlineimagecontroller,
+ specialTextSpanBuilder: ImageBuilder(),
+ style: _selectedFontFamily != null
+ ? GoogleFonts.getFont(_selectedFontFamily!)
+ : null,
+ decoration: InputDecoration(
+ border: OutlineInputBorder(
</code_context>
<issue_to_address>
No fallback for invalid or unavailable font family.
Validate _selectedFontFamily or add a fallback to avoid exceptions if the font is unsupported.
</issue_to_address>
### Comment 3
<location> `lib/bademagic_module/utils/file_helper.dart:203` </location>
<code_context>
+ /// Save the original text for a badge
+ static Future<void> saveOriginalText(
+ String badgeFilename, String originalText) async {
+ try {
+ // Get the existing text storage or create a new one
+ Map<String, String> textStorage = await _getTextStorage();
</code_context>
<issue_to_address>
Logging uses info level for errors.
Use `logger.e` for error messages to clearly indicate error-level events instead of `logger.i`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lib/view/homescreen.dart
Outdated
| animationProvider.stopAllAnimations.call(); // If method exists | ||
| animationProvider.initializeAnimation.call(); // If method exists |
There was a problem hiding this comment.
issue (bug_risk): Calling methods with .call() may cause runtime errors if not callable.
Using .call() here will throw if these are standard methods. Invoke them directly as methods instead.
lib/view/homescreen.dart
Outdated
| style: _selectedFontFamily != null | ||
| ? GoogleFonts.getFont(_selectedFontFamily!) | ||
| : null, |
There was a problem hiding this comment.
issue: No fallback for invalid or unavailable font family.
Validate _selectedFontFamily or add a fallback to avoid exceptions if the font is unsupported.
| try { | ||
| final path = await _getFilePath(filename); | ||
| final file = File(path); | ||
|
|
||
| if (await file.exists()) { | ||
| final content = await file.readAsString(); | ||
| final dynamic decodedData = jsonDecode(content); | ||
|
|
||
| // Automatically return decoded JSON as a dynamic type | ||
| return decodedData; | ||
| } else { | ||
| logger.d('File not found: $filename'); | ||
| return null; | ||
| } | ||
| return await _badgeFileLock.synchronized(() async { | ||
| if (await file.exists()) { | ||
| final content = await file.readAsString(); | ||
| return jsonDecode(content); | ||
| } else { | ||
| logger.i('File not found: $path'); | ||
| return null; |
There was a problem hiding this comment.
nitpick: Logging uses info level for errors.
Use logger.e for error messages to clearly indicate error-level events instead of logger.i.
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/17009673723/artifacts/3779983603. 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}
|
@nope3472 will you add recording or image for the output ? |
There was a problem hiding this comment.
Please check the following
- Code structure and maintainability: The font-handling logic is directly embedded within core UI or rendering code, making it difficult to maintain or extend. Recommendation:
- Move font-specific logic (e.g., style detection, hex generation) into a dedicated utility or service class.
- Keep the UI code focused on layout and user interaction, not logic processing.
- Lack of inline documentation: There are no comments explaining how the font style logic works, especially for the hex string generation and bitmap handling. Recommendation:
- Add
///comments above all public methods and non-obvious logic blocks. - Document the font support limitations (e.g., resolution, character set constraints).
-
Hardware constraints not accounted for
Based on prior discussion, some fonts clip or render indistinctly due to badge resolution limits. However, these limitations are not surfaced in the code or PR description. Clarify, how could this be handled, e.g. Would it be suitable to add conditionals or UI warnings if unsupported font styles are selected? -
Missing tests: No unit or integration tests are provided to verify the font conversion logic or UI state handling. Recommendation:
- Add test coverage for font selection logic.
- Test fallback behavior if a font cannot be applied.
- PR Description: Add the issue that is fixed into the PR description.
|
@mariobehling sure i will add the warning for specific fonts and will add the comments for the understanding |
| segments.add({'type': 'text', 'content': currentText}); | ||
| } | ||
|
|
||
| // If using default font, return per-character hexes (old behavior) |
There was a problem hiding this comment.
@nope3472 as per our last talk we want to get rid of the maps that we have already but I can see them getting used here.
There was a problem hiding this comment.
@Jhalakupadhyay i was thinking that as we have limitation with few characters provided in our current firmware size so we keep it as backup like is any character required more than alloted space we can fallback to this default.
pubspec.yaml
Outdated
| @@ -51,6 +51,7 @@ dependencies: | |||
| google_fonts: ^6.2.1 | |||
| url_launcher: ^6.3.1 | |||
| image: ^4.5.4 | |||
| synchronized: ^3.3.1 | |||
There was a problem hiding this comment.
@nope3472 If we are not using them please remove the dependency from here.
There was a problem hiding this comment.
@Jhalakupadhyay yeah sure
There was a problem hiding this comment.
@nope3472 and also as per our previous talk we wanted to get rid of the existing map I can still see we are using it here.
|
@nope3472 this #1259 also adds support for different fonts. Please review it and enhance your implementation of font support accordingly. Check the method used to read the fontStyle, how it is sent to the badge, whether the transfer works correctly, and which font styles are currently implemented. Also, consider which logic can be applied to make it easier to add new fonts in the future. |
|
@nope3472 any progress here? |
I see that you are creating a saved text feature, I think its better to create a seperate issue for this. Do you mind explaining me how you are using json to save the text vivek.download.mp4 |
|
hey @Vishveshwara thank for pointing this out i have fixed your concerns and here is how i am using json to save the text- |
yes now the fontstyles are more accurate and gets transferred to the badge correctly |
|
@Vishveshwara there will be some fonts which will not be able to render the characters correctly due to our 11x8 logic. |
|
@AsCress What do you think cause of this ? |
|
@samruddhi-Rahegaonkar yeah will do the changes |
|
@nope3472 here the failing CIs cause is not you code i guess, its something related to CI/CD |
|
@nope3472 please check if any font supports chinese characters |
|
@nope3472 Please add recordings for the new changes or lets review in tommorows meeting anything can work. |
|
@samruddhi-Rahegaonkar can you please review? |
|
@nope3472 please resolve conflict |
|
@hpdang i have resolved the conflicts |
Summary
Add a font‐picker button to the HomeScreen text field so users can choose and apply Google Fonts in real time.
Changes
FontPickerDialogfor selecting fonts.google_fontspackage.Checklist
constants.dart.flutter analyze&flutter test).images
Summary by Sourcery
Add a font picker to the HomeScreen text field and extend badge persistence and editing capabilities, while improving file handling and data management utilities.
New Features:
Enhancements:
Build:
Chores: