Re: [BUGS] Tab completion of function arguments not working in all cases

Hi,

I noticed this while testing 9.2, but it seems to go back to at least
8.3. Tab completion of function arguments doesn't work if the function
is schema-qualified or double-quoted. So for example,

DROP FUNCTION my_function ( <TAB>

completes the functions arguments, but

DROP FUNCTION my_schema.my_function ( <TAB>

doesn't offer any completions, and nor does

DROP FUNCTION "my function" ( <TAB>

The attached patch fixes these problems by introducing a new macro
COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
seems to be the nearest analogous code that covers all the edge cases.

Regards,
Dean

On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> I noticed this while testing 9.2, but it seems to go back to at least
> 8.3. Tab completion of function arguments doesn't work if the function
> is schema-qualified or double-quoted. So for example,

Good idea, would you like to submit for the open CF?

Josh

On 12 June 2012 21:27, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
>> I noticed this while testing 9.2, but it seems to go back to at least
>> 8.3. Tab completion of function arguments doesn't work if the function
>> is schema-qualified or double-quoted. So for example,
>
> Good idea, would you like to submit for the open CF?
>

Hmm, I was thinking of this as a bug because I found it during beta
testing, but on reflection I guess that it's more of an annoying
limitation than a bug per se. Also since no one appears to have
noticed until now, I guess it's not worth back-patching, so I'll
submit it for 9.3.

Regards,
Dean

[Hope it's OK if I move this thread to -hackers, as part of CF review.]

On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Hi,
>
> I noticed this while testing 9.2, but it seems to go back to at least
> 8.3. Tab completion of function arguments doesn't work if the function
> is schema-qualified or double-quoted. So for example,
>
>  DROP FUNCTION my_function ( <TAB>
>
> completes the functions arguments, but
>
>  DROP FUNCTION my_schema.my_function ( <TAB>
>
> doesn't offer any completions, and nor does
>
>  DROP FUNCTION "my function" ( <TAB>

+1 for the idea. I find the existing behavior rather confusing,
particularly the fact that a schema-qualified function name will be
tab-completed, i.e. this works.

DROP FUNCTION my_schema.my<TAB>

but then, as your second example above shows, no completions are
subsequently offered for the function arguments.

As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
DROP FUNCTION my_f<TAB>

which completes to:
DROP FUNCTION my_function

enter parenthesis, and hit tab:
DROP FUNCTION my_function(<TAB>

which, if there is only one match, could complete to:
DROP FUNCTION my_function(integer)

when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.

> The attached patch fixes these problems by introducing a new macro
> COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
> seems to be the nearest analogous code that covers all the edge cases.

Anyway, on to the review of the patch itself:

* Submission *

Patch applies cleanly to git head, and regression tests are not
expected for tab-completion enhancements.

* Features & Usability *

I've verified that tab-completing of the first argument to functions
for DROP FUNCTION and ALTER FUNCTION commands for the most part works
as expected. The one catch I noticed was that
Query_for_list_of_arguments wasn't restricting its results to
currently-visible functions, so with a default search_path, if you
have these two functions defined:

public.doppelganger(text)
my_schema.doppelganger(bytea)

and then try:

DROP FUNCTION doppelganger(<TAB>

you get tab-completions for both "text)" and "bytea(", when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.

* Coding *

The new macro COMPLETE_WITH_ARG seems fine. The existing code used
malloc() directly for DROP FUNCTION and ALTER FUNCTION
(tab-complete.c, around lines 867 and 2190), which AIUI is frowned
upon in favor of pg_malloc(). The patch avoids this ugliness by using
the new COMPLETE_WITH_ARG macro, so that's a nice fixup.

Overall, a nice fix for an overlooked piece of the tab-completion machinery.

Josh

On 18 June 2012 04:21, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> +1 for the idea. I find the existing behavior rather confusing,
> particularly the fact that a schema-qualified function name will be
> tab-completed, i.e. this works.
>
>  DROP FUNCTION my_schema.my<TAB>
>
> but then, as your second example above shows, no completions are
> subsequently offered for the function arguments.
>
> As a side note unrelated to this patch, I also dislike how function
> name tab-completions will not fill in the opening parenthesis, which
> makes for unnecessary work for the user, as one then has to type the
> parenthesis and hit tab again to get possible completions for the
> function arguments. The current behavior causes:
>  DROP FUNCTION my_f<TAB>
>
> which completes to:
>  DROP FUNCTION my_function
>
> enter parenthesis, and hit tab:
>  DROP FUNCTION my_function(<TAB>
>
> which, if there is only one match, could complete to:
>  DROP FUNCTION my_function(integer)
>
> when the last three steps could have been consolidated with better
> tab-completion. Perhaps this could be a TODO.
>

Hmm, I find that it does automatically fill in the opening
parenthesis, but only once there is a space after the completed
function name. So
"DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
(note the space at the end). Then pressing <TAB> one more time gives
"DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
the function arguments.

Alternatively "DROP FUNCTION my_function<TAB>" (no space after
function name) first completes to "DROP FUNCTION my_function " (adding
the space), and then completes with the opening parenthesis, and then
with the function arguments.

It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
than typing the opening bracket.

>> The attached patch fixes these problems by introducing a new macro
>> COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
>> seems to be the nearest analogous code that covers all the edge cases.
>
> Anyway, on to the review of the patch itself:
>
> * Submission *
>
> Patch applies cleanly to git head, and regression tests are not
> expected for tab-completion enhancements.
>
> * Features & Usability *
>
> I've verified that tab-completing of the first argument to functions
> for DROP FUNCTION and ALTER FUNCTION commands for the most part works
> as expected. The one catch I noticed was that
> Query_for_list_of_arguments wasn't restricting its results to
> currently-visible functions, so with a default search_path, if you
> have these two functions defined:
>
>  public.doppelganger(text)
>  my_schema.doppelganger(bytea)
>
> and then try:
>
>  DROP FUNCTION doppelganger(<TAB>
>
> you get tab-completions for both "text)" and "bytea(", when you
> probably expected only the former. That's easy to fix though, please
> see attached v2 patch.
>

Good catch.
I think that's a useful additional test, and is also consistent with
the existing code in Query_for_list_of_attributes.

> * Coding *
>
> The new macro COMPLETE_WITH_ARG seems fine. The existing code used
> malloc() directly for DROP FUNCTION and ALTER FUNCTION
> (tab-complete.c, around lines 867 and 2190), which AIUI is frowned
> upon in favor of pg_malloc(). The patch avoids this ugliness by using
> the new COMPLETE_WITH_ARG macro, so that's a nice fixup.
>
> Overall, a nice fix for an overlooked piece of the tab-completion machinery.
>
> Josh

Thanks for the review.

Cheers,
Dean

On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 18 June 2012 04:21, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:

>> As a side note unrelated to this patch, I also dislike how function
>> name tab-completions will not fill in the opening parenthesis, which
>> makes for unnecessary work for the user, as one then has to type the
>> parenthesis and hit tab again to get possible completions for the
>> function arguments. The current behavior causes:
>>  DROP FUNCTION my_f<TAB>
>>
>> which completes to:
>>  DROP FUNCTION my_function
>>
>> enter parenthesis, and hit tab:
>>  DROP FUNCTION my_function(<TAB>
>>
>> which, if there is only one match, could complete to:
>>  DROP FUNCTION my_function(integer)
>>
>> when the last three steps could have been consolidated with better
>> tab-completion. Perhaps this could be a TODO.
>>
>
> Hmm, I find that it does automatically fill in the opening
> parenthesis, but only once there is a space after the completed
> function name. So
> "DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
> (note the space at the end). Then pressing <TAB> one more time gives
> "DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
> the function arguments.
>
> Alternatively "DROP FUNCTION my_function<TAB>" (no space after
> function name) first completes to "DROP FUNCTION my_function " (adding
> the space), and then completes with the opening parenthesis, and then
> with the function arguments.
>
> It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
> than typing the opening bracket.

Interesting, I see the behavior you describe on Linux, using psql +
libreadline, and the behavior I showed (i.e. repeatedly pressing tab
won't automatically fill in the function arguments after the function
name is completed, seemingly because no space is deposited after the
completed function name) is with OS X 10.6, using psql + libedit.

[snip]
>> you get tab-completions for both "text)" and "bytea(", when you
>> probably expected only the former. That's easy to fix though, please
>> see attached v2 patch.
>
> Good catch.
> I think that's a useful additional test, and is also consistent with
> the existing code in Query_for_list_of_attributes.

OK, I'll mark Ready for Committer in the CF.

Josh

On Mon, Jun 18, 2012 at 5:45 PM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 18 June 2012 04:21, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
>
>>> As a side note unrelated to this patch, I also dislike how function
>>> name tab-completions will not fill in the opening parenthesis, which
>>> makes for unnecessary work for the user, as one then has to type the
>>> parenthesis and hit tab again to get possible completions for the
>>> function arguments. The current behavior causes:
>>> DROP FUNCTION my_f<TAB>
>>>
>>> which completes to:
>>> DROP FUNCTION my_function
>>>
>>> enter parenthesis, and hit tab:
>>> DROP FUNCTION my_function(<TAB>
>>>
>>> which, if there is only one match, could complete to:
>>> DROP FUNCTION my_function(integer)
>>>
>>> when the last three steps could have been consolidated with better
>>> tab-completion. Perhaps this could be a TODO.
>>>
>>
>> Hmm, I find that it does automatically fill in the opening
>> parenthesis, but only once there is a space after the completed
>> function name. So
>> "DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
>> (note the space at the end). Then pressing <TAB> one more time gives
>> "DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
>> the function arguments.
>>
>> Alternatively "DROP FUNCTION my_function<TAB>" (no space after
>> function name) first completes to "DROP FUNCTION my_function " (adding
>> the space), and then completes with the opening parenthesis, and then
>> with the function arguments.
>>
>> It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
>> than typing the opening bracket.
>
> Interesting, I see the behavior you describe on Linux, using psql +
> libreadline, and the behavior I showed (i.e. repeatedly pressing tab
> won't automatically fill in the function arguments after the function
> name is completed, seemingly because no space is deposited after the
> completed function name) is with OS X 10.6, using psql + libedit.
>
> [snip]
>>> you get tab-completions for both "text)" and "bytea(", when you
>>> probably expected only the former. That's easy to fix though, please
>>> see attached v2 patch.
>>
>> Good catch.
>> I think that's a useful additional test, and is also consistent with
>> the existing code in Query_for_list_of_attributes.
>
> OK, I'll mark Ready for Committer in the CF.

Applied with minor revisions:
* the comment above the COMPLETE_WITH_xyz macros needed an update
* I renamed the macro to COMPLETE_WITH_FUNCTION_ARG - to make it even
more clear what it does

Thanks!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/