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

feat: enhance PathTag validation and improve TUI file picker #18

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

umutdz
Copy link

@umutdz umutdz commented Mar 16, 2025

feat: enhance PathTag validation and improve TUI file picker

  • Add validation for single/multiple path selections
  • Implement directory-only path validation
  • Add file existence checks
  • Fix Enter key handling in the TUI interface

- Add validation for single/multiple path selections
- Implement directory-only path validation
- Add file existence checks
- Fix Enter key handling in TUI interface
@umutdz
Copy link
Author

umutdz commented Mar 16, 2025

@e3rd I tried on my machine, but I am not prof at Textual stuff, pls check it, waiting for your feedback

Copy link
Member

@e3rd e3rd left a comment

Choose a reason for hiding this comment

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

Will you give me your screen? I see no button to traversal


m = run(interface="tui")
out = m.form({
    "A number": 32,
    "A path1": PathTag("/tmp"),
    "A path2": PathTag("/var"),
    "A string": "ahoj",
})

image

multiple: bool = False
""" The user can select multiple files. """

file_exist: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

Let's name it just exist to keep the same nomenclature as pathlib.Path.

def on_button_pressed(self, event):
event.prevent_default() # prevent calling the parent MyButton
event.prevent_default()
Copy link
Member

Choose a reason for hiding this comment

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

keep that comment

# Special type: Submit button
elif tag.annotation is SubmitButton: # NOTE EXPERIMENTAL
elif tag.annotation is SubmitButton:
Copy link
Member

Choose a reason for hiding this comment

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

keep that comment, this PR has nothing to do with SubmitButton I think

case SecretTag():
o = SecretInput(tag, placeholder=tag.name or "", type="text")
case DatetimeTag():
Copy link
Member

Choose a reason for hiding this comment

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

let's add a comment

# NOTE: To be expanded or something. Because before, a DatetimeTag automatically defaulted to MyInput. You did not seem to change the behaviour.

from .widgets import (Changeable, MyButton, MyCheckbox, MyInput, MyRadioSet,
MySubmitButton, SecretInput)

if TYPE_CHECKING:
from . import TextualInterface


class FilePickerInput(Horizontal, Changeable):
Copy link
Member

Choose a reason for hiding this comment

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

put it to another file

- Add new file_picker_input.py with FileBrowser and FilePickerInput classes
- Fix PathTag annotation handling to prevent "Type must be str" errors
- Improve file selection UX with better tree visualization
- Update widgets.py to handle Path objects properly
- Enhance textual_adaptor.py and textual_app.py for better integration
@umutdz
Copy link
Author

umutdz commented Mar 18, 2025

Will you give me your screen? I see no button to traversal


m = run(interface="tui")
out = m.form({
    "A number": 32,
    "A path1": PathTag("/tmp"),
    "A path2": PathTag("/var"),
    "A string": "ahoj",
})

image

Absolutely, you're right, I examined more and added remained features

@e3rd
Copy link
Member

e3rd commented Mar 18, 2025

Great, I'll check soon!

@e3rd
Copy link
Member

e3rd commented Mar 18, 2025

It's much better than I thought it might be!

  • move focus to the file tree after clicking on the button (so that you can directly use arrow down to browse)
  • the file browser is at home. Let's begin at the folder from the current tag value. Ex. when the tag value is /var, let's start at var
  • the file browser is at home. Make it possible to navigate out. Currently, I found no possibility to go to root /.
  • if the file list is longer than 18 items, I can never navigate to those in the bottom

umutdz added 3 commits March 18, 2025 22:24
… visual feedback

- Add auto-focus on tree after opening file browser
- Implement keyboard navigation with arrow keys and Enter/Escape/Space shortcuts
- Add type-to-search functionality to quickly navigate to files/directories
- Implement starting directory based on tag value instead of home dir
@umutdz
Copy link
Author

umutdz commented Mar 18, 2025

Cool, I did it! Now, it is more useful

@e3rd
Copy link
Member

e3rd commented Mar 18, 2025

Really, the arrows work excellent now! Thanks! Could I have few more requests to make it even better please?

  • when in the text field, activate a shortcut like ctrl+b for browse
  • instead of Files, is it easy to put there the dir name instead? (instead of having this in the status)
  • When I'm in the tree and I hit a letter, Attribute error 'Tree' object has no attribute walk_nodes. Couldn't the keys be disabled? Or even better, could it be a quick search? Like hitting 's' would put me on dir starting with 's'.
  • The title does not work – look, the first field has the field title 'A number', it s missing for paths.
  • The default value stopped working for other fields, look, 'A number' should have a number pre-filled.

@umutdz
Copy link
Author

umutdz commented Mar 19, 2025

Cool, I'll update asap!

@umutdz
Copy link
Author

umutdz commented Mar 20, 2025

Hi @e3rd, tried to fix all, but I confused what you mean in here;
"when in the text field, activate a shortcut like ctrl+b for browse"

Please check new version.

Copy link
Member

@e3rd e3rd left a comment

Choose a reason for hiding this comment

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

I just meant the same thing you've already done for SecretTag. There, a shortcut Ctrl+T toggles the masked state. Here, the shorcut might toggle the button.
But nevertheless, what you did is awsome, a proper Quick Search, wow, it's working great and you don't have to add this detail.
Just resolve the two new minor comments and I'll gladly merge.

@@ -48,9 +131,6 @@ def on_radio_set_changed(self):

def on_key(self, event: events.Key) -> None:
if event.key == "enter":
# If the radio button is not selected, select it and prevent default
Copy link
Member

Choose a reason for hiding this comment

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

Why stripping the comment out?

def __hash__(self):
# The function is needed, otherwise the following would not work:
Copy link
Member

Choose a reason for hiding this comment

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

I see the same method hash already in parent. I still don't get why it's needed... but do you think it's a good idea to remove the method from PathTag altogether?

@@ -132,9 +135,46 @@ def __post_init__(self):
else:
for origin, _ in self._get_possible_types():
if origin in common_iterables:
self.multiple = True
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this? I thing we'll miss this. It's like

PathTag(["/tmp","/var"]) will become PathTag(["/tmp","/var"], multiple=True)

@@ -33,8 +33,91 @@ def get_ui_value(self):


class MyInput(Input, Changeable):
async def on_blur(self):
return self.trigger_change()
def __init__(self, value_or_tag, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've missed this part earlier. I fear the code will become a yet more labyrinth if such things are done not only in the Tag abstract logic and in the Adaptor widgetize logic but also in the widgets themself. Even though that does work now!

Make it accept either the val or the tag, not both.
Make a PathInput or MyPathInput or anything instead of checking isinstance. If needed, spread it into multiple Changeable classes.

# This is triggered when Enter is pressed
if self._convert_value(self.value) is not None:
self.trigger_change()
if hasattr(self._link, 'facet'):
Copy link
Member

Choose a reason for hiding this comment

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

Facet is an optional attribute, so it is always there but might be None.

Hence if self._link.facet.

initial_value = ""
if hasattr(tag, 'val') and tag.val is not None:
if isinstance(tag.val, list):
initial_value = ", ".join(str(p) for p in tag.val)
Copy link
Member

Choose a reason for hiding this comment

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

What use case do you cover here? A list handling. I fear this looks so difficult it should have a test for it. Tell me, I struggled a lot with the Tag internal typing and it might be handled somewhere there in the Tag.

@umutdz
Copy link
Author

umutdz commented Mar 21, 2025

Hi @e3rd, I just saw your comments, I'll check it out soon

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.

2 participants