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

Text input improvements #528

Merged
merged 29 commits into from
May 25, 2022
Merged

Text input improvements #528

merged 29 commits into from
May 25, 2022

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented May 18, 2022

Another pass through text input which adds:

  • Click to move cursor
  • Cursor blinking
  • Scrolling support
  • Autocomplete via completion function
  • Support for CJK and single-codepoint double width emoji
  • Adds a new sandbox example "file search" demo (see video below)
text-input-filesearch-dogfood.mov

@darrenburns darrenburns changed the base branch from text-input-cursor-flash to css May 20, 2022 13:49
@darrenburns darrenburns changed the title Click on text input to move cursor Text input improvements May 20, 2022
Comment on lines +57 to +59
elif key == "home" or key == "ctrl+a":
self._editor.cursor_text_start()
elif key == "end":
elif key == "end" or key == "ctrl+e":
Copy link
Member Author

@darrenburns darrenburns May 24, 2022

Choose a reason for hiding this comment

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

Adding these standard terminal keybindings made me think about how these could clash with app-level keybindings. Should there be a standard way to say "for this instance of a widget, ignore this keybinding"?

I'm picturing an application that has "ctrl+a" to open some menu, and the user is currently focussed on a text input. Do they want to open the menu or do they want to move their cursor home? Do they want both? Do we offer Textual app authors the flexibility to choose what happens (and if so, how)?

Choose a reason for hiding this comment

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

...or would the user expects to have a "closer to a text edition context" behaviours, where "ctrl+a" for example selects the whole text instead?
Definitely not easy to figure out what would be the option that would fit the "principle of least surprise" for most users! 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be solved with bindings and actions? Bindings adds this layer of indirection, so the dev can decide what happens.

But this does feel like something we can figure out later.

@darrenburns darrenburns marked this pull request as ready for review May 24, 2022 09:41
@darrenburns darrenburns requested review from willmcgugan and olivierphi and removed request for willmcgugan May 24, 2022 09:48
Copy link

@olivierphi olivierphi left a comment

Choose a reason for hiding this comment

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

Awesome! 🤩

I only have a very little comment regarding the call to time.monotonic() :-)

Comment on lines +57 to +59
elif key == "home" or key == "ctrl+a":
self._editor.cursor_text_start()
elif key == "end":
elif key == "end" or key == "ctrl+e":

Choose a reason for hiding this comment

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

...or would the user expects to have a "closer to a text edition context" behaviours, where "ctrl+a" for example selects the whole text instead?
Definitely not easy to figure out what would be the option that would fit the "principle of least surprise" for most users! 😅

src/textual/widgets/text_input.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Looks great. A few comments.

src/textual/widgets/text_input.py Outdated Show resolved Hide resolved
src/textual/widgets/text_input.py Outdated Show resolved Hide resolved
src/textual/widgets/text_input.py Outdated Show resolved Hide resolved
"""Manages the blinking of the cursor - ensuring blinking only starts when the
user hasn't pressed a key in some time"""
if time.monotonic() - self._last_keypress_time > self.cursor_blink_period:
self._cursor_blink_visible = not self._cursor_blink_visible
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this, or could it be, a Reactive value? In that case you wouldn't need the explicit refresh.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think sometimes we set this value and don't want a refresh (because code which follows shortly after refreshes anyway) so it'd be an extra refresh that isn't required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The refreshes are always done on idle, so if you set reactive values and call refresh() explicitly, Textual will only repaint the screen once.

@willmcgugan willmcgugan merged commit 3e4721e into css May 25, 2022
@willmcgugan willmcgugan deleted the text-input-cursor-to-click branch May 25, 2022 16:12
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.

3 participants