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.

Sign up for GitHub

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

Add tab completion for file paths in save/open prompts #323

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

Add tab completion for file paths in save/open prompts #323

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ecece9b
prompt: add optional path tab-completion
InsanePrawn Oct 4, 2023
c6f9ad0
screen: enable file tab completion when prompting during open and save
InsanePrawn Oct 4, 2023
3d5d498
tests: test for file tab completion in open_test
InsanePrawn Oct 4, 2023
77943fc
prompt: add permanent _tab() handler
InsanePrawn Oct 6, 2023
bd003e1
prompt: only allow file completion at well-defined points
InsanePrawn Oct 6, 2023
0541559
screen.prompt(): make file_glob mandatory
InsanePrawn Oct 6, 2023
9fae655
adapt tests
InsanePrawn Oct 6, 2023
d3a5bd8
add search test to check that globbing is disabled
InsanePrawn Oct 6, 2023
4dffbc9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions babi/prompt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import curses
import enum
import glob
import os
from typing import TYPE_CHECKING

from babi.horizontal_scrolling import line_x
Expand All @@ -14,12 +16,20 @@


class Prompt:
def __init__(self, screen: Screen, prompt: str, lst: list[str]) -> None:
def __init__(
self,
screen: Screen,
prompt: str, lst: list[str],
file_glob: bool = False,
Copy link
Owner

Choose a reason for hiding this comment

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

this is a boolean trap -- it should be named-only if anything

Copy link
Author

@InsanePrawn InsanePrawn Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, not sure I understand what exactly you'd like me to do here.

Something like lst: list[str], *, file_glob: bool[ = False]?

Do you want me to drop the default value as well?

[Also realised I forgot a line break between prompt and lst]

Copy link

Choose a reason for hiding this comment

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

hi, I hope my comment doesn't come out of place -- anthony made a video on boolean traps

) -> None:
self._screen = screen
self._prompt = prompt
self._lst = lst
self._y = len(lst) - 1
self._x = len(self._s)
self._dispatch = Prompt.DISPATCH.copy()
if file_glob:
self._dispatch[b'^I'] = Prompt._file_complete
Copy link
Owner

Choose a reason for hiding this comment

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

unknown keys are already ignored in prompts -- the completion callback should just check if it's enabled or not rather than having to copy this dict every time

Copy link
Author

Choose a reason for hiding this comment

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

I don't think I understand this point - a key seems to be considered "known" as soon as it's in the dispatch dict, so I can't just add it globally.

If I didn't copy the dict, I'd be modifying the global DISPATCH dict, AFAIUI?

Also, for the callback to check on whether globbing is enabled, i'd have to make file_glob an instance variable, correct?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm saying you should handle the option inside the callback


@property
def _s(self) -> str:
Expand Down Expand Up @@ -95,6 +105,17 @@ def _delete(self) -> None:
def _cut_to_end(self) -> None:
self._s = self._s[:self._x]

def _file_complete(self) -> None:
partial = self._s[:self._x]
Copy link
Owner

Choose a reason for hiding this comment

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

this doesn't seem quite right -- it's going to take imagine foo.txt is on disk and you've written fo][od and you tab you're going to get foo.txtod which is nonsensical. I think it should really only listen to tab if you're at the end of the string

Copy link
Author

@InsanePrawn InsanePrawn Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sometimes, this is what I want, and I use this with tools that allow it.
Example: so][/foo.txt -> someLongFolder/foo.txt

This is especially useful to me to create a new file in an existing folder.

I understand this is a question of preference, I'd personally prefer this behaviour to stay as is.

[I usually add a space to the right of the cursor before hitting tab for visual separation and then delete it after completion has done its job, but that uses the same mechanic. I doubt we'd want to check for whitespace after the cursor and make it conditional]

Copy link

Choose a reason for hiding this comment

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

in that case it could listen to tab if it is at the end of the string or before a path separator I guess?

completions = glob.glob(partial + '*')
InsanePrawn marked this conversation as resolved.
Show resolved Hide resolved
if not completions:
return
common = os.path.commonprefix(completions)
if not common or common == partial:
return
self._s = common + self._s[self._x:] # don't eat text behind cursor
self._x = len(common)

def _resize(self) -> None:
self._screen.resize()

Expand Down Expand Up @@ -180,8 +201,8 @@ def run(self) -> PromptResult | str:
self._render_prompt()

key = self._screen.get_char()
if key.keyname in Prompt.DISPATCH:
ret = Prompt.DISPATCH[key.keyname](self)
if key.keyname in self._dispatch:
ret = self._dispatch[key.keyname](self)
if ret is not None:
return ret
elif key.keyname == b'STRING':
Expand Down
15 changes: 11 additions & 4 deletions babi/screen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ def prompt(
history: str | None = None,
default_prev: bool = False,
default: str | None = None,
file_glob: bool = False,
) -> str | PromptResult:
default = default or ''
self.status.clear()
Expand All @@ -407,7 +408,7 @@ def prompt(
else:
history_data = [default]

ret = Prompt(self, prompt, history_data).run()
ret = Prompt(self, prompt, history_data, file_glob=file_glob).run()

if ret is not PromptResult.CANCELLED and history is not None:
if ret: # only put non-empty things in history
Expand Down Expand Up @@ -720,7 +721,9 @@ def save(self) -> PromptResult | None:
# TODO: strip trailing whitespace?
# TODO: save atomically?
if self.file.filename is None:
filename = self.prompt('enter filename')
filename = self.prompt(
'enter filename', default=self.file.filename, file_glob=True,
)
Comment on lines +729 to +731
Copy link
Owner

Choose a reason for hiding this comment

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

this changes the behaviour. these shouldn't have a default

if filename is PromptResult.CANCELLED:
return PromptResult.CANCELLED
else:
Expand Down Expand Up @@ -762,15 +765,19 @@ def save(self) -> PromptResult | None:
return None

def save_filename(self) -> PromptResult | None:
response = self.prompt('enter filename', default=self.file.filename)
response = self.prompt(
'enter filename', default=self.file.filename, file_glob=True,
)
if response is PromptResult.CANCELLED:
return PromptResult.CANCELLED
else:
self.file.filename = response
return self.save()

def open_file(self) -> EditResult | None:
response = self.prompt('enter filename', history='open')
response = self.prompt(
'enter filename', history='open', file_glob=True,
)
if response is not PromptResult.CANCELLED:
opened = File(
response,
Expand Down
31 changes: 31 additions & 0 deletions tests/features/open_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,37 @@ def test_open(run, tmpdir):

h.press('^X')
h.await_text('hello world')
h.press('^X')
h.await_exit()


def test_file_glob(run, tmpdir):
base = 'globtest'
prefix = base + 'ffff.txt'
Comment on lines +43 to +44
Copy link
Owner

Choose a reason for hiding this comment

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

rather than variables I'd rather see the actual strings in the assertions -- rather than having to hunt around for what is actually being tested the expressions should be clear in tests. https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html

f = tmpdir.join(prefix + 'f')
f.write('hello world')
g = tmpdir.join(base + 'fggg')
g.write('goodbye world')
nonexistant = str(tmpdir.join('NONEXISTANT'))

with run(str(g)) as h:
h.await_text('goodbye world')

h.press('^P')
h.press(nonexistant)
h.press('Tab')
h.await_text(f'«{nonexistant[-7:]}')
h.press('^C')
h.await_text('cancelled')
h.press('^P')
h.press(str(tmpdir.join(base + 'fff')))
h.press('Tab')
h.await_text(str(f)[-7:])
h.press('Tab') # second tab shouldn't change anything
h.await_text(str(f)[-7:])
h.press('Enter')
h.await_text('[2/2]')
h.await_text('hello world')
h.press('^X')
h.press('^X')
h.await_exit()