Skip to content
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

Fix bug with incorrect renaming of archives after aborting edit #2213

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

VandalByte
Copy link
Contributor

@VandalByte VandalByte commented Mar 6, 2025

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

  • 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:

  • I have read the CONTRIBUTING guide.
  • 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
Copy link
Contributor Author

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.

@m3nu m3nu marked this pull request as draft March 6, 2025 07:21
@VandalByte
Copy link
Contributor Author

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.

@m3nu
Copy link
Contributor

m3nu commented Mar 7, 2025

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

@VandalByte VandalByte marked this pull request as ready for review March 8, 2025 07:18
@VandalByte
Copy link
Contributor Author

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

@m3nu
Copy link
Contributor

m3nu commented Mar 10, 2025

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
m3nu previously approved these changes Mar 10, 2025
@VandalByte
Copy link
Contributor Author

VandalByte commented Mar 10, 2025

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

@m3nu
Copy link
Contributor

m3nu commented Mar 10, 2025

Never mind then. Was just an idea.

@VandalByte
Copy link
Contributor Author

VandalByte commented Mar 10, 2025

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

@m3nu
Copy link
Contributor

m3nu commented Mar 10, 2025

No need to tag me each time.

@VandalByte
Copy link
Contributor Author

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.

simplescreenrecorder-2025-03-11_07.53.13.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vorta displays the same name for all archives
2 participants