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

Do not autocomplete if only whitespace precedes cursor #410

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

Do not autocomplete if only whitespace precedes cursor #410

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8145e39
if we are on the first character of a line and the input buffer is em…
krageon Aug 30, 2017
7651758
now properly only completes tabs when cursor is preceded by only whit…
krageon Aug 30, 2017
8044af6
applied styleCI proposed patch
krageon Aug 30, 2017
55bfd24
added end parameter to processCallback call in AutoCompleterTest
krageon Aug 30, 2017
07f723c
no longer passes a function call as the argument to empty, as that do…
krageon Aug 30, 2017
23d0cbb
reverted public API changes, use rl_point instead of rl_end in should…
krageon Aug 31, 2017
fbe76e0
Merge branch 'develop' of github.com:bobthecow/psysh into feature/tab…
krageon Sep 5, 2017
833eb66
Merge remote-tracking branch 'upstream/develop' into feature/tab-paste
krageon Sep 12, 2017
785a976
according to the readline spec: When rl_point is at the end of the li…
krageon Sep 12, 2017
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
36 changes: 30 additions & 6 deletions src/Psy/TabCompletion/AutoCompleter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ public function activate()
* Handle readline completion.
*
* @param string $input Readline current word
* @param int $index Current word index
* @param int $start Readline start match
* @param int $end Readline end match
* @param array $info readline_info() data
*
* @return array
*/
public function processCallback($input, $index, $info = array())
public function processCallback($input, $start, $end, $info = array())
Copy link
Owner

Choose a reason for hiding this comment

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

unfortunately this is a public method, so we won't be able to break backwards compatibility. right now.

but! $info['end'] is supposed to be a thing already, and we're using it below. we could work the $end param from the callback into the $info array to augment and/or replace that one, or we could just use that one?

or we could add the $end param to the end here, but that's just a bit awkward.

or we could rename this method and add a deprecated stub of a method called processCallback that still takes the old arguments. i think i like this third option best :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually the function passed to readline_completion_function that needs the change, which is also public. Refactoring that in a non-breaking manner gives me a large headache, so I reverted it back to the old (and admittedly broken) way. I use point now, which is more in line with what should happen anyway.

{
// Some (Windows?) systems provide incomplete `readline_info`, so let's
// try to work around it.
Expand All @@ -62,6 +63,10 @@ public function processCallback($input, $index, $info = array())
$line = $input;
}

if ($this->shouldInsertOnlyTab($line, $start, $end)) {
return array("\t");
Copy link
Owner

Choose a reason for hiding this comment

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

because of the magic space inserted after tab completion, this inserts five spaces for the first tab stop, then nine spaces for each additional tab stop. why don't we hijack this and return three spaces? then it's always four space indents :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It inserts a tab character, not spaces. I'm definitely not a fan of replacing it with space characters, as those are different (no matter what you think of tabs vs spaces) and substituting one for the other silently can never be what you want.

The tab + space behaviour is sort of vexing, but it's not something I can realistically solve in a way that doesn't break everything in environments that do work properly (for example ones that have the path from php/php-src#2720 applied).

What I can probably do is return array("\t" . chr(8));. chr(8) is the backspace character, which should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add in a setting for this and apply it to every autocompletion, thinking about it. Until the patch I mentioned works that's a sort of viable workaround to get autocompletion as you'd expect from an IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait no, that won't work. The space is appended after the completion - so a backspace that is inserted will actually just remove the tab character entirely.

Copy link
Owner

@bobthecow bobthecow Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

i understand that tabs and spaces are different. but practically, one looks like you'd expect, and the other looks really bad. so ¯\_(ツ)_/¯

}

$tokens = token_get_all('<?php ' . $line);

// remove whitespaces
Expand All @@ -81,19 +86,38 @@ public function processCallback($input, $index, $info = array())
return !empty($matches) ? $matches : array('');
}

/**
* @param string $line Readline current line
* @param int $start The start of the readline input
* @param int $end The end of the readline input
*
* @return bool true if the characters before $index are only whitespace
*
* @internal param int $index Current word index
*/
private function shouldInsertOnlyTab($line, $start, $end)
{
$precedingInput = substr($line, 0, $end);

$trimmedInput = trim($precedingInput);

return empty($trimmedInput);
Copy link
Owner

Choose a reason for hiding this comment

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

don't use empty, use === ''. because php.

>>> empty('0')
=> true

}

/**
* The readline_completion_function callback handler.
*
* @see processCallback
*
* @param $input
* @param $index
* @param string $input
* @param int $start
* @param int $end
*
* @return array
*/
public function callback($input, $index)
public function callback($input, $start, $end)
Copy link
Owner

Choose a reason for hiding this comment

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

do all the possible combinations of readline and libedit and php versions all provide $end for this callback? documentation is sadly lacking here :(

if not, should we fall back to the end in readline_info if it's available?

Copy link
Contributor Author

@krageon krageon Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I looked here: https://github.com/php/php-src/blob/master/ext/readline/readline.c#L490

And I saw that params always contains int start and int end.

For editline / libedit, editline/readline.h is included. This has typedef char **rl_completion_func_t(const char *, int, int); and extern rl_completion_func_t *rl_attempted_completion_function;

in editline's readline.c I see this:

/*
 * complete word at current point
 */
/* ARGSUSED */
int
rl_complete(int ignore __attribute__((__unused__)), int invoking_key)
{
	static ct_buffer_t wbreak_conv, sprefix_conv;
	char *breakchars;

	if (h == NULL || e == NULL)
		rl_initialize();

	if (rl_inhibit_completion) {
		char arr[2];
		arr[0] = (char)invoking_key;
		arr[1] = '\0';
		el_insertstr(e, arr);
		return CC_REFRESH;
	}

	if (rl_completion_word_break_hook != NULL)
		breakchars = (*rl_completion_word_break_hook)();
	else
		breakchars = rl_basic_word_break_characters;

	_rl_update_pos();

	/* Just look at how many global variables modify this operation! */
	return fn_complete(e,
	    (rl_compentry_func_t *)rl_completion_entry_function,
	    rl_attempted_completion_function,
	    ct_decode_string(rl_basic_word_break_characters, &wbreak_conv),
	    ct_decode_string(breakchars, &sprefix_conv),
	    _rl_completion_append_character_function,
	    (size_t)rl_completion_query_items,
	    &rl_completion_type, &rl_attempted_completion_over,
	    &rl_point, &rl_end);


}

Where rl_point and rl_end are updated as follows:

static void
_rl_update_pos(void)
{
	const LineInfo *li = el_line(e);

	rl_point = (int)(li->cursor - li->buffer);
	rl_end = (int)(li->lastchar - li->buffer);
}

Which leads me to believe that it does the same thing (http://www.delorie.com/gnu/docs/readline/rlman_28.html tells me that rl_end contains the number of characters in the line).

Even if that is in question, from the readline.c file in php-src, we can see that rl_end (the value passed to the callback as end) and the end key output by readline_info are the same:

https://github.com/php/php-src/blob/master/ext/readline/readline.c#L257

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, having dived into it this far, I cannot help but wonder how this works - I had interpreted end differently than what the documentation suggests it does, and I saw it work in that way as well. The variable I actually need is rl_point, which is always in readline_info but not passed as a callback argument.

{
return $this->processCallback($input, $index, readline_info());
return $this->processCallback($input, $start, $end, readline_info());
}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/Psy/Test/TabCompletion/AutoCompleterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public function testClassesCompletion($line, $mustContain, $mustNotContain)

$context->setAll(array('foo' => 12, 'bar' => new \DOMDocument()));

$code = $tabCompletion->processCallback('', 0, array(
$code = $tabCompletion->processCallback('', 0, strlen($line), array(
'line_buffer' => $line,
'point' => 0,
'end' => strlen($line),
Expand Down