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

Multiselection of folders in sources tab #1871

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

SAMAD101
Copy link
Collaborator

@SAMAD101 SAMAD101 commented Dec 14, 2023

Description

In the source, multiple folders cant be selected, due to some limitations in QFileDialog .
Fixes #1869

Motivation and Context

It would be more convenient if one can select multiple directories in one go, not having to open add folder dialog window multiple time, saving previous time and clicks.

How Has This Been Tested?

By running them in my system. No actual tests edited so far.

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.

@real-yfprojects
Copy link
Collaborator

If it doesn't get to complicated go ahead!

@SAMAD101
Copy link
Collaborator Author

Committed the changes, multiselection and addition of directories to sources is working now.

@SAMAD101
Copy link
Collaborator Author

This needs some changes to the tests, will commit them soon.

@SAMAD101
Copy link
Collaborator Author

So apparently, some tests are failing due to the changes I've made to the source_add.
I'm not quite getting how this work:
src/vorta/tests/unit/test_source.py (line 40-41

mocker.patch('os.access', return_value=False)
 tab.source_add(want_folder=True)

We are getting a TypeError: 'MockFileDialog' object is not iterable'
So can you explain a bit how these tests work? @real-yfprojects

Comment on lines 182 to 183
dialog.exec()
return dialog.selectedFiles()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change that? Using the non-blocking open with a slot is probably better then using exec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change that? Using the non-blocking open with a slot is probably better then using exec.

For some reason that way was not working out, the directories (if selected multiple) were not getting added to the sources.

@real-yfprojects
Copy link
Collaborator

So can you explain a bit how these tests work?

Have a look at pytest fixtures.
The failing tests use the source_env fixture which monkeypatches (that is replaces) the choose_file_dialog method with a different implementation which is needed for the tests to work without user interaction.

@pytest.fixture
def choose_file_dialog(tmpdir):
class MockFileDialog:
def __init__(self, *args, **kwargs):
self.directory = kwargs.get('directory', None)
self.subdirectory = kwargs.get('subdirectory', None)
def open(self, func):
func()
def selectedFiles(self):
if self.subdirectory:
return [str(tmpdir.join(self.subdirectory))]
elif self.directory:
return [str(self.directory)]
else:
return [str(tmpdir)]
return MockFileDialog

This mocking function still returns a dialog while the code expects a list of selected files resulting in the exception.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Dec 21, 2023

@real-yfprojects now I understand how this tests, when the tab.source_add is called inside test_source_add_remove and also in other tests, choose_file_dialog 's mocking fixture is used instead of the real one. So, if I change the returning value of that fixture to simply a list of directories, which are now used by the receive function.
But for some reason this way is not working, Now its showing

>       paths = choose_file_dialog(self, msg, want_folder=want_folder)
E       TypeError: 'list' object is not callable

I'm quite lost in what I could be still missing atm.

@real-yfprojects
Copy link
Collaborator

So, if I change the returning value of that fixture to simply a list of directories

Before you change the fixture returned a function, now its a list. The return value of the fixture is available in tests using the fixture through the choose_file_dialog parameter. Before this parameter consequently contained a function that was then called. Now it contains a list. However you still try to call like the function that it contained before, raising an error.

@SAMAD101
Copy link
Collaborator Author

So, if I change the returning value of that fixture to simply a list of directories

Before you change the fixture returned a function, now its a list. The return value of the fixture is available in tests using the fixture through the choose_file_dialog parameter. Before this parameter consequently contained a function that was then called. Now it contains a list. However you still try to call like the function that it contained before, raising an error.

Thank you for the assistance @real-yfprojects

Finally done, multiselection working and tests pass now.
Here's a video:

untitled.mp4

@sayem314 sayem314 mentioned this pull request Feb 24, 2024
9 tasks
@sayem314
Copy link
Member

PR #1938 provides a more detailed implementation that accounts for different types of file dialog views (QListView and QTreeView).

@SAMAD101
Copy link
Collaborator Author

PR #1938 provides a more detailed implementation that accounts for different types of file dialog views (QListView and QTreeView).

Just checked it out, it does account for two different types of view.
But is that needed for this issue?
I've used the NativeDialog one, which is already in use for the purpose. Just by using a QTreeView, we can do this pretty easily in just 3 lines of code.

@m3nu m3nu self-assigned this Apr 6, 2024
@m3nu
Copy link
Contributor

m3nu commented Apr 8, 2024

This works to select multiple files.

For the folder dialog I get this error.

Of course having just one dialog for files and folders including multiple files and folders would be even better.

Screenshot 2024-04-08 at 12 53 15

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Apr 8, 2024

fixed this in this little commit.
However, the intended feature of this PR is not working anymore, perhaps due to some update to PyQT.
The QFileDialog doesn't seem to have any child of QTreeView or QListView.
I'll try to find a workaround.

@m3nu m3nu marked this pull request as draft October 18, 2024 09:45
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.

Multiple selection does not work for directories.
4 participants