-
Notifications
You must be signed in to change notification settings - Fork 155
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
base: master
Are you sure you want to change the base?
Conversation
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:
|
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 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. simplescreenrecorder-2025-03-11_07.53.13.mp4 |
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 delegateEscKeyDelegate
to catch the ESC key press event when an item in table is being edited.cell_changed
andcell_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?
Types of changes
Checklist:
I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.