-
Notifications
You must be signed in to change notification settings - Fork 11
feat: added ImageLibrary Section. #63
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: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR introduces a full-featured Image Library section backed by persistent storage and a provider, with end-to-end workflows for saving, renaming, deleting (single and batch), filtering/searching, previewing, and direct transfer of images to the ePaper device, and integrates it into the existing editor and app state. Sequence diagram for saving an image from the editor to the librarysequenceDiagram
actor User
participant Editor as ImageEditor
participant SaveHandler as ImageSaveHandler
participant SaveDialog as ImageSaveDialog
participant Provider as ImageLibraryProvider
participant OpsService as ImageOperationsService
User->>Editor: Clicks 'Save to Library'
Editor->>SaveHandler: _saveCurrentImage()
SaveHandler->>SaveDialog: Show save dialog
User->>SaveDialog: Enters image name, confirms save
SaveDialog->>SaveHandler: onSave(imageName)
SaveHandler->>OpsService: saveImageWithFeedback(...)
OpsService->>Provider: saveImage(...)
Provider-->>OpsService: Image saved
OpsService-->>SaveHandler: Show success feedback
Sequence diagram for deleting images (single and batch) from the librarysequenceDiagram
actor User
participant Library as ImageLibraryScreen
participant Provider as ImageLibraryProvider
participant OpsService as ImageOperationsService
participant DeleteDialog as DeleteConfirmationDialog
participant BatchDialog as BatchDeleteConfirmationDialog
User->>Library: Selects image(s) to delete
alt Single delete
Library->>DeleteDialog: Show delete confirmation
User->>DeleteDialog: Confirms delete
DeleteDialog->>OpsService: deleteImage(image, provider)
OpsService->>Provider: deleteImage(id)
Provider-->>OpsService: Image deleted
OpsService-->>Library: Show success feedback
else Batch delete
Library->>BatchDialog: Show batch delete confirmation
User->>BatchDialog: Confirms delete
BatchDialog->>OpsService: batchDeleteImages(selected, provider)
OpsService->>Provider: deleteImage(id) (for each)
Provider-->>OpsService: Images deleted
OpsService-->>Library: Show batch success feedback
end
Entity relationship diagram for SavedImage and persistent storageerDiagram
SAVED_IMAGE {
string id
string name
string filePath
datetime createdAt
string source
json metadata
}
SAVED_IMAGE ||..|| IMAGE_LIBRARY_PROVIDER : manages
IMAGE_LIBRARY_PROVIDER ||..|| SHARED_PREFERENCES : persists_metadata
IMAGE_LIBRARY_PROVIDER ||..o| FILE : stores_image_file
FILE {
string path
bytes data
}
SHARED_PREFERENCES {
string key
string value
}
Class diagram for new and updated Image Library typesclassDiagram
class ImageLibraryProvider {
- List~SavedImage~ _savedImages
- bool _isLoading
- String _searchQuery
- String _selectedSource
+ List~SavedImage~ get savedImages
+ bool get isLoading
+ String get searchQuery
+ String get selectedSource
+ List~SavedImage~ get filteredImages
+ Future~void~ loadSavedImages()
+ Future~void~ saveImage(...)
+ Future~void~ deleteImage(String id)
+ Future~void~ renameImage(String id, String newName)
+ void updateSearchQuery(String query)
+ void updateSourceFilter(String source)
}
class SavedImage {
+ String id
+ String name
+ String filePath
+ DateTime createdAt
+ String source
+ Map~String, dynamic~? metadata
+ Map~String, dynamic~ toJson()
+ Future~Uint8List?~ getImageData()
+ Future~bool~ fileExists()
}
class ImageOperationsService {
+ Future~void~ renameImage(...)
+ Future~void~ deleteImage(...)
+ Future~void~ batchDeleteImages(...)
+ Future~void~ saveImageWithFeedback(...)
+ Future~void~ transferSingleImage(...)
+ String getFilterNameByIndex(...)
}
class ImageSaveHandler {
+ Future~void~ saveCurrentImage(...)
}
ImageLibraryProvider "1" o-- "*" SavedImage
ImageSaveHandler --> ImageOperationsService
ImageOperationsService --> ImageLibraryProvider
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
@Dhruv1797 Does this have some immediate changes in the UI which you could demo ? |
Here is the demo video: @AsCress @Vishveshwara please review it. |
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 @Dhruv1797 - 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/image_library/provider/image_library_provider.dart:56` </location>
<code_context>
+ final prefs = await SharedPreferences.getInstance();
+ final savedImagesJson =
+ prefs.getStringList('saved_images_metadata') ?? [];
+ _savedImages = [];
+ for (String json in savedImagesJson) {
+ try {
+ final image = SavedImage.fromJson(jsonDecode(json));
</code_context>
<issue_to_address>
Clearing _savedImages before loading may cause issues if loadSavedImages is called concurrently.
Concurrent calls to this method could lead to race conditions. Use synchronization (e.g., a lock) or prevent concurrent execution to avoid data inconsistency.
</issue_to_address>
### Comment 2
<location> `lib/image_library/services/image_operations_service.dart:117` </location>
<code_context>
+ ImageProcessing.bwrTriColorAtkinsonDither: 'BWR Atkinson',
+ ImageProcessing.bwrThreshold: 'BWR Threshold',
+ };
+ if (index < 0 || index >= processingMethods.length) return "Unknown";
+ return filterMap[processingMethods[index]] ?? "Unknown";
+ }
</code_context>
<issue_to_address>
Returning 'Unknown' for invalid filter index may hide bugs.
Consider logging a warning or raising an error in debug mode when an invalid index is encountered to help identify logic errors early.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (index < 0 || index >= processingMethods.length) return "Unknown";
return filterMap[processingMethods[index]] ?? "Unknown";
=======
if (index < 0 || index >= processingMethods.length) {
assert(() {
// This will only run in debug mode.
print('Warning: Invalid filter index $index encountered in getFilterName.');
return true;
}());
return "Unknown";
}
return filterMap[processingMethods[index]] ?? "Unknown";
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `lib/image_library/services/image_save_handler.dart:29` </location>
<code_context>
+ }) async {
+ if (rawImages.isEmpty) return;
+
+ img.Image finalImg = rawImages[selectedFilterIndex];
+
+ if (flipHorizontal) {
</code_context>
<issue_to_address>
No bounds check for selectedFilterIndex in rawImages.
Accessing rawImages with an invalid selectedFilterIndex will cause an exception. Please validate the index before using it.
</issue_to_address>
### Comment 4
<location> `lib/image_library/services/image_save_handler.dart:31` </location>
<code_context>
+
+ img.Image finalImg = rawImages[selectedFilterIndex];
+
+ if (flipHorizontal) {
+ finalImg = img.flipHorizontal(finalImg);
+ }
+ if (flipVertical) {
+ finalImg = img.flipVertical(finalImg);
+ }
</code_context>
<issue_to_address>
Image flipping is applied in-place, which may affect other references.
Since flipHorizontal and flipVertical may mutate the original image, clone rawImages[selectedFilterIndex] before applying these transformations to avoid unintended side effects.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
_savedImages = []; | ||
for (String json in savedImagesJson) { |
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.
issue (bug_risk): Clearing _savedImages before loading may cause issues if loadSavedImages is called concurrently.
Concurrent calls to this method could lead to race conditions. Use synchronization (e.g., a lock) or prevent concurrent execution to avoid data inconsistency.
if (index < 0 || index >= processingMethods.length) return "Unknown"; | ||
return filterMap[processingMethods[index]] ?? "Unknown"; |
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 (bug_risk): Returning 'Unknown' for invalid filter index may hide bugs.
Consider logging a warning or raising an error in debug mode when an invalid index is encountered to help identify logic errors early.
if (index < 0 || index >= processingMethods.length) return "Unknown"; | |
return filterMap[processingMethods[index]] ?? "Unknown"; | |
if (index < 0 || index >= processingMethods.length) { | |
assert(() { | |
// This will only run in debug mode. | |
print('Warning: Invalid filter index $index encountered in getFilterName.'); | |
return true; | |
}()); | |
return "Unknown"; | |
} | |
return filterMap[processingMethods[index]] ?? "Unknown"; |
}) async { | ||
if (rawImages.isEmpty) return; | ||
|
||
img.Image finalImg = rawImages[selectedFilterIndex]; |
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.
issue (bug_risk): No bounds check for selectedFilterIndex in rawImages.
Accessing rawImages with an invalid selectedFilterIndex will cause an exception. Please validate the index before using it.
if (flipHorizontal) { | ||
finalImg = img.flipHorizontal(finalImg); | ||
} | ||
if (flipVertical) { |
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.
issue (bug_risk): Image flipping is applied in-place, which may affect other references.
Since flipHorizontal and flipVertical may mutate the original image, clone rawImages[selectedFilterIndex] before applying these transformations to avoid unintended side effects.
i marked this draft pr as as ready to review, @AsCress @kienvo @Vishveshwara please review it once. Here is the demo video: |
@AsCress @Dhruv1797 @kienvo , is it better saving the image with the filter like Floyd steinberg (which is what dhruv has done in this PR) or should it take the saved image to the filter screen to select a filter again? What are your thoughts guys ? 2nd option might be better for last minute changes or , if we are using it on a different display(different colors supported) where another filter might look good. |
@Vishveshwara In my opinion, it would be better to save the image with the applied filter. From my perspective, the purpose of the Image Library is to ensure that the image is ready for immediate transfer, ideally just one click away. If we only save the original image and require the user to go through the Filter screen again, it feels quite similar to simply importing an image from the device gallery |
For this i think If a different display requires a different filter, users can save separate images with filters tailored to each display. Since the display name is already shown in the bottom-right corner in ImageLibrary card, we could further enhance usability by adding filter options based on display type in the Image Library. Here like for different Displays GDEY037T03 and GDEY037Z03 : |
@Dhruv1797 |
Please resolve conflicts. |
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.
just add the strings in string_constants.dart, rest are fine.
@mariobehling I’ve resolved the conflicts. Please review it. |
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.
Looks Good To Me.
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.
3000+ changes, is this worth it? I'm more on the side of saving the original image. That would be much simpler.
@Dhruv1797 @Vishveshwara General suggestion: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/helping-others-review-your-changes. |
@kienvo @AsCress actually, most of the 3000+ changes are due to UI/UX improvements like batch delete, single delete, rename, search & filter management, showing metadata (editor, imported image, display compatibility, filter applied, size, resolution, format, dimensions, etc.), saved date. and various dialogs (delete, save, rename, delete-batch, single delete, image properties, etc.). The code is also modularized for better maintainability, which naturally adds more lines. If you prefer, we can simplify it by just saving the original image and redirecting to the filter screen, as you said. But for a better UI/UX experience, this increase in code is kind of unavoidable. Here is the demo video: screen-20250627-100629.1.mp4 |
|
…st even after uninstall and reinstall app.
Nice catch! @Jhalakupadhyay , i have update the saving logic and removed the dependencies on Now the images and metadata is persisting even if we uninstall and reinstall the app ! Here is the demo video For storing the images and metadata on internal storage and storage permission handling: WhatsApp.Video.2025-07-02.at.6.41.48.PM.mp4@Jhalakupadhyay @kienvo @AsCress @Vishveshwara please check it once. |
Fixes: #54
Introduce a library feature that allows users to store and manage all finalized images in one place. Add a dedicated transfer section to easily select and send any saved image directly to the ePaper device.
Summary by Sourcery
Introduce a dedicated Image Library feature to store, manage, preview, and transfer finalized images, and integrate it into the image editor for saving and selecting library items.
New Features:
Enhancements:
Build: