-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
Batch move #1188
base: main
Are you sure you want to change the base?
Batch move #1188
Conversation
Add new method override for MoveBetweenFolders that takes a List<CheckpointFileViewModel> and performs a bulk move of all files before performing RefreshIndex
When DragMovesAllSelected is true and allSelectedCheckpoints has checkpoints perform a bulk move using the new MoveBetweenFolders method to avoid refreshing the model index after every move operation
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.
Hello @RTHilton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a new method, MoveBetweenFolders
, to handle the movement of multiple model files between directories. This is intended to prevent the model cache from being refreshed after each individual move, improving efficiency. The changes primarily involve adding this new method to the CheckpointsPageViewModel
and updating the CheckpointsPage
view to utilize it when moving multiple selected checkpoints.
Highlights
- Batch Model Moving: A new
MoveBetweenFolders
method is added toCheckpointsPageViewModel
to move multiple model files at once. - UI Integration: The
CheckpointsPage
view is updated to use the new batch move method when handling drag-and-drop operations for multiple selected checkpoints. - Error Handling: The new method includes error handling for invalid folder selections and file transfer conflicts (e.g., destination file already exists).
Changelog
- StabilityMatrix.Avalonia/ViewModels/CheckpointsPageViewModel.cs
- Added a new
MoveBetweenFolders
method that accepts a list ofCheckpointFileViewModel
objects and a destination folder. - Implemented logic to iterate through the source files, move the model file, its associated
.cm-info.json
file, and its preview image. - Added checks to prevent moving files to the same directory or the models directory.
- Implemented error handling for file transfer exceptions, specifically
FileTransferExistsException
. - Refreshes the model index and clears progress after the move operation.
- The original
MoveBetweenFolders
method is still present, but now handles a singleLocalModelFile
.
- Added a new
- StabilityMatrix.Avalonia/Views/CheckpointsPage.axaml.cs
- Modified the
OnDrop
method to call the newMoveBetweenFolders
method with a list of selected checkpoints instead of iterating and calling the single-file move method. - Removed the loop that iterated through the selected checkpoints.
- Modified the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In folders deep, where models reside,
A batch move speeds, with code as our guide.
No more slow crawls, each file on its own,
Now swift migrations, efficiently sown.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
CLA Assistant Lite bot CLA Assistant bot All Contributors have signed the CLA. |
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.
Code Review
This pull request introduces a new method for moving multiple models at once, which is a great optimization to prevent excessive model cache refreshes. The code looks well-structured, but I have a few suggestions to improve error handling and code clarity.
Summary of Findings
- Redundant condition: The condition
destinationFolder.FullPath == settingsManager.ModelsDirectory || (sourceDirectory != null && sourceDirectory == destinationFolder.FullPath)
can be simplified by checking if the source and destination directories are the same, regardless of whether the destination is the models directory. - Missing error handling: The code does not handle the scenario where
Path.GetDirectoryName
returns null, which could lead to unexpected behavior. - Inconsistent Notification Messages: The notification message in the catch block is generic and does not provide specific information about the file that failed to move.
Merge Readiness
The pull request is almost ready for merging. However, I recommend addressing the identified issues, especially the redundant condition and the error handling improvements, before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
{ | ||
if (sourceFiles != null && sourceFiles.Count() > 0) | ||
{ | ||
var sourceDirectory = Path.GetDirectoryName(sourceFiles[0].CheckpointFile.GetFullPath(settingsManager.ModelsDirectory)); |
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.
Consider adding a null check for the result of Path.GetDirectoryName
to handle cases where the path is a root directory or an invalid path. This will prevent potential NullReferenceException
when accessing the CheckpointFile
property.
var sourceDirectory = sourceFiles[0].CheckpointFile != null ? Path.GetDirectoryName(sourceFiles[0].CheckpointFile.GetFullPath(settingsManager.ModelsDirectory)) : null;
if ( | ||
destinationFolder.FullPath == settingsManager.ModelsDirectory | ||
|| (sourceDirectory != null && sourceDirectory == destinationFolder.FullPath) |
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.
This condition checks if the destination folder is the same as the models directory or the source directory. The check destinationFolder.FullPath == settingsManager.ModelsDirectory
seems redundant because the second part of the condition already covers the case where the source and destination are the same. Consider simplifying this to just check if the source and destination directories are equal.
Also, it would be good to add a null check for sourceDirectory
before accessing FullPath
to avoid potential NullReferenceException
.
if (
sourceDirectory == null || destinationFolder.FullPath == sourceDirectory
)
{ | ||
notificationService.Show( | ||
"Failed to move file", | ||
"Destination file exists", | ||
NotificationType.Error | ||
); |
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.
The notification message in the catch block is generic. Consider including the file name in the notification to provide more context to the user about which file failed to move.
notificationService.Show(
"Failed to move file",
$"Failed to move '{sourcePath.Name}': Destination file exists",
NotificationType.Error
);
I have read the CLA Document and I hereby sign the CLA |
recheck |
Add a new method for moving multiple models at the same time to prevent the model cache from getting refreshed after each move