-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[UI] Create a new file manager to handle save as new version #2951
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: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2951 +/- ##
===========================================
- Coverage 79.80% 79.75% -0.05%
===========================================
Files 64 64
Lines 8674 8684 +10
===========================================
+ Hits 6922 6926 +4
- Misses 1752 1758 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4ebff46 to
d30cf11
Compare
d30cf11 to
5aba40d
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
|
||
| // Custom title bar | ||
| // Could be used to add options and menus | ||
| // For now only the name of the mode (Opn/Save and a close UI button) |
Copilot
AI
Jan 14, 2026
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.
There's a typo in this comment. "Opn" should be "Open".
| // For now only the name of the mode (Opn/Save and a close UI button) | |
| // For now only the name of the mode (Open/Save and a close UI button) |
| // onRejected: { | ||
| // console.log("File not saved") | ||
| // } |
Copilot
AI
Jan 14, 2026
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 commented-out code should be removed if it's not needed. Keeping commented-out code reduces maintainability.
| // onRejected: { | |
| // console.log("File not saved") | |
| // } |
| // Split filename and extension | ||
| var lastDotIndex = filename.lastIndexOf(".") | ||
| var baseName = "" | ||
| var extension = "" | ||
|
|
||
| if (lastDotIndex > 0) { | ||
| baseName = filename.substring(0, lastDotIndex) | ||
| extension = filename.substring(lastDotIndex) | ||
| } else { | ||
| baseName = filename | ||
| } |
Copilot
AI
Jan 14, 2026
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.
When a file doesn't have an extension (lastDotIndex <= 0), the extension variable remains empty but is still concatenated. While this works correctly, the condition should be lastDotIndex > 0 (which it is), but the comment or logic could be clearer about this edge case. The current implementation is correct but could benefit from a comment explaining the handling of files without extensions.
| } | ||
|
|
||
| Label { | ||
| text: model.fileIsDir ? "" : (model.fileSize / 1024).toFixed(1) + " KB" |
Copilot
AI
Jan 14, 2026
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 file size is always displayed in KB (kilobytes), which may not be appropriate for very large files. Consider implementing dynamic units (KB, MB, GB) based on file size for better readability. For example, files larger than 1024 KB could be shown in MB.
| @Slot(result=bool) | ||
| def isDarkPalette(self): | ||
| return QApplication.instance().palette() == self.darkPalette |
Copilot
AI
Jan 14, 2026
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 isDarkPalette method uses equality comparison (==) to check if the current palette is the dark palette. This comparison may not work as expected since QPalette objects may not be comparable with ==. Consider using a dedicated boolean flag to track which palette is active, or compare specific palette properties instead.
| color: Qt.lighter(root.activePalette.base, 1.4) | ||
|
|
||
| function getSelectedPath() { | ||
| return folderModel.folder.toString().replace("file://", "") + "/" + filenameField.text |
Copilot
AI
Jan 14, 2026
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 path concatenation using + "/" + could result in double slashes or incorrect paths if folderModel.folder already ends with a slash or if filenameField.text is empty. Consider using proper path joining utilities or handling edge cases explicitly to ensure correct path construction.
| return folderModel.folder.toString().replace("file://", "") + "/" + filenameField.text | |
| var basePath = folderModel.folder.toString().replace("file://", ""); | |
| // Remove trailing slash from basePath, if any | |
| if (basePath.length > 1 && basePath[basePath.length - 1] === "/") { | |
| basePath = basePath.slice(0, -1); | |
| } | |
| var fileName = filenameField.text; | |
| if (!fileName) { | |
| return basePath; | |
| } | |
| return basePath + "/" + fileName; |
| property string fileToSave: "" | ||
|
|
||
| onFileSelected: (path) => { | ||
| fileToSave = path.toString().replace("file://", "") | ||
| // Do something with the file path | ||
| } | ||
|
|
||
| onAccepted: { | ||
| if (!validateFilepathForSave(fileToSave, newSaveFileDialog)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Only save a valid file | ||
| _reconstruction.saveAs("file://" + fileToSave) |
Copilot
AI
Jan 14, 2026
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 variable 'fileToSave' is declared as a property but its value assignment in the onFileSelected handler appears redundant since the same path is already captured in the 'path' parameter and used in onAccepted. The fileToSave property could be removed and the path parameter from fileSelected could be used directly, or if it serves a specific purpose, that should be documented.
| property string fileToSave: "" | |
| onFileSelected: (path) => { | |
| fileToSave = path.toString().replace("file://", "") | |
| // Do something with the file path | |
| } | |
| onAccepted: { | |
| if (!validateFilepathForSave(fileToSave, newSaveFileDialog)) | |
| { | |
| return; | |
| } | |
| // Only save a valid file | |
| _reconstruction.saveAs("file://" + fileToSave) | |
| onAccepted: (path) => { | |
| let fileToSave = path.toString().replace("file://", "") | |
| if (!validateFilepathForSave(fileToSave, newSaveFileDialog)) | |
| { | |
| return; | |
| } | |
| // Only save a valid file | |
| _reconstruction.saveAs(path.toString()) |
| function getSelectedPath() { | ||
| return folderModel.folder.toString().replace("file://", "") + "/" + filenameField.text |
Copilot
AI
Jan 14, 2026
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.
Multiple places in this file use .replace("file://", "") to convert URLs to paths. This pattern is repeated throughout (lines 279, 303, 433, 522, 628). Consider creating a utility function to handle this conversion consistently and correctly, which would improve maintainability and reduce code duplication.
| function getSelectedPath() { | |
| return folderModel.folder.toString().replace("file://", "") + "/" + filenameField.text | |
| function fileUrlToPath(url) { | |
| return url.toString().replace("file://", "") | |
| } | |
| function getSelectedPath() { | |
| return fileUrlToPath(folderModel.folder) + "/" + filenameField.text |
meshroom/ui/qml/Application.qml
Outdated
| } | ||
| } | ||
| Action { | ||
| id: newSaveAsAction |
Copilot
AI
Jan 14, 2026
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 action name "newSaveAsAction" is somewhat ambiguous. The "new" prefix suggests creating something new, but this is actually a "Save As" action that allows saving with version incrementing. Consider renaming to something more descriptive like "saveAsWithVersionAction" or "saveAsNewVersionAction" to better reflect its purpose.
| id: newSaveAsAction | |
| id: saveAsNewVersionAction |
| onClicked: { | ||
| var url = folderModel.folder.toString() | ||
| var parentPath = url.substring(0, url.lastIndexOf("/")) | ||
| if (parentPath.length > 7) // more than "file://" | ||
| folderModel.customFolder = parentPath | ||
| } |
Copilot
AI
Jan 14, 2026
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.
When the "Go up one level" button is clicked and the parent path length check fails (line 425), the operation silently does nothing. Consider providing user feedback (e.g., disabling the button or showing a tooltip) when at the root directory to indicate why navigation up is not possible.
a1d8fd0 to
7cf7035
Compare
7cf7035 to
44b26e4
Compare
Why this PR ?
I was mainly inspired by Blender save UI :

Description of the content
Create a new file manager :
Next steps
If reviewers are okay with this proposal there's a few changes to add still :