feat: add flip functionality on final processed image#46
feat: add flip functionality on final processed image#46kienvo merged 12 commits intofossasia:mainfrom
Conversation
Reviewer's GuideThis PR introduces horizontal and vertical flip functionality for the final processed image by converting the image editor to a stateful widget with flip toggles, extracting processing steps into a reusable utility, adding dedicated UI controls for flipping, and updating a dependency version. Sequence Diagram for Image Flip OperationsequenceDiagram
actor User
participant FC as FlipControls
participant IES as _ImageEditorState
participant IEU as ImageEditorUtils
participant IL as ImageList
User->>+FC: Taps 'Flip Horizontal'
FC->>IES: onFlipHorizontal()
activate IES
IES->>IES: toggleFlipHorizontal()
IES->>IES: setState({ flipHorizontal = !flipHorizontal })
IES->>+IEU: processImages(originalImage, newFlipH, flipV, epd)
IEU-->>-IES: List~img.Image~ (flipped)
IES->>IL: Update with new images (during rebuild)
IL-->>User: Displays horizontally flipped image
deactivate IES
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @Dhruv1797 - I've reviewed your changes - here's some feedback:
- The flip controls are using the opposite asset filenames (the horizontal flip button shows the vertical icon and vice versa), please swap the asset paths.
- Consider giving the flip buttons a visual “active” state (e.g. change color or icon) so users know when horizontal or vertical flipping is currently applied.
- The intl version bump seems unrelated to this feature—please separate that dependency update into its own PR to keep the flip functionality focused.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 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.
| child: Image.asset( | ||
| "assets/images/vertical_flip.png", | ||
| height: 24, | ||
| width: 24, |
There was a problem hiding this comment.
suggestion (bug_risk): Horizontal flip button uses vertical flip asset
The flipH button should use the horizontal flip icon asset instead of the vertical_flip icon.
| child: Image.asset( | |
| "assets/images/vertical_flip.png", | |
| height: 24, | |
| width: 24, | |
| child: Image.asset( | |
| "assets/images/horizontal_flip.png", | |
| height: 24, | |
| width: 24, |
| for (final method in epd.processingMethods) { | ||
| processedImgs.add(method(transformed)); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Clone transformed image per method to avoid side effects
Passing the same transformed image to all methods may lead to unintended side effects if any method mutates its input. Pass a copy of transformed to each method instead.
| for (final method in epd.processingMethods) { | |
| processedImgs.add(method(transformed)); | |
| } | |
| for (final method in epd.processingMethods) { | |
| processedImgs.add(method(img.copyResize(transformed))); | |
| } |
| } | ||
| } | ||
| final List<img.Image> processedImgs = orgImg != null | ||
| ? processImages( |
There was a problem hiding this comment.
suggestion (performance): Reprocessing on every build might hurt performance
Consider memoizing processImages results when its inputs (orgImg, flipHorizontal, flipVertical, epd) are unchanged to prevent unnecessary repeated processing.
| @@ -0,0 +1,47 @@ | |||
| import 'package:flutter/material.dart'; | |||
|
|
|||
| class FlipControls extends StatelessWidget { | |||
There was a problem hiding this comment.
suggestion: Add visual state feedback for flip controls
Pass the flip state to FlipControls and update the button style or icon color to reflect when a flip is active.
Suggested implementation:
class FlipControls extends StatelessWidget {
final VoidCallback onFlipHorizontal;
final VoidCallback onFlipVertical;
final bool isFlippedHorizontally;
final bool isFlippedVertically;
const FlipControls({
super.key,
required this.onFlipHorizontal,
required this.onFlipVertical,
required this.isFlippedHorizontally,
required this.isFlippedVertically,
});
@override
Widget build(BuildContext context) {
final Color activeColor = Theme.of(context).colorScheme.primary;
final Color inactiveColor = Theme.of(context).iconTheme.color ?? Colors.black;
return Padding(
You will also need to update the widget tree inside build to use the isFlippedHorizontally and isFlippedVertically booleans. For example, if you use IconButton for the flip controls, set the color property of the icon to activeColor when the corresponding flip is active, otherwise use inactiveColor.
Additionally, update all usages of FlipControls to pass the new required parameters: isFlippedHorizontally and isFlippedVertically.
Vishveshwara
left a comment
There was a problem hiding this comment.
@Dhruv1797 This looks good, well done!! @AsCress @kienvo can you guys review this as well?
|
Performance issue. Please avoid flipping every image. Instead, use Transform.flip to flip the view, then use Image.flip** to flip the selected image at the transfer event. |
@kienvo Nice catch, Thank you, i have made this change now using use Transform.flip instead of flipping every image and only fliping the selected one at the transfer event. please review it once. |
|
Looks good! There is one thing left: |
Thanks @Kinevo, yeah sure, this is done. I’ve rotated |
Fixes #35: added Flip (horizontal/vertical) for the final processed image to enhance output flexibility.
Previous UI: There was no option for the user to flip the final processed image.
Updated Ui: Now there are two button at bottom right corner that allows user to Flip (horzontal/vertical) the final processed image.
WhatsApp.Video.2025-06-04.at.10.49.54.PM.mp4
Summary by Sourcery
Introduce flip functionality for final processed images by adding UI controls and integrating flip transformations into the image processing workflow
New Features:
Enhancements:
Build: