feat: Enable editing of saved badges (fixes #1305) #1311
Conversation
Reviewer's GuideImplements in-place editing of saved badges by extending HomeScreen to accept saved data, loading that data into input fields and providers, and invoking a new update flow in SavedBadgeProvider to overwrite existing badge files (and cache) rather than creating duplicates. Sequence Diagram for Updating an Existing BadgesequenceDiagram
actor User
participant HomeScreen
participant SavedBadgeProvider
participant BadgeTextStorage
participant FileSystemCache as "File System / Cache"
User->>HomeScreen: Clicks 'Save' (for an existing badge)
HomeScreen->>HomeScreen: if editing (savedBadgeFilename is not null)
HomeScreen->>SavedBadgeProvider: updateBadgeData(filename, newText, effects, speed, mode)
SavedBadgeProvider->>SavedBadgeProvider: Create/Prepare Data object for badge
SavedBadgeProvider->>FileSystemCache: Write updated badge data to existing file (e.g., filename.json)
SavedBadgeProvider->>FileSystemCache: Update FileHelper.imageCacheProvider.savedBadgeCache
SavedBadgeProvider->>BadgeTextStorage: saveOriginalText(filename.json, newText)
BadgeTextStorage->>FileSystemCache: Update/Write badge_original_texts.json file
BadgeTextStorage-->>SavedBadgeProvider: Original text saved
SavedBadgeProvider-->>HomeScreen: Badge update complete
HomeScreen->>User: Show "Badge Updated Successfully" toast
Class Diagram for Badge Editing Feature ChangesclassDiagram
class HomeScreen {
+savedBadgeData: Map~String, dynamic~? (new)
+savedBadgeFilename: String? (new)
#_applySavedBadgeData(): void (new)
+handleSave(): void (modified: calls updateBadgeData if editing)
}
class SavedBadgeProvider {
+updateBadgeData(String filename, String message, bool isFlash, bool isMarquee, bool isInvert, int? speed, int animation): Future~void~ (new)
+saveBadgeData(String filename, String message, ...): Future~void~ (modified: calls BadgeTextStorage.saveOriginalText)
}
class BadgeTextStorage {
<<Utility>>
+TEXT_STORAGE_FILENAME: String (new)
+saveOriginalText(String badgeFilename, String originalText): Future~void~ (new)
+getOriginalText(String badgeFilename): Future~String~ (new)
+deleteOriginalText(String badgeFilename): Future~void~ (new)
}
class FileHelper {
+saveBadgeData(Data data, String filename, bool isInvert): Future~void~ (modified: updates cache entry if exists)
+jsonToData(Map~String, dynamic~ jsonData): Data (modified: handles missing 'messages' key)
}
class SaveBadgeCard {
+handleEdit(): void (modified: navigates to HomeScreen with badge data)
#_safeGetFlashValue(Map~String,dynamic~ data): bool (new)
#_safeGetMarqueeValue(Map~String,dynamic~ data): bool (new)
#_safeGetInvertValue(Map~String,dynamic~ data): bool (new)
}
HomeScreen "1" o-- "1" SavedBadgeProvider : uses
SaveBadgeCard ..> HomeScreen : navigates to / passes data to
SavedBadgeProvider "1" o-- "1" FileHelper : uses
SavedBadgeProvider "1" o-- "1" BadgeTextStorage : 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 @nope3472 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- fileHelper is undefined in this scope (link)
General comments:
- HomeScreen has grown very large with UI and business logic mixed—consider extracting the badge-editing setup (applying savedBadgeData, parsing mode/speed, etc.) into a dedicated controller or service to improve readability and maintainability.
- The enum-to-int mapping logic for modes and speeds (string parsing, switch statements) is duplicated and verbose; centralize that in your enums or a shared utility to avoid inconsistencies.
- SaveBadgeCard uses pushAndRemoveUntil to navigate back into HomeScreen which clears the entire stack; consider using a normal push or preserving previous routes to avoid unexpected UX/navigation side effects.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 5 other issues
- 🟢 Security: all looks good
- 🟢 Testing: 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.
lib/view/homescreen.dart
Outdated
| _startImageCaching(); | ||
| speedDialProvider = SpeedDialProvider(animationProvider); | ||
| super.initState(); | ||
|
|
||
| _tabController = TabController(length: 3, vsync: this); |
There was a problem hiding this comment.
suggestion: Call super.initState() before other initializations
Invoke super.initState() at the start of initState() to ensure proper initialization of inherited behavior.
| _startImageCaching(); | |
| speedDialProvider = SpeedDialProvider(animationProvider); | |
| super.initState(); | |
| _tabController = TabController(length: 3, vsync: this); | |
| super.initState(); | |
| _startImageCaching(); | |
| speedDialProvider = SpeedDialProvider(animationProvider); | |
| _tabController = TabController(length: 3, vsync: this); |
|
|
||
| // Update the cache | ||
| final cacheKey = '$cleanFilename.json'; | ||
| final cache = fileHelper.imageCacheProvider.savedBadgeCache; |
There was a problem hiding this comment.
issue (bug_risk): fileHelper is undefined in this scope
Instantiate fileHelper in updateBadgeData or use a shared instance before referencing fileHelper.imageCacheProvider.
| // If we're editing an existing badge, update it instead of showing save dialog | ||
| if (widget.savedBadgeFilename != null) { | ||
| // Update the existing badge file using the new updateBadgeData method | ||
| SavedBadgeProvider savedBadgeProvider = |
There was a problem hiding this comment.
issue (bug_risk): Instantiating SavedBadgeProvider in widget build
Use Provider.of(context, listen: false) to access the existing provider instead of creating a new instance.
There was a problem hiding this comment.
@nope3472 look at this
| String badgeFilename = badgeData.key; | ||
|
|
||
| // Navigate to HomeScreen and replace the current route | ||
| Navigator.of(context).pushAndRemoveUntil( |
There was a problem hiding this comment.
suggestion: pushAndRemoveUntil clears entire navigation stack
Using pushAndRemoveUntil removes all previous routes, which may not be intended. Consider pushReplacement to preserve navigation history.
| /// @param isInvert Whether invert effect is enabled | ||
| /// @param speed The speed value for the animation | ||
| /// @param animation The animation mode index | ||
| Future<void> updateBadgeData(String filename, String message, bool isFlash, |
There was a problem hiding this comment.
suggestion (bug_risk): Missing notifyListeners() after updating badge data
Call notifyListeners() after updating the badge file and cache to ensure UI updates correctly.
Suggested implementation:
Future<void> updateBadgeData(String filename, String message, bool isFlash,
bool isMarquee, bool isInvert, int? speed, int animation) async {
// Make sure filename doesn't have .json extension
String cleanFilename = filename;
if (cleanFilename.endsWith('.json')) {
cleanFilename = cleanFilename.substring(0, cleanFilename.length - 5);
}
logger.i('Updating existing badge: $cleanFilename');
// Create the updated badge data
Data data = await getBadgeData(
message,
// Create the updated badge data
Data data = await getBadgeData(
message,
);
// ... (rest of your update logic, e.g. writing to file, updating cache, etc.)
notifyListeners();
Place the notifyListeners(); call at the end of the function, after all badge data and cache updates are complete, to ensure the UI is notified of changes. If there is additional logic after the shown code (such as writing to file or updating an in-memory cache), ensure notifyListeners(); is called after those operations.
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/16655090297/artifacts/3660377402. 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}
|
@mariobehling can you review this pr |
|
Please check sourcery comments and resolve where reasonable first. @nope3472 |
|
sure @mariobehling will make these minor improvements |
There was a problem hiding this comment.
Is it necessary to change the getIntValue method to return speed.index + 1 instead of parsing it from the hex value for resolving the current issue?
There was a problem hiding this comment.
I’m using this so the mapping’s simpler—otherwise we’d have to mess with the hex value and convert it back just to end up with the same integer.
There was a problem hiding this comment.
- Every get, save, or delete operation reads/writes the whole JSON file. For a large number of badges, this could become slow.
- If two badges are saved simultaneously (race condition), one could overwrite the other's changes, leading to lost data.
There was a problem hiding this comment.
we are saving it in local storage its not in the same DB and at an instance only one badge will be saved or edited user cannot be doing two things at a time.
Json we are saving is very snall so that will not be an issue.
|
hey @mariobehling @Jhalakupadhyay can you guys finalise this pr and any other possible issues that you think should be resolved for this |
There was a problem hiding this comment.
why are we storing the data why wen are not directly assigning the data in the controllers and route the user to the homescreen. Everything that the user needs to edit is bieng saved already in the device storage we can use that to update the controllers rather than saving the data in the cache for sometime.
There was a problem hiding this comment.
@Jhalakupadhyay i have now Refactored the edit flow so that only the badge filename is passed to HomeScreen.
Refactored HomeScreen to load badge data from disk and populate controllers/providers.
|
@nope3472 can you break it down so it's easy to review. Follow: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/helping-others-review-your-changes. |
|
@nope3472 You have updated the test or Navigation? |
|
Hey @samruddhi-Rahegaonkar — I’ve changed the flow slightly: |
|
@nope3472 after clicking on overwrite or overwriting a badge it should go to Saved badge Screen. isn't it ? |
|
@nope3472 After saving or editing a badge, the text field isn't clearing when navigating back to the Home screen. |
|
@nope3472 Should badge names be treated as case-insensitive like (one, One, ONE) ? |
Its asking for Overwrite but its not updating the name of the original one instead creating duplicate this is resulting in the wrong JSON sharing where both the badges having different characterstics but after sharing treating same. |
|
this is development (Having Marquee = true) : this is Development (having Marquee=false) : |
|
@samruddhi-Rahegaonkar thanks for pointing this out will do it |
There was a problem hiding this comment.
@nope3472 Why always there a pre-created empty badge named as badge_original_texts Please fix this ?
Second thing After saving or editing a badge, the text field isn't clearing when navigating back to the Home screen.
|
@samruddhi-Rahegaonkar i will look for the empty badge but for clearing the text the original code of the app reatins the text after save so i have also followed that same rule |
|
@nope3472 After overwriting its creating another again |
There was a problem hiding this comment.
@nope3472 Have you tested this ? because in my machine the same file names are still do not overwriting each other instead it is creating new badge as previous.
🎯 What does this PR do?
This PR implements in-place editing of saved badges so that updates modify the existing badge file and cache, instead of creating duplicates.
🐛 Problem Solved
Fixes #1305 – previously, editing a saved badge would spawn a new badge, cluttering the list with duplicates. Now the original badge is updated directly.
recording
Screen_Recording_20250716_200307.mp4
Screen_Recording_20250716_200758.mp4
✨ Changes Made
Saving a New Badge
Where:
lib/providers/saved_badge_provider.dartsaveBadgeData()method: Serializes badge data and writes it to disk.lib/bademagic_module/utils/file_helper.dartsaveBadgeData()method: Handles writing the JSON file and updating the cache.lib/bademagic_module/utils/badge_text_storage.dartsaveOriginalText()method: Stores the original text for later restoration.Editing an Existing Badge
Where:
lib/view/widgets/save_badge_card.dartonPressed: Navigates toHomeScreen, passing only the badge filename.lib/view/homescreen.dart_loadBadgeDataFromDisk()method:Saving an Edited Badge
Where:
lib/view/homescreen.dartupdateBadgeData()instead of creating a new badge.lib/providers/saved_badge_provider.dartupdateBadgeData()method:lib/bademagic_module/utils/file_helper.dartsaveBadgeData()method: Handles the actual file overwrite and cache update logic.Robustness & User Experience
Where:
lib/view/homescreen.dart_loadBadgeDataFromDisk()method:try/catch, showing user-friendly error messages on failure.lib/bademagic_module/utils/file_helper.dartlib/providers/saved_badge_provider.dart🧪 How to Test
📸 Before vs After
Before: Editing a saved badge created a duplicate.
After: Editing a saved badge updates the original in-place.