vorta
vorta copied to clipboard
Fix bug with incorrect renaming of archives after aborting edit
Description
This PR fixes a bug where renaming an archive and then canceling the operation by pressing the ESC key caused subsequent actions (Extract, Diff, etc.) to rename all archives in the list to the attempted name in UI level.
Changes made:
- ~~Created a delegate
EscKeyDelegateto catch the ESC key press event when an item in table is being edited.~~
Attaching event filter directly to archive table did not work because while editing events are then handled by the editor widget, not the viewport.
- Introduced a flag in
cell_changedandcell_double_clickedto track whether the edit was completed or canceled.
Related Issue
Fixes #2041
Motivation and Context
This issue was originally reported by romlok, where canceling the renaming of an archive by pressing the ESC key caused all archives to be incorrectly renamed. The bug affects operations such as extracting and diffing, resulting in incorrect archive names and exceptions due to non-unique keys.
How Has This Been Tested?
- Tested the UI by running the build and verifying the fix.
- Tested all the buttons that previously triggered the behavior, ensuring the issue was resolved.
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Checklist:
- [x] I have read the CONTRIBUTING guide.
- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.
I just encountered another bug, renaming the archive now doesn't rename it in the backend and logs that archive not found (new archive name) and refreshes the list to previous state, I'll update the PR once I fix it.
Newly added changes:
- Fixed the rename borg job not being run after archive name change (cell change).
- Added a
edit_delegateto check the editor close to avoid race condition. - Added a method
on_editing_finishedto facilitate the transition tocell_changedby avoiding race condition, wherecell_changedwas executed before thecell_double_clicked. - Fixed the archive name not returning to original name when faced with a blank value.
- Removed ESC delegate as it was not doing anything.
It's ready in your opinion? Then I'll start testing it locally.
@m3nu yeah it's ready for review. I was looking at ways to avoid using the global variable, but doing so would require significant changes to existing methods and might lead to potential issues down the line. So, I don't think that going to be the best option here.
Works as expected locally.
If you want, I saw one related thing that could be improved in the same feature: It says "Task started" when renaming. If this code is specific to renaming, why not change this to "Renaming archive..." (the 3 dots should be used for stuff that's ongoing)
@m3nu I have set the status accordingly but it's visible only for a split second then it shows "Task started" then the following statuses. One way I found is to suppress the text in the _set_status like this:
def _set_status(self, text):
if text == "Task started":
return
self.mountErrors.setText(text)
self.mountErrors.repaint()
From the tests I did, this method doesn't seem to affect any other status messages. I haven't included this change in the latest commit. Should I add this also or keep it the same as of last commit?
Never mind then. Was just an idea.
@m3nu adding this change I mentioned doesn't seem to affect any existing functionalities as of now. If we are not adding the change, then the rest of the PR is ready to be merged.
No need to tag me each time.
I tried adding the status update for rename operation in the bottom one as well (since it was missing), which apparently fixed the issue with the last one (above mentioned). So it's all good now.
https://github.com/user-attachments/assets/919d83d0-9517-4ce4-b308-d1bbe8b5c218