Added the ability to change the directory disk #8
Conversation
| @@ -70,8 +72,10 @@ class FileSystemPickerScreen(ModalScreen[Optional[Path]]): | |||
| BINDINGS = [Binding("full_stop", "hidden"), Binding("escape", "dismiss(None)")] | |||
| """The bindings for the dialog.""" | |||
|
|
|||
| select_Changed = False | |||
There was a problem hiding this comment.
Couple of things here:
- It's not clear to me why this needs to be a class variable.
- Variables within this code should always follow the "usual" Python convention; so this would be
select_changed(all lower-case).
There was a problem hiding this comment.
this varible is just a way to prevent the select init from triggering the DirectoryNavigation.location.setter twice in a row first with the given path and then from finding mounted disks.
But I know this is not a good solution.
| def __init__( | ||
| self, location: str | Path = ".", title: str = "", select_button: str = "" | ||
| self, location: str | Path = ".", title: str = "", select_button: str = "", drives: list[str] = [] |
There was a problem hiding this comment.
Mutable defaults in method arguments are normally a really bad idea. I don't see why it would be used here.
There was a problem hiding this comment.
dirt of an old implementation
| @@ -81,6 +85,14 @@ def __init__( | |||
| select_button: Label for the select button. | |||
There was a problem hiding this comment.
Newly-added parameters would need to be documented in the doctoring.
|
|
||
| if os.name == 'nt': | ||
| import win32api | ||
| drivesStr = win32api.GetLogicalDriveStrings() |
There was a problem hiding this comment.
Please use snake_case for variable names, not camelCase.
| drivesStr = win32api.GetLogicalDriveStrings() | ||
| self.drives = drivesStr.split('\x00')[:-1] | ||
| else: | ||
| self.drives = "/" |
There was a problem hiding this comment.
It seems the intention is that drives is a list[str], but here it's been assigned a string.
Also, is there a reason why drives would be a "public" value?
| @@ -100,11 +112,14 @@ def compose(self) -> ComposeResult: | |||
| """ | |||
| with Dialog() as dialog: | |||
| dialog.border_title = self._title | |||
| currentDrive = str(os.path.splitdrive(MakePath.of(self._location).expanduser().absolute())[0]+'\\').upper() | |||
There was a problem hiding this comment.
This library is written so that it uses pathlib (and so also allows anyone to provide a Path-derived class so other implementations can be used). Could you consider a version of what you're doing here that follows the same approach.
| @@ -100,11 +112,14 @@ def compose(self) -> ComposeResult: | |||
| """ | |||
| with Dialog() as dialog: | |||
| dialog.border_title = self._title | |||
| currentDrive = str(os.path.splitdrive(MakePath.of(self._location).expanduser().absolute())[0]+'\\').upper() | |||
There was a problem hiding this comment.
Please use snake_case for variable names, not camelCase.
| @@ -100,11 +112,14 @@ def compose(self) -> ComposeResult: | |||
| """ | |||
| with Dialog() as dialog: | |||
| dialog.border_title = self._title | |||
| currentDrive = str(os.path.splitdrive(MakePath.of(self._location).expanduser().absolute())[0]+'\\').upper() | |||
There was a problem hiding this comment.
Could you break down for me what this line of code is actually doing? For example, it looks like there's a hard-coded path separator being used here; what is the reason for that?
There was a problem hiding this comment.
shady is to take the root of the given path
and serialize it to match the select values that match self.disks
disks = ["C:\\","D:\\","E:\\", ...]
# Get path absolute to _location exsample: ~\textual_fspicker
absolute_path = MakePath.of(self._location).expanduser().absolute()
# out: Path d:\project\textual_fspicker
# Split the root from the path
tuple_path = os.path.splitdrive(absolute_path)
# out: Tuple ["d:\", "project\textual_fspicker"]
# Get only root and serialize backslash
serialize_root_path = tuple_path[0]+'\\'
# out: String "d:\\"
# Type: String as root disck
current_drive = serialize_root_path.upper()
# out String "D:\\"| @@ -100,11 +112,14 @@ def compose(self) -> ComposeResult: | |||
| """ | |||
| with Dialog() as dialog: | |||
| dialog.border_title = self._title | |||
| currentDrive = str(os.path.splitdrive(MakePath.of(self._location).expanduser().absolute())[0]+'\\').upper() | |||
| yield Select.from_values(self.drives, value=currentDrive) | |||
There was a problem hiding this comment.
This would seem to force a drive selection widget on every platform; I'm not sure this is really a good design decision. If I were attempting this I suspect I'd only have this widget in place when on a system where it's relevant.
There was a problem hiding this comment.
the idea was to make it available only in systems such as Windows.
Not wanting to modify too much was to put general control on what system you are using.
| yield Button(self._select_button, id="select") | ||
| yield Button("Cancel", id="cancel") | ||
| yield Button("Cancel", id="cancel", variant="error") | ||
| yield Button(self._select_button, id="select", variant="primary") |
There was a problem hiding this comment.
This seems to swap the buttons around for no obvious reason; which would also be out of scope of this PR.
There was a problem hiding this comment.
Im sorry for the switch of buttons was not intended
|
This was intended to be just a draft of a possible implementation. |
I added a method to change the directory by reading the mounted drives. you just need to select from the select