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

Profile list in the main window are not kept sorted when adding/renaming entries #1983

Closed
Parnassius opened this issue Apr 6, 2024 · 2 comments · Fixed by #1986
Closed
Labels
type:enhancement Improvement of an existing function

Comments

@Parnassius
Copy link
Contributor

Description

Creating a new profile adds the new profile at the bottom of the list in the main window. Similarly, editing an existing profile doesn't change its position in the list. I think that in both cases the list should be kept sorted. Restarting Vorta correctly updates the positions of the profiles to be sorted alphabetically.

This was recently noticed during the review of #1899 (comment)

Steps to reproduce the behavior:

  1. Create a new profile using the menu in the main window
  2. The profile is added last on the list, even if it should sort before other entries

Alternatively:

  1. Rename an already existing profile
  2. Its position in the list doesn't change, even if the new alphabetical ordering is no longer valid

Environment

  • OS: Linux
  • Vorta version: latest commit (9b8dbce)
  • Installed from: pip
  • Borg version: N/A

I see two possible solutions for this:

  1. Add some additional handling in the profile_add_edit_result function to keep the list sorted. This is a quick commit that does that: Parnassius@59661f9

  2. Use the sortItems method of QListWidget to sort the list after every update, thus removing the need to use the order_by sql method.

The two solutions cannot co-exist since the sql ordering is case-insensitive while QListWidget.sortItems is case-sensitive. In the second case additional care should be taken to ensure the use of sortItems doesn't break anything. From a quick glance I think the way current_profile is set during the initialization of the main window should be updated, and maybe some additional changes are needed as well.

self.current_profile = BackupProfileModel.get_or_none(id=prev_profile_id.str_value)
if self.current_profile is None:
self.current_profile = BackupProfileModel.select().order_by('name').first()

What do you think? Let me know if I should create a small pull request with the commit linked above or if a solution based on option 2 should be preferred

@m3nu
Copy link
Contributor

m3nu commented Apr 7, 2024

Yeah, we can make this change. Thanks for even considering a solution.

Regarding the 2 options: Choose the one that's easier to understand later and adds less code to the project.

@m3nu m3nu added the type:enhancement Improvement of an existing function label Apr 7, 2024
@Parnassius
Copy link
Contributor Author

Sure, in that case I've created #1986 with option 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Improvement of an existing function
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants