-
Notifications
You must be signed in to change notification settings - Fork 234
feat: Added Support for Fonts #1259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: flutter_app
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces support for custom fonts in the application. It adds a dropdown menu to select from a list of Google Fonts, and implements the logic to render text using the selected font. The implementation includes handling characters with descenders and optimizing for characters that exceed the grid size. Sequence diagram for rendering text with a custom fontsequenceDiagram
participant User
participant HomeScreen
participant FontProvider
participant Converters
User->>HomeScreen: Enters text and selects font
HomeScreen->>FontProvider: changeFont(newFont)
FontProvider-->>HomeScreen: Notifies listeners
HomeScreen->>Converters: messageTohex(message, isInverted)
Converters->>FontProvider: get selectedTextStyle
Converters->>Converters: _processCustomFontChar(char, style)
Converters-->>HomeScreen: Returns hex strings
HomeScreen->>HomeScreen: Updates badge with new font
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Vishveshwara - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the font rendering logic into a separate widget for better reusability and testability.
- The
_processCustomFontChar
function has hardcoded character checks; consider using a more data-driven approach.
Here's what I looked at during the review
- 🟡 General issues: 1 issue 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.
bool hasDescender = false; | ||
if (char == "y" || | ||
char == "g" || | ||
char == "p" || | ||
char == "q" || | ||
char == "j") { | ||
hasDescender = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Simplify the descender character check.
Rather than checking each character individually (e.g. 'y', 'g', etc.), consider using a Set and a single contains check for better readability and easier maintenance.
bool hasDescender = false; | |
if (char == "y" || | |
char == "g" || | |
char == "p" || | |
char == "q" || | |
char == "j") { | |
hasDescender = true; | |
} | |
final descenders = {'y', 'g', 'p', 'q', 'j'}; | |
bool hasDescender = descenders.contains(char); |
@Vishveshwara Thanks for working ! |
@samruddhi-Rahegaonkar The issue with spacing of characters is due to the width of characters like "w","h","m" which does not fit in 11x8 grid which is the size we are using for the default font. so for now , I have written a code in which a wide character can occupy 11x16 grid and hence the character does not get cut off but the character occupies double the space. app.vid.mp4the Solutions for this issue:
I will try to work on this, @Jhalakupadhyay can you guide me on which possible solution would be the best |
@Vishveshwara That’s a good temporary fix, but it introduces an inconsistency where some characters take double the space, breaking uniform alignment. |
@Vishveshwara you might try |
@Vishveshwara nice going but there are few changes needed.
I will look closely to PR once and then can tell you better, but we can go for the above changes first. |
font_support.mp4Challenges
@Jhalakupadhyay @adityastic please review this PR. Changes
|
Why do we have 13k loc changed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need 13k lines to have font support in our app.
@adityastic |
I don't think he meant caching it directly in the repository 😅 |
I'm sorry for the confusion. I noticed that the default font was being cached in "lib/badgemagic_module/utils/data_to_bytearray_converter.dart", and so I assumed it was acceptable to cache them directly. However, I understand that this might not be the ideal approach. The code can work without caching as well, so I would appreciate your guidance on where the best place to cache these characters would be. |
We should cache every character in memory while conversation is being performed and reuse the character or the subset if we see repeated entries to improve execution time. That's all to it I guess. Let me know if you have any other thoughts. |
@adityastic |
@adityastic can you please review this pr? I have done the changes you have mentioned. |
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/15227284094/artifacts/3190499738. Screenshots (Android)
Screenshots (iPhone)
Screenshots (iPad)
|
@nope3472 i have read your scrum update, as you are working on the font change feature , you can check this PR out and mention your comments. |
Fixes #239
Fixes #1134
Changes
Screenshots / Recordings
Font.Support.mp4
Checklist:
constants.dart
without hard coding any value.Summary by Sourcery
Add support for custom fonts in the badge display, allowing users to select from a predefined list of Google Fonts and dynamically render text with improved character rendering
New Features:
Enhancements: