vorta icon indicating copy to clipboard operation
vorta copied to clipboard

Fix bug with incorrect renaming of archives after aborting edit

Open VandalByte opened this issue 9 months ago • 10 comments

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 EscKeyDelegate to 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_changed and cell_double_clicked to 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.

VandalByte avatar Mar 06 '25 04:03 VandalByte

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.

VandalByte avatar Mar 06 '25 05:03 VandalByte

Newly added changes:

  • Fixed the rename borg job not being run after archive name change (cell change).
  • Added a edit_delegate to check the editor close to avoid race condition.
  • Added a method on_editing_finished to facilitate the transition to cell_changed by avoiding race condition, where cell_changed was executed before the cell_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.

VandalByte avatar Mar 07 '25 15:03 VandalByte

It's ready in your opinion? Then I'll start testing it locally.

m3nu avatar Mar 07 '25 21:03 m3nu

@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.

VandalByte avatar Mar 08 '25 07:03 VandalByte

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 avatar Mar 10 '25 12:03 m3nu

@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?

VandalByte avatar Mar 10 '25 16:03 VandalByte

Never mind then. Was just an idea.

m3nu avatar Mar 10 '25 16:03 m3nu

@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.

VandalByte avatar Mar 10 '25 16:03 VandalByte

No need to tag me each time.

m3nu avatar Mar 10 '25 17:03 m3nu

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

VandalByte avatar Mar 11 '25 02:03 VandalByte