Add case-insensitive autocomplete #928

ChrisJefferson · 2016-10-27T20:41:05Z

Please make sure that this pull request:

is submitted to the correct branch (the stable branch is only for bugfixes)
contains an accurate description of changes for the release notes below
provides new tests or relies on existing ones
correctly refers to other issues and related pull requests

Tick all what applies to this pull request

[X ] Adds new features
[ X] Improves and extends functionality

Write below the description of changes (for the release notes)

Adds a new configuration option Autocompleter, which chooses how GAP filters when tab-completing words. Adds an option case-insensitive which does case-insensitive matching, and also allows an arbitrary function to be used.

(this option works only when GAP is compiled with readline support).

ChrisJefferson · 2016-10-27T20:49:21Z

Note: This patch is only for readline, I should add a comment that says that.

codecov-io · 2016-10-27T21:21:34Z

Current coverage is 54.45% (diff: 0.00%)

Merging #928 into master will decrease coverage by 0.02%

@@             master       #928   diff @@
==========================================
  Files           430        430          
  Lines        224650     224706    +56   
  Methods        3431       3431          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         122383     122361    -22   
- Misses       102267     102345    +78   
  Partials          0          0

Diff Coverage	File Path
0%	lib/cmdledit.g
•••••••••• 100%	lib/init.g

Powered by Codecov. Last update b7f89ca...89865c3

olexandr-konovalov · 2016-11-01T22:10:15Z

@ChrisJefferson - thank you. If this is does not change the default behaviour, and may be useful in some situations, then let's have it.

Regarding

Note: This patch is only for readline, I should add a comment that says that.

you can of course edit github comments, so you may update the description of changes for the release notes.

This should be documented somewhere in the manual - I think at least twice, in the section about readline, and in the section about name completion.
One should certainly test this manually, as this is a UI feature. I've checked out your branch, and it works fine. An interesting feature is that automatically corrects the case in the already typed text, and I think I rather like this. However, in the case described below it seems to do an unwanted change.

First, define

gap> LetItBeMixedCase:=1;
1
gap> letitbelowercase:=2;
2

Now, if you type letit and press Tab twice, it displays

gap> letit
LetItBeMixedCase  letitbelowercase

and this is what I expect.

However, if you type leti and press Tab, it automatically changes it to LetIt what is not what I would expect.

ChrisJefferson · 2016-11-01T22:19:57Z

Oo, that second one is interesting. I'm going to have to think about the best way to do a partial completion. There may not be a best thing to do, but I agree that changing the case is a bit weird.

For example, if I had LetItBeMixedCase and lEtiTBeWeirdCase, and the user types let<tab>, what should it complete to?

ssiccha · 2016-11-02T00:42:03Z

Sounds to me like it should only change the case of a prefix if the capitalization is the same across all matching names. Otherwise it should stay lowercase / not change the case. E.g.

with LetItBeMixedCase and lEtiTBeWeirdCase typing let<tab> should complete to letitbe
with LetItBeMixedCase and LetItAlsoBeMixedCase typing let<tab> should complete to LetIt

ChrisJefferson · 2016-11-02T08:00:05Z

ssiccha: That sounds reasonable. One more bad case, when a tab-completion produces a complete word, which can still be extended to a longer name. For example in gap we have Group and GROUP_BY_PCGS_FINITE_ORDERS (that second is an undocumented function, but let's ignore that).

If I type gro<tab> we are going to fill in the letters group, but what should the case be? I think we have to choose Group, because the user might be finished. If not, they'll have to tab-complete onwards which will change the case.

In general pressing when you have a finished GAP name will case-correct it.

olexandr-konovalov · 2016-11-02T09:58:25Z

@ssiccha

Sounds to me like it should only change the case of a prefix if the capitalization is the same across all matching names.

I think so too.

@ChrisJefferson

If I had LetItBeMixedCase and lEtiTBeWeirdCase, and the user types let, what should it complete to?

Maybe nothing, and then when the user press Tab second time, it displays both names.

Perhaps with readline one could also implement scrolling across all available completions, and also "backwards" completion (e.g. type "classes" and see all names that end with this string) - don't know how practically useful this is though.

A real life use case is also Complement. We have both Complementclasses and ComplementClasses as well as ComplementClassesRepresentatives, Complements etc.

ssiccha · 2016-11-02T11:02:03Z

@ChrisJefferson

If I type gro we are going to fill in the letters group, but what should the case be? I think we have to choose Group, because the user might be finished. If not, they'll have to tab-complete onwards which will change the case.

That sounds reasonable. Although I would like the following more:
@alex-konovalov

Perhaps with readline one could also implement scrolling across all available completions.

Typing gro<tab> completes to group. Pressing tab again should then show the available completions and also start cycling through them, starting with group itself and the next tab changes it to Group since it should come first among the possible completions.

frankluebeck · 2016-11-02T15:05:30Z

For my taste all the behaviours discussed here are more confusing than helpful. The mentioned problems/questions are the reason why I decided against a case-insensitive completion when I wrote the current function.

(Nevertheless, as long as the user has a choice I have no objections to experimenting with alternative Completion functions.)

ChrisJefferson · 2016-11-02T15:21:44Z

@frankluebeck : I agree that this is quite painful. For the specific use case of blind GAP users however it is very useful, as screen readers often don't give the case of characters when they are reading out, so case-insensitive completion makes GAP much easier to use.

ChrisJefferson · 2016-12-07T13:32:44Z

I believe this is now ready to be merged -- I am using it day-to-day.

fingolfin · 2016-12-07T13:47:28Z

lib/cmdledit.g

@@ -751,10 +751,108 @@ GAPInfo.CommandLineEditFunctions.Functions.(INT_CHAR(' ')) :=
                GAPInfo.CommandLineEditFunctions.Functions.SpaceDeletePrompt;
 BindKeysToGAPHandler(" ");

+
+BindGlobal("STANDARD_EXTENDERS", rec(


This name tells me nothing about the purpose of this record. Coud you add a comment before it that briefly states what is is used for?

fingolfin · 2016-12-07T13:48:44Z

lib/cmdledit.g

@@ -751,10 +751,108 @@ GAPInfo.CommandLineEditFunctions.Functions.(INT_CHAR(' ')) :=
                GAPInfo.CommandLineEditFunctions.Functions.SpaceDeletePrompt;
 BindKeysToGAPHandler(" ");

+
+BindGlobal("STANDARD_EXTENDERS", rec(
+  caseSensitive := function(cand, word)


What is cand, what is word, what is this function supposed to do? Again, I can of course reverse engineer it, by grepping for places this is used, but a brief comment would help a lot. Since there seem to be multiple such functions in this record, this could also be explained once in a comment before STANDARD_EXTENDERS

fingolfin · 2016-12-07T13:56:43Z

lib/cmdledit.g

+        c := cand[1][i+1];
+        for j in [2..Length(cand)] do
+          if Length(cand[j]) > i and cand[j][i+1] = c then
+            j := j+1;


This code confuses me. You increment j, but that's the loop variable, this has basically no effect, except perhaps for the last iteration, when j=Length(cand), yes? So, while I think I see why you are doing this, that seems like an needlessly "clever" way to do this loop, which obscures what goes on.

Why not instead drop this, negate the if-condition, and the replace the break; by something like commonPrefixFound := false. So the loop would be roughly this (untested, just typed it into my browser):

commonPrefixFound := true while commonPrefixFound and i < Length(cand[1])) do i := i + 1; # try finding a common prefix with one more character c := cand[1][i]; for j in [2..Length(cand)] do if Length(cand[j]) < i or cand[j][i] <> c then commonPrefixFound := false; i := i - 1; break; fi; od; od;

Ah OK, I see now that this was the original code, and you simply copied and then edited it.

I still think that it was / is needlessly arcane :-)

I agree it's weird, I'll be honest I hadn't looked at it carefully!

fingolfin · 2016-12-07T13:58:58Z

lib/cmdledit.g

+      # If there are several equal words, just pick the first one...
+      if Length(filtequal) > 0 then
+        return filtequal[1];
+      fi;


Why not just do this:

filtequal := First(cand, a -> LowercaseString(a) = lowword); if filtequal <> fail then return filtequal; fi;

fingolfin · 2016-12-07T14:00:18Z

lib/cmdledit.g

+      else
+        return fail;
+      fi;
+    end


perhaps add a comma after this, so that if more functions are added to the list in the future, no diff for that line shows up?

fingolfin · 2016-12-07T14:01:31Z

lib/init.g

+  description := [
+                   "Set how names are filtered during tab-autocomplete, \
+this can be: \"default\": case-sensitive matching. \"case-insensitive\": \
+case-insensitive matching, or a record of two functions, each of which take \


take -> takes

fingolfin · 2016-12-07T14:03:15Z

lib/init.g

+                   "Set how names are filtered during tab-autocomplete, \
+this can be: \"default\": case-sensitive matching. \"case-insensitive\": \
+case-insensitive matching, or a record of two functions, each of which take \
+two arguments. 'filter' takes a list of names and a partial identifier and returns \


Maybe clarify what filter is, namely the name of a record component? So:
or a record with two components named 'filter' and 'completer' which both must be set to functions: 'filter' takes...

olexandr-konovalov · 2017-02-15T00:28:51Z

Ping everyone - sounds like we shall merge this?

fingolfin · 2017-02-18T14:18:33Z

Personally I am not very keen on this change. But that's just a subjective thing, I guess... and it can be turned off (or is even off by default). So I am not actively objecting, but I am also not going to be the one who merges it ;-).

ssiccha mentioned this pull request Nov 2, 2016

Tab-completion: option to cycle through possible completions #931

Open

olexandr-konovalov added this to the GAP 4.9.0 milestone Nov 15, 2016

ChrisJefferson force-pushed the autocomplete branch 2 times, most recently from e2b416c to b7aae2a Compare November 24, 2016 09:15

fingolfin reviewed Dec 7, 2016

View reviewed changes

ChrisJefferson force-pushed the autocomplete branch from b7aae2a to 7fe2d8d Compare January 13, 2017 22:17

Add case-insensitive autocomplete

89865c3

ChrisJefferson force-pushed the autocomplete branch from 7fe2d8d to 89865c3 Compare January 13, 2017 22:35

olexandr-konovalov merged commit d7f2b93 into gap-system:master Feb 18, 2017

olexandr-konovalov mentioned this pull request Feb 21, 2017

GAP tab-completion should be case insensitive #919

Closed

ChrisJefferson deleted the autocomplete branch February 22, 2017 20:30

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add case-insensitive autocomplete #928

Add case-insensitive autocomplete #928

ChrisJefferson commented Oct 27, 2016 •

edited by olexandr-konovalov

Loading

ChrisJefferson commented Oct 27, 2016

codecov-io commented Oct 27, 2016 •

edited

Loading

olexandr-konovalov commented Nov 1, 2016 •

edited

Loading

ChrisJefferson commented Nov 1, 2016 •

edited

Loading

ssiccha commented Nov 2, 2016

ChrisJefferson commented Nov 2, 2016

olexandr-konovalov commented Nov 2, 2016

ssiccha commented Nov 2, 2016 •

edited

Loading

frankluebeck commented Nov 2, 2016

ChrisJefferson commented Nov 2, 2016

ChrisJefferson commented Dec 7, 2016

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

ChrisJefferson Jan 13, 2017

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

fingolfin Dec 7, 2016

olexandr-konovalov commented Feb 15, 2017

fingolfin commented Feb 18, 2017

Add case-insensitive autocomplete #928

Add case-insensitive autocomplete #928

Conversation

ChrisJefferson commented Oct 27, 2016 • edited by olexandr-konovalov Loading

Tick all what applies to this pull request

Write below the description of changes (for the release notes)

ChrisJefferson commented Oct 27, 2016

codecov-io commented Oct 27, 2016 • edited Loading

Current coverage is 54.45% (diff: 0.00%)

olexandr-konovalov commented Nov 1, 2016 • edited Loading

ChrisJefferson commented Nov 1, 2016 • edited Loading

ssiccha commented Nov 2, 2016

ChrisJefferson commented Nov 2, 2016

olexandr-konovalov commented Nov 2, 2016

ssiccha commented Nov 2, 2016 • edited Loading

frankluebeck commented Nov 2, 2016

ChrisJefferson commented Nov 2, 2016

ChrisJefferson commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olexandr-konovalov commented Feb 15, 2017

fingolfin commented Feb 18, 2017

ChrisJefferson commented Oct 27, 2016 •

edited by olexandr-konovalov

Loading

codecov-io commented Oct 27, 2016 •

edited

Loading

olexandr-konovalov commented Nov 1, 2016 •

edited

Loading

ChrisJefferson commented Nov 1, 2016 •

edited

Loading

ssiccha commented Nov 2, 2016 •

edited

Loading