[ENH] Added dropdown to select drives and to select files from the tree view while selecting location of new project

[sumana-2705]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?
Currently, SelectDirectoryTreeScreen starts at Path.home() by default, which is not ideal for selecting external drives. Windows and some Linux systems do not have a master root where all drives are listed, making drive selection difficult. This PR introduces a dropdown to allow users to choose the drive they want to navigate.

What does this PR do?

• Adds a dropdown to select available drives while choosing location for new project.
• Retrieves a list of mounted drives dynamically.
• Ensures cross-platform compatibility for Windows and Linux.

References

fixes #448

How has this PR been tested?

I have tested it my windows laptop, the dropdown successfully worked. But the directory tree is not getting refreshed, I need your help to fix this. I am attaching the current state of the window:

datashuttle.mp4

Is this a breaking change?

No, this PR does not break existing functionality. It only enhances the directory selection process by allowing users to switch between drives.

Does this PR require an update to the documentation?

Yes, documentation should be updated to mention the new drive selection feature in SelectDirectoryTreeScreen.

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[sumana-2705]

Hello @JoeZiminski @adamltyson

I have tried to solve the issue #448 but got stuck in the process. As shown in the video, I have added a dropdown to select drives, the dropdown is successfully showing the available files. But the tree view is not updating the files according to the drive selected. Can you please help me how to refine the code to correctly add this feature. Thank you!!!

[JoeZiminski]

@sumana-2705 thanks for this! This is a really nice addition and will be very useful for users, please see comments above.

I can take a look at the linux / macOS situation next week and will let you know about that ASAP (feel free to look into this also if you have access to these systems). It is possible to mount external drives on macOS / linux and so it will be nice to support these also if possible.

We will also need to add a test for this, unfortunately it is not a simple thing to test. I think it will be impossible to actually test the behaviour on real drives (we at the NIU might have to add this to our internal tests). However, we can at least test the interface. I think the approach will be:

1. monkeypatch the functions that get the drives names to return a) an actual drive on the system b) some other made up drive.
2. select these different drives and check the tree updates
3. accept and check the input boxes are filled as expected once the new drive is chosen.

You can take a look at this function on the test suite for inspiration. However this is quite advanced so happy to give more detail and an example once this round of comments have been responded too. Thanks again!

[sumana-2705]

Thank you for providing the reference and background, @JoeZiminski. Unfortunately, most of my peers at university use Windows, so I currently don’t have access to Mac or Linux systems. For now, I will focus on understanding and implementing monkeypatch. I will provide an update as soon as possible. Once it is working properly, we can test it on different systems and refine the tests accordingly.

[JoeZiminski]

Sounds good! Thanks @sumana-2705 I'll get back to you ASAP, let me know if you have any questions about the monkeypatching.

[sumana-2705]

Hello @JoeZiminski @adamltyson

Before adding tests using monkeypatch, I wanted to ensure that the TUI correctly updates the tree based on the selected drive on my local machine. I have included a few functions, but I am having trouble identifying which function is causing the issue that prevents the tree from updating as expected. Is there a way to open a console and inspect the TUI, similar to how we do on a website, to gain more clarity? Also I want to know what is the process of deleting the existing projects. Thank you

[JoeZiminski]

HI @sumana-2705, I'm very sorry when I last reviewed your PR I did not click the submit button, I thought I had in the process of posting the summary. Please see some suggested changes, apologies for this oversight.

That's a good question, I think textual has a pilot.app.save_screenshot() function, this will let you see the current state of the TUI in the text environment by saving a .png image to the current working directory. To delete a project, currently you need to use the project.get_configs_path() to find where the datahuttle projects are saved on your machine, then delete the folder. In future we would like to make this more convenient. let me know if you have any questions!

[sumana-2705]

HI @sumana-2705, I'm very sorry when I last reviewed your PR I did not click the submit button, I thought I had in the process of posting the summary. Please see some suggested changes, apologies for this oversight.

That's a good question, I think textual has a pilot.app.save_screenshot() function, this will let you see the current state of the TUI in the text environment by saving a .png image to the current working directory. To delete a project, currently you need to use the project.get_configs_path() to find where the datahuttle projects are saved on your machine, then delete the folder. In future we would like to make this more convenient. let me know if you have any questions!

Thank you for the suggestions @JoeZiminski I will go through them clearly and work on them

[sumana-2705]

Thank you @JoeZiminski all the suggestions worked like a magic. Now everything is perfect, I tried so hard to correct this issue but it was not updating properly even after so many attempts. You helped me a lot by suggesting changes. Thank you once again. Also I wanted to conform that whether test_tui_directorytree.py is the correct file to add tests?

Untitled.video.-.Made.with.Clipchamp.mp4

[JoeZiminski]

Hi @sumana-2705 great glad it helped! Yes textual can be hard to work with at first because it is a huge package and there are a lot of different ways to do similar things. Yes test_tui_directorytree is the right place, let me know if you have any questions!

[JoeZiminski]

BTW I did not have time in the end this week to test on the macOS in the office, but I will do next week. In terms of working on this PR don't worry much about it, it should just be a quick check to find what the mount paths are and setting appropriately.

[sumana-2705]

BTW I did not have time in the end this week to test on the macOS in the office, but I will do next week. In terms of working on this PR don't worry much about it, it should just be a quick check to find what the mount paths are and setting appropriately.

Got it! It took me some time since I'm new to TUI development, but I'm gaining valuable experience while working on these issues. I'll also try to check this on a Mac in the meantime. Thanks for your support!

[sumana-2705]

Hello @JoeZiminski

I have checked the behavior of the directory tree on Mac and modified the code to ensure the functionalities work correctly on both Windows and Mac. Here is a picture of the directory tree on Mac for reference.

[JoeZiminski]

HI @sumana-2705 thanks again for this PR, I tested on linux and it works perfectly! Did you have a chance to play around with the tests? I'm happy to finish this off and add a few before merging if things are busy at the moment.

[JoeZiminski]

Hey @sumana-2705 I just had a very small suggestions

[sumana-2705]

Thanks for your review @JoeZiminski Sure I'll make changes as early as possible. I was inactive for a few days due to my thesis work and end semester exams. Thanks again for the suggestions 😊

[JoeZiminski]

Hey @sumana-2705 thanks, it seems that first issue is because:

            Select(  # Dropdown for drives
                [(drive, drive) for drive in self.get_drives()],
                value=self.selected_drive,
                allow_blank=False,
                id="select_directory_tree_drive_select",
            ),

since self.get_drives() is monkeypatched, but self.selected_drive is still using the real drive, then the selected drive is not in the list of drives and it's causing the error. I think the easiest solution is to make a function self.selected_drive = self.get_selected_drive() and move the conditional that sets the selected drive into that function. Then that function can be monkeypatched as well and you can set it to the actual_drive.

Yes textual.pilot.OutOfBounds: Target offset is outside of currently-visible screen region is a really annoying textual issue that I thought was solved but must not be on all systems. For now, please ignore those failures and we can work through them on the CI. At present the CI is failing because of #522 and also it makes sense to wait until #520 is merged. Then hopefully that test issue will resolve itself. I'll let you know ASAP when these are merged!

[JoeZiminski]

Hey @sumana-2705 this is coming along nicely, the test is structured well and attacks the problem nicely. I've suggested some changes please let me know what you think. BTW just for your interest in case it is useful to see, the steps I took to solve the bug with the textual.widgets._select.InvalidSelectValueError: Illegal select value 'C:\\' was:

1. The stack trace from textual is quite cryptic, but sort-of indicated it was a problem with the set up of the select in the compose function

2. went to the definition of the select widget in the compose function and added a breakpoint. These are useful for stopping execution of the program so you can see the state of the variables:

       breakpoint()
        yield Container(
            Static(label_message, id="select_directory_tree_screen_label"),
            Select(  # Dropdown for drives
                [(drive, drive) for drive in self.get_drives()],
                value=self.selected_drive,
                allow_blank=False,
                id="select_directory_tree_drive_select",
            ),

Then I played around with the inputs to the Select until it was clear that the value was the problem.

[JoeZiminski]

On further reflection, I think we can just have two separate names, Drive 1, Drive 2 (as we are artificially setting the drive, it doesn't matter if they are real or not, as long as they are different

[JoeZiminski]

I was getting an error select.options was not found, I found removing this line worked

[sumana-2705]

Hi @JoeZiminski, Thank you so much for helping me out and taking the time to explain everything so clearly. It really made things much easier to understand. Just wanted to let you know that the test is now passing successfully. Thanks again!!

[JoeZiminski]

Great! Thanks @sumana-2705. A couple of fixes are now on the main branch, if you git rebase your branch onto the most recent main, I think the tests should pass on the CI. Then I believe this is good to go! Let me know if you have any questions about rebasing it can be a bit mysterious the first few times you do it.

[sumana-2705]

Hello @JoeZiminski. I've rebased the mount_external_drives branch onto the latest main, resolved the conflicts, and confirmed that all tests are now passing. Let me know if there's anything else you'd like me to update!

[JoeZiminski]

Hey @sumana-2705 I added psutils to the pyproject.toml and everything seems to be working well! Just one minor change suggested and this is good to merge!

[JoeZiminski]

Hey @sumana-2705 a very small thing, here this makes sense to handle the test case. But as a general rule it is good to try and avoid test cases being handled explicitly in the code (as it might be confusing how such a case could occur in general use).

In this case, I think it makes sense to factor out

def get_selected_drive(self):
    if platform.system() == "Windows":
        selected_drive = self.path_.drive + "\\"
    else:
        selected_drive = "/"
    return selected drive

and do self.selected_drive = self.get_selected_drive()

and then monkeypatch this function in the test. It still unfortunately does change the main code for the tests, but it is a more general refactoring than explicit handling of a test case which (should hopefully) never occur in the real world. I hope that makes sense let me know if not!

[sumana-2705]

Hello @JoeZiminski I have made changes as suggested by you. Please review it

[JoeZiminski]

Hey @sumana-2705 this is good to go! I merged in main and fixed the tests, and just made a few very minor edits:

• set all=True for the psutil call as it will also show mounted drives which can be useful
• call the functions directly to set the Select rather than use intermediate attributes on the class
• on macos / linux, for the default drive use the current first-folder rather than /

Thanks again for this great contribution!