feat: inline edit for archive renaming #1734

diivi · 2023-06-19T21:19:05Z

Description

This PR adds the ability to rename archives in the archive tab. When a user double-clicks on the name of an archive, they can edit the name. If the new name is valid, the archive is renamed. If the new name is invalid, an error message is displayed. This feature is implemented using the cellChanged signal of the QTableWidget. The rename_action method has been removed.

Related Issue

Part of #1723

Motivation and Context

Faster renaming of archives.

How Has This Been Tested?

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.

diivi · 2023-06-19T21:27:31Z

tests/test_archives.py

@@ -168,31 +168,3 @@ def test_archive_delete(qapp, qtbot, mocker, borg_json_output):
    qtbot.waitUntil(lambda: 'Archive deleted.' in main.progressText.text(), **pytest._wait_defaults)
    assert ArchiveModel.select().count() == 1
    assert tab.archiveTable.rowCount() == 1
-
-
-def test_archive_rename(qapp, qtbot, mocker, borg_json_output):


Will look at some docs online to add a new test here, according to the changes. Any help will also be appreciated!

I tested using simple double mouse click event and it works

pos = tab.archiveTable.visualRect(tab.archiveTable.model().index(0, 4)).center() qtbot.mouseClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos) qtbot.mouseDClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos)

This will trigger the double click and rename action. You can then send new name and enter key.

m3nu · 2023-06-23T11:56:45Z

Will review this after some of the other PRs are merged. Else you will have too many conflicts.

diivi · 2023-06-25T16:00:00Z

I tried this:

    pos = tab.archiveTable.visualRect(tab.archiveTable.model().index(0, 4)).center()
    qtbot.mouseClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos)
    qtbot.mouseDClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos)
    qtbot.keyClicks(tab.archiveTable.viewport().focusWidget(), new_archive_name)
    qtbot.keyClick(tab.archiveTable.viewport(), QtCore.Qt.Key.Key_Return)

    # Successful rename case
    print(tab.archiveTable.model().index(0, 4).data())
    print(tab.mountErrors.text())

but it did not work, it just printed the old name. Am I missing something?

real-yfprojects

Can you add back the rename button and the context menu action?

The KDE HIG states:

Don’t use context menus as the only way to access functionality. Every item in a context menu must be available via a method that is somehow visible by default

This also applies for this new feature. Inexperienced user might not discover that you can double click the entry. The old buttons should now enter the edit mode of the code cell.

diivi · 2023-06-27T15:34:00Z

Done.

jetchirag · 2023-06-27T17:57:04Z

@diivi It needs some time to update the model. Add this after sending Return key.

qtbot.waitUntil(lambda:  tab.archiveTable.model().index(0, 4).data() == new_archive_name, **pytest._wait_defaults)

diivi · 2023-06-27T18:03:52Z

Thanks, but I did that first and did not work. It just says

E pytestqt.exceptions.TimeoutError: waitUntil timed out in 20000 milliseconds

Complete test:

def test_archive_rename(qapp, qtbot, mocker, borg_json_output):
    main = qapp.main_window	
    tab = main.archiveTab	
    main.tabWidget.setCurrentIndex(3)	

    tab.populate_from_profile()	
    qtbot.waitUntil(lambda: tab.archiveTable.rowCount() == 2)	

    tab.archiveTable.selectRow(0)	
    new_archive_name = 'idf89d8f9d8fd98'	
    stdout, stderr = borg_json_output('rename')	
    popen_result = mocker.MagicMock(stdout=stdout, stderr=stderr, returncode=0)	
    mocker.patch.object(vorta.borg.borg_job, 'Popen', return_value=popen_result)

    pos = tab.archiveTable.visualRect(tab.archiveTable.model().index(0, 4)).center()
    qtbot.mouseClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos)
    qtbot.mouseDClick(tab.archiveTable.viewport(), QtCore.Qt.MouseButton.LeftButton, pos=pos)
    qtbot.keyClicks(tab.archiveTable.viewport().focusWidget(), new_archive_name)
    qtbot.keyClick(tab.archiveTable.viewport(), QtCore.Qt.Key.Key_Return)

    # Successful rename case
    qtbot.waitUntil(lambda:  tab.archiveTable.model().index(0, 4).data() == new_archive_name, **pytest._wait_defaults)

jetchirag · 2023-06-27T18:39:00Z

You missed focusWidget:

qtbot.keyClick(tab.archiveTable.viewport().focusWidget(), QtCore.Qt.Key.Key_Return)

+You can add main.show() to see where it's stuck in testing.

diivi · 2023-06-27T21:20:42Z

🤦🏼 it works now, also thanks for the man.show() tip, I was looking for something like that!

m3nu · 2023-06-28T11:04:11Z

Worked as expected locally and doesn't add much new code by reusing the existing function. Good job!

m3nu · 2023-06-28T11:28:01Z

Minor comment: Better to make a feature branch in your personal fork of Vorta's repo. Else it can get messy when merging from the master branch more than once.

I.e.: Make a new branch named e.g. inline-rename and merge that into the master branch in the main repo.

diivi · 2023-06-28T14:02:19Z

Yeah, I do that everytime but missed it for this PR🥲

real-yfprojects

While your solution will work after some bugs are eliminated, I think this would be the perfect opportunity for implementing a QTableModel for the archive list which would be more elegant and probably easier to maintain.

Further Information:
https://doc.qt.io/qt-6/model-view-programming.html
https://doc.qt.io/qt-6/modelview.html#2-5-the-minimal-editing-example

src/vorta/views/archive_tab.py

diivi · 2023-07-08T13:04:51Z

How much time will that take, and will I have to scrape this off completely? Also, I started this 20 days ago, if it was the perfect time to implement this you should've told me a little early (or mentioned it in the task in the first place) 😅

real-yfprojects · 2023-07-08T13:41:08Z

Also, I started this 20 days ago, if it was the perfect time to implement this you should've told me a little early (or mentioned it in the task in the first place)

You are right. However occupied with other things like mentoring my own contributor I didn't look into this PR in detail. Let's settle on the current approach then without a QTableModel.

…l isnt 4

m3nu · 2023-07-24T10:58:31Z

Clear implementation and works as expected.

Just noticed that the archive duration and archive size get lost when renaming and then doing a "Refresh Archives". Possible that some row in the DB isn't updated?

diivi · 2023-07-24T11:41:16Z

It was working the last time I checked, but now I can't understand why the Size and duration get lost :/

The error occurs here for me - https://github.com/diivi/vorta/blob/48204f8bfd087b0206421c515d3f11bdc803ddaa/src/vorta/borg/info_archive.py#L46

  File "/home/divyansh/Desktop/vorta/src/vorta/borg/info_archive.py", line 46, in process_result
    archive.name = remote_archive['name']  # incase name changed
AttributeError: 'NoneType' object has no attribute 'name'

diivi · 2023-07-24T11:47:09Z

I deleted the database and tested this again, it worked.

Maybe some migration is missing? Can someone with the old database test it and let me know, I'm a bit confused.

src/vorta/views/archive_tab.py

bigtedde · 2023-07-26T18:50:41Z

It was working the last time I checked, but now I can't understand why the Size and duration get lost :/

The error occurs here for me - https://github.com/diivi/vorta/blob/48204f8bfd087b0206421c515d3f11bdc803ddaa/src/vorta/borg/info_archive.py#L46
  File "/home/divyansh/Desktop/vorta/src/vorta/borg/info_archive.py", line 46, in process_result
    archive.name = remote_archive['name']  # incase name changed
AttributeError: 'NoneType' object has no attribute 'name'

This error occurs during a "recalculate" archive action done after a name change. Separately, I am seeing the size and duration information get lost during the "refresh" archive action.

diivi · 2023-07-28T05:11:06Z

Figured out what the issue is. The Borg rename command changes the archive's ID too during renaming, so I need to update that here - https://github.com/diivi/vorta/blob/48204f8bfd087b0206421c515d3f11bdc803ddaa/src/vorta/borg/rename.py#L37. How can I get the archive's ID after the rename operation though?

real-yfprojects · 2023-07-28T07:46:35Z

How can I get the archive's ID after the rename operation though?

Since you know the new name I would think you can run borg info or borg list with the new name. I don't know which of these is faster in this scenario.

src/vorta/borg/info_archive.py

src/vorta/views/archive_tab.py

diivi · 2023-08-02T09:47:15Z

1ac4240 Changed the code so that the name reverts to the original one if the new name is already taken.
Also updated the snapshot_id in the if block.

real-yfprojects · 2023-08-05T14:02:24Z

This might solve the bug, where duration and size is lost when refreshing the archive list.

This is still the case. I think it is linked to snapshot_id.

Steps to reproduce

Rename archive
Refresh archive list
See that the archive appears but the columns size, duration and trigger are empty.

This doesn't happen when recalculating the archive size of the renamed archive between steps 1 and 2.

diivi · 2023-08-05T14:22:20Z

So should I start an info_archive job everytime the rename job finishes? To update the snapshot id for the archive in the database

real-yfprojects · 2023-08-05T14:35:50Z

So should I start an info_archive job everytime the rename job finishes? To update the snapshot id for the archive in the database

If there is no other way to deal with this issue, I would accept running borg once more.

diivi · 2023-08-05T14:45:37Z

I don't think there is - https://borgbackup.readthedocs.io/en/stable/usage/list.html

check Keys available only when listing files in an archive:

I can only get the dedup size and duration by listing the archive that was renamed 😞

diivi · 2023-08-05T15:11:20Z

@real-yfprojects I added a call to refresh_archive_info after an archive is reamed, now I can't reproduce the error, seems like it's fixed.
Please check and let me know too.

m3nu

Works as expected, including keeping stats.

diivi added 2 commits June 20, 2023 02:25

feat: inline edit for archive renaming

026ced4

remove old renaming test

eded8be

diivi mentioned this pull request Jun 19, 2023

feat: refresh multiple archives #1723

Merged

14 tasks

diivi commented Jun 19, 2023

View reviewed changes

m3nu self-assigned this Jun 23, 2023

Merge branch 'borgbase:master' into master

970c517

real-yfprojects reviewed Jun 27, 2023

View reviewed changes

bring back context menu and rename button

cd51dae

diivi and others added 2 commits June 28, 2023 02:50

add rename test

e0e3efa

Merge branch 'master' into master

0597769

m3nu previously approved these changes Jun 28, 2023

View reviewed changes

m3nu requested a review from real-yfprojects June 28, 2023 11:25

Merge branch 'master' into master

93f3d33

diivi dismissed m3nu’s stale review via 93f3d33 July 6, 2023 06:03

diivi requested a review from m3nu July 6, 2023 06:03

real-yfprojects reviewed Jul 8, 2023

View reviewed changes

src/vorta/views/archive_tab.py Outdated Show resolved Hide resolved

src/vorta/views/archive_tab.py Show resolved Hide resolved

src/vorta/views/archive_tab.py Show resolved Hide resolved

diivi added 2 commits July 11, 2023 19:33

undo archive rename if blank. fix typo. early return when changed cel…

2de4b3f

…l isnt 4

merge with master

31ef41a

typo

48204f8

diivi requested a review from real-yfprojects July 14, 2023 11:55

bigtedde reviewed Jul 26, 2023

View reviewed changes

src/vorta/views/archive_tab.py Show resolved Hide resolved

fix archive rename id change bug

52adf31

real-yfprojects reviewed Aug 2, 2023

View reviewed changes

src/vorta/borg/info_archive.py Show resolved Hide resolved

src/vorta/views/archive_tab.py Show resolved Hide resolved

use original name when new name already exists

1ac4240

refresh archive info on rename

b25957f

diivi closed this Aug 5, 2023

diivi force-pushed the master branch from b25957f to b015368 Compare August 5, 2023 15:07

Merge branch 'master' of https://github.com/diivi/vorta

b53a21b

diivi reopened this Aug 5, 2023

fix tests

01a4f62

m3nu approved these changes Aug 9, 2023

View reviewed changes

Merge branch 'master' into master

623010f

m3nu merged commit 30c5722 into borgbase:master Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: inline edit for archive renaming #1734

feat: inline edit for archive renaming #1734

diivi commented Jun 19, 2023

diivi Jun 19, 2023

jetchirag Jun 20, 2023

diivi Jun 20, 2023

m3nu commented Jun 23, 2023

diivi commented Jun 25, 2023

real-yfprojects left a comment

diivi commented Jun 27, 2023

jetchirag commented Jun 27, 2023

diivi commented Jun 27, 2023 •

edited

Loading

jetchirag commented Jun 27, 2023

diivi commented Jun 27, 2023

m3nu commented Jun 28, 2023

m3nu commented Jun 28, 2023 •

edited

Loading

diivi commented Jun 28, 2023

real-yfprojects left a comment •

edited

Loading

diivi commented Jul 8, 2023

real-yfprojects commented Jul 8, 2023

m3nu commented Jul 24, 2023

diivi commented Jul 24, 2023

diivi commented Jul 24, 2023

bigtedde commented Jul 26, 2023

diivi commented Jul 28, 2023

real-yfprojects commented Jul 28, 2023 •

edited

Loading

diivi commented Aug 2, 2023 •

edited

Loading

real-yfprojects commented Aug 5, 2023

diivi commented Aug 5, 2023

real-yfprojects commented Aug 5, 2023

diivi commented Aug 5, 2023

diivi commented Aug 5, 2023

m3nu left a comment

feat: inline edit for archive renaming #1734

feat: inline edit for archive renaming #1734

Conversation

diivi commented Jun 19, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

diivi Jun 19, 2023

Choose a reason for hiding this comment

jetchirag Jun 20, 2023

Choose a reason for hiding this comment

diivi Jun 20, 2023

Choose a reason for hiding this comment

m3nu commented Jun 23, 2023

diivi commented Jun 25, 2023

real-yfprojects left a comment

Choose a reason for hiding this comment

diivi commented Jun 27, 2023

jetchirag commented Jun 27, 2023

diivi commented Jun 27, 2023 • edited Loading

jetchirag commented Jun 27, 2023

diivi commented Jun 27, 2023

m3nu commented Jun 28, 2023

m3nu commented Jun 28, 2023 • edited Loading

diivi commented Jun 28, 2023

real-yfprojects left a comment • edited Loading

Choose a reason for hiding this comment

diivi commented Jul 8, 2023

real-yfprojects commented Jul 8, 2023

m3nu commented Jul 24, 2023

diivi commented Jul 24, 2023

diivi commented Jul 24, 2023

bigtedde commented Jul 26, 2023

diivi commented Jul 28, 2023

real-yfprojects commented Jul 28, 2023 • edited Loading

diivi commented Aug 2, 2023 • edited Loading

real-yfprojects commented Aug 5, 2023

Steps to reproduce

diivi commented Aug 5, 2023

real-yfprojects commented Aug 5, 2023

diivi commented Aug 5, 2023

diivi commented Aug 5, 2023

m3nu left a comment

Choose a reason for hiding this comment

diivi commented Jun 27, 2023 •

edited

Loading

m3nu commented Jun 28, 2023 •

edited

Loading

real-yfprojects left a comment •

edited

Loading

real-yfprojects commented Jul 28, 2023 •

edited

Loading

diivi commented Aug 2, 2023 •

edited

Loading