feat: Added undo/redo functionality for drawing cliparts #1378
+143
−36
Conversation
This file contains hidden or 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 GuideThis PR implements full undo/redo support and toggleable shape‐drawing modes in the draw clipart feature, refactors the drawing toolbar UI into modular builder methods, and enhances gesture handling and grid management in the provider. Sequence diagram for drawing with undo/redo and shape previewsequenceDiagram
actor User
participant DrawBadge as DrawBadgeScreen
participant BMBadge
participant Provider as DrawBadgeProvider
User->>DrawBadge: Tap and drag to draw
DrawBadge->>BMBadge: Pass gesture events
BMBadge->>Provider: pushToUndoStack()
BMBadge->>Provider: setCell()/clearPreviewGrid() (for shape preview)
BMBadge->>Provider: commitGridUpdate() (on gesture end)
User->>DrawBadge: Tap Undo
DrawBadge->>Provider: undo()
User->>DrawBadge: Tap Redo
DrawBadge->>Provider: redo()
Sequence diagram for toggling and using shape toolssequenceDiagram
actor User
participant DrawBadge as DrawBadgeScreen
participant Provider as DrawBadgeProvider
User->>DrawBadge: Tap Shapes button
DrawBadge->>DrawBadge: Show shape options
User->>DrawBadge: Select shape (e.g., Rectangle)
DrawBadge->>Provider: setShape(Rectangle)
User->>DrawBadge: Draw gesture
DrawBadge->>Provider: setCell()/clearPreviewGrid() (preview shape)
User->>DrawBadge: Release gesture
DrawBadge->>Provider: commitGridUpdate()
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 @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
- The Undo and Redo buttons are always enabled—consider using drawProvider.canUndo/canRedo to disable them when no actions are available.
- The new UndoRedoControls widget isn’t actually integrated anywhere—either wire it into your screen or remove it to avoid dead code.
- Consider renaming GridPosition.x and .y to row/col (or similar) to make the mapping between coordinates and grid indices clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Undo and Redo buttons are always enabled—consider using drawProvider.canUndo/canRedo to disable them when no actions are available.
- The new UndoRedoControls widget isn’t actually integrated anywhere—either wire it into your screen or remove it to avoid dead code.
- Consider renaming GridPosition.x and .y to row/col (or similar) to make the mapping between coordinates and grid indices clearer.
## Individual Comments
### Comment 1
<location> `lib/virtualbadge/view/draw_badge.dart:45` </location>
<code_context>
+ final localPosition = _getLocalPosition(details.globalPosition);
+ final shape = drawProvider.selectedShape;
+
+ final start = drawProvider.getGridPosition(dragStart!, _cellSize);
+ final end = drawProvider.getGridPosition(localPosition, _cellSize);
+
</code_context>
<issue_to_address>
Potential null dereference of dragStart.
Since dragStart is force-unwrapped, add a null check or assertion to avoid runtime errors if _handlePanUpdate is called before _handlePanStart.
</issue_to_address>
### Comment 2
<location> `lib/providers/draw_badge_provider.dart:73` </location>
<code_context>
+ }
+ }
+
+ void commitGridUpdate() {
+ _pushToUndoStack();
+ for (int i = 0; i < rows; i++) {
</code_context>
<issue_to_address>
commitGridUpdate overwrites cells with preview values without considering erasing.
The current logic only updates cells when _previewGrid[i][j] is true, so erasing (when a preview cell is false) isn't handled. Please update the logic to support both drawing and erasing in preview mode.
</issue_to_address>
### Comment 3
<location> `lib/view/draw_badge_screen.dart:197` </location>
<code_context>
+ fileHelper.generateClipartCache();
+ ToastUtils().showToast("Clipart Saved Successfully");
+
+ Future.delayed(const Duration(milliseconds: 800), () {
+ Navigator.of(context).popUntil((route) => route.isFirst);
+ });
</code_context>
<issue_to_address>
Fixed delay before navigation may not be robust.
Trigger navigation after the save operation completes, rather than relying on a fixed delay, to ensure reliability regardless of save duration.
Suggested implementation:
```
await fileHelper.generateClipartCache();
ToastUtils().showToast("Clipart Saved Successfully");
if (mounted) {
Navigator.of(context).popUntil((route) => route.isFirst);
}
@override
```
- If `generateClipartCache()` is not already `async` and does not return a `Future`, you will need to make it asynchronous for this to work.
- The containing function must be marked as `async` and return a `Future`, and any callers must handle this.
- If the save operation itself is asynchronous, you should also `await` it before generating the cache.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
- Consider disabling or visually dimming the Undo/Redo buttons when canUndo/canRedo is false so users get clear feedback about available actions.
- You might want to cap the size of the undo/redo stacks (e.g. a max history length) to prevent unbounded memory growth during long drawing sessions.
- The toolbar has grown quite large—extracting it (and its button builders) into its own widget or smaller components could improve readability and maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider disabling or visually dimming the Undo/Redo buttons when canUndo/canRedo is false so users get clear feedback about available actions.
- You might want to cap the size of the undo/redo stacks (e.g. a max history length) to prevent unbounded memory growth during long drawing sessions.
- The toolbar has grown quite large—extracting it (and its button builders) into its own widget or smaller components could improve readability and maintenance.
## Individual Comments
### Comment 1
<location> `lib/providers/draw_badge_provider.dart:54` </location>
<code_context>
- _drawViewGrid[i][j] = badgeData[i][j];
- } else {
- _drawViewGrid[i][j] = false;
+ void setCell(int row, int col, bool value, {bool preview = false}) {
+ if (row >= 0 && row < rows && col >= 0 && col < cols) {
+ if (preview) {
</code_context>
<issue_to_address>
setCell notifies listeners on every cell change, which may cause excessive rebuilds.
Batch updates and call notifyListeners() once after all relevant cells are set to avoid multiple unnecessary UI rebuilds during bulk operations.
Suggested implementation:
```
void setCell(int row, int col, bool value, {bool preview = false, bool notify = true}) {
if (row >= 0 && row < rows && col >= 0 && col < cols) {
if (preview) {
_previewGrid[row][col] = value;
} else {
_drawViewGrid[row][col] = value;
if (notify) {
notifyListeners();
}
}
}
}
/// Batch update cells and notify listeners only once after all updates.
void setCells(List<Tuple4<int, int, bool, bool>> updates) {
for (final update in updates) {
// update.item1: row, update.item2: col, update.item3: value, update.item4: preview
setCell(update.item1, update.item2, update.item3, preview: update.item4, notify: false);
}
notifyListeners();
}
```
- You will need to import a tuple class, such as `package:tuple/tuple.dart`, or define your own data structure for the batch update parameter in `setCells`.
- Update any code that performs bulk cell updates to use `setCells` instead of calling `setCell` in a loop.
- If you want a more type-safe or readable batch update, consider defining a small class or struct for the cell update instead of using `Tuple4`.
</issue_to_address>
### Comment 2
<location> `lib/providers/draw_badge_provider.dart:73` </location>
<code_context>
+ }
+ }
+
+ void commitGridUpdate() {
+ _pushToUndoStack();
+ for (int i = 0; i < rows; i++) {
</code_context>
<issue_to_address>
commitGridUpdate overwrites cells with preview values, potentially losing previous cell state.
Consider merging _previewGrid and _drawViewGrid using logical OR/AND or a more explicit strategy to avoid unintentionally overwriting existing cell states.
</issue_to_address>
### Comment 3
<location> `lib/providers/draw_badge_provider.dart:112` </location>
<code_context>
+
+ // ========== UNDO / REDO ==========
+
+ void _pushToUndoStack() {
+ _undoStack.add(_copyGrid(_drawViewGrid));
+ _redoStack.clear(); // Invalidate redo stack on new action
</code_context>
<issue_to_address>
Undo/redo stacks may grow unbounded, leading to increased memory usage.
Consider setting a maximum size for _undoStack and _redoStack, discarding the oldest entries when the limit is exceeded to prevent excessive memory usage.
Suggested implementation:
```
void _pushToUndoStack() {
_undoStack.add(_copyGrid(_drawViewGrid));
_redoStack.clear(); // Invalidate redo stack on new action
}
=======
// ========== UNDO / REDO ==========
static const int _maxUndoRedoStackSize = 50;
void _pushToUndoStack() {
_undoStack.add(_copyGrid(_drawViewGrid));
if (_undoStack.length > _maxUndoRedoStackSize) {
_undoStack.removeAt(0); // Remove the oldest entry
}
_redoStack.clear(); // Invalidate redo stack on new action
}
```
```
void undo() {
if (_undoStack.isNotEmpty) {
_redoStack.add(_copyGrid(_drawViewGrid));
if (_redoStack.length > _maxUndoRedoStackSize) {
_redoStack.removeAt(0); // Remove the oldest entry
}
_drawViewGrid = _undoStack.removeLast();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nope3472
reviewed
Jul 22, 2025
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
7371fb3 to
5f1fd8b
Compare
mariobehling
requested changes
Jul 30, 2025
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
1b822ff to
8751896
Compare
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/17409222203/artifacts/3908098980. ScreenshotsAndroid Screenshots
iPhone Screenshots
iPad Screenshots
|
||||||||||||||||||||||||
-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
approved these changes
Jul 30, 2025
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
887546b to
8751896
Compare
|
@samruddhi-Rahegaonkar resolve conflict |
nope3472
reviewed
Aug 10, 2025
There was a problem hiding this comment.
@samruddhi-Rahegaonkar use the pull request.yml file from #1381 the test will run correctly
|
@samruddhi-Rahegaonkar the stable one was failing for the animations pr so i looked at the available versions and used them. |
Yeah, But that is not appropriate fix we need to figure out something to make it automated. |
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
856cd7f to
8751896
Compare
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
d91c731 to
2bb692c
Compare
nope3472
approved these changes
Aug 16, 2025
|
@samruddhi-Rahegaonkar please resolve conflicts |
|
Hey, @hpdang We can merge this. |
|
@samruddhi-Rahegaonkar please see screenshot |
I will raise fix shortly. |
fixed!
|
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
8b0532d to
448c658
Compare
|
@samruddhi-Rahegaonkar it looks like the undo/redo buttons are activated only after user clicks on Erase. Can you explain why the behavior of this function please? |
|
@hpdang Undo/Redo Buttons are activated not after clicking on erase but when we move out from the shapes. undo redo functionality is only applied for freehand drawing. and it requires some trigger to get activated like clicking on shapes or draw again. |
|
Let's show it live at the meeting today, I find some issues with this function |
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
6d121b7 to
fe6885c
Compare
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
d3578fb to
b16fc0b
Compare
|
@hpdang Can we merge this ? |
mariobehling
changed the title
|
samruddhi-Rahegaonkar
force-pushed
the
issue1377
branch
from
436c79f to
5e32be6
Compare
Screen.Recording.2025-09-02.at.9.37.55.PM.mov |
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 #1377
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Add undo/redo and shape-drawing support to the clipart editor and refactor UI and provider for improved state management
New Features:
Enhancements: