Completer usability 2 (rebased of pr #1082) #1108

Merged
merged 5 commits into from Dec 7, 2011

Projects

None yet

3 participants

@Carreau
Member
Carreau commented Dec 6, 2011

rebased version of #1082 plus one fix.
ment to fix #1080
short recap:

  • dismiss the completer on .[]{}()...
  • continue to type in codemirror
  • space around =
  • uniformise behaviour between FF and chrome
@fperez
Member
fperez commented Dec 6, 2011

I'll test this now, but as a general comment, it's better not to close a PR and open a new one just b/c of a rebase: that breaks the discussion flow of the old one, people who were already being notified stop being so, etc. The only real reason for closing a PR and starting a new one is if a completely new approach is required, and the old branch is more or less abandoned. But since in this case you're just rebasing and continuing, there's really no need for a whole new PR.

Having said that, don't worry this time: let's continue here. I just mention it for future cases.

@fperez
Member
fperez commented Dec 6, 2011

OK, behavior-wise this is perfect. Awesome job!

I'd like @minrk to have a quick look at the JS as well, since he has by now picked up better JS skills than mine. I'm still going to review the code, but his eyes are better than mine :)

But unless we spot some minor fixes needed in terms of code/implementation, this is the functionality we needed. Many thanks for the great work.

I've checked on both Chrome and Firefox (on linux) and everything looks good.

@fperez fperez and 1 other commented on an outdated diff Dec 6, 2011
IPython/frontend/html/notebook/static/js/codecell.js
isCompSymbol : function (code)
- {return ((code>64 && code <=122)|| code == 189)}
+ {
+ return (code > 64 && code <= 90)
+ || (code >= 97 && code <= 122)
+ || (code == 95)
+ },
+ dismissAndAppend : function (code)
+ {
+ chararr = ['(',')','[',']','+','-','/','\\','.',' '];
@fperez
fperez Dec 6, 2011 IPython member

Just for readability, use spaces around the commas here so it's easier to see what each element is:

chararr = [ '(', ')', '[', ']', '+', '-', '/', '\\', '.', ' ' ];
@minrk
minrk Dec 6, 2011 IPython member

or better yet, avoid all those quotes with str.split:

chararr = "()[]+-/. ".split("")

@fperez
fperez Dec 6, 2011 IPython member

nice, I didn't know JS had split too :)

@fperez fperez commented on an outdated diff Dec 6, 2011
IPython/frontend/html/notebook/static/js/codecell.js
isCompSymbol : function (code)
- {return ((code>64 && code <=122)|| code == 189)}
+ {
+ return (code > 64 && code <= 90)
+ || (code >= 97 && code <= 122)
+ || (code == 95)
+ },
+ dismissAndAppend : function (code)
+ {
+ chararr = ['(',')','[',']','+','-','/','\\','.',' '];
+ codearr = chararr.map(function(x){return x.charCodeAt(0)});
@fperez
fperez Dec 6, 2011 IPython member

Is it necessary to do this dynamically or would it be cleaner/simpler to simply put the static code list in there manually? I just don't know enough about JS to tell...

@fperez fperez commented on an outdated diff Dec 6, 2011
IPython/frontend/html/notebook/static/js/codecell.js
isCompSymbol : function (code)
- {return ((code>64 && code <=122)|| code == 189)}
+ {
+ return (code > 64 && code <= 90)
+ || (code >= 97 && code <= 122)
+ || (code == 95)
+ },
+ dismissAndAppend : function (code)
+ {
+ chararr = ['(',')','[',']','+','-','/','\\','.',' '];
+ codearr = chararr.map(function(x){return x.charCodeAt(0)});
+ return jQuery.inArray(code, codearr) != -1;
@fperez
fperez Dec 6, 2011 IPython member

Is there no standard JS functionality to do this so that we need to use jQuery for it?

@fperez
Member
fperez commented Dec 6, 2011

I tested changing also the background of the fixed part to a different color (gray), and it doesn't really look very good. Your approach of using just boldface is nice and clean.

I reviewed the code and other than some small questions and quick fixes, it looks good to me. Once we also have @minrk's eyes on it and you address those small points, this can go in.

@Carreau
Member
Carreau commented Dec 6, 2011

To respond to your questions :

  • I'll add spaces
  • yes that can be made static, but I preferd readability. I already made enough mistake with keycode/keypress value to think that's worth.
  • I didn't found native Js function to do it... but if it exist it will be great

Sorry for the double PR, but the rebase had a 3 way merge and I preferd to keep the old branch, I could have pushed it on another branch and then override this one but it was too late

@fperez
Member
fperez commented Dec 6, 2011

On Tue, Dec 6, 2011 at 9:39 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

 * yes that can be made static, but I preferd readability. I already made enough mistake with keycode/keypress value  to think that's worth.

No problem.

 * I didn't found native Js function to do it... but if it exist it will be great

Sorry for the double PR, but the rebase had a 3 way merge and I preferd to keep the old branch, I could have pushed it on another branch and then override this one but it was too late

Ah, understood. No worries, that's actually a valid reason too: if it
makes your worfklow easier such as in this case, it's OK.

@minrk
Member
minrk commented Dec 6, 2011

I'm extremely far from a js expert, but I don't see anything obvious (other than the split() mentioned above). So if the behavior is right, then we should be set.

@fperez
Member
fperez commented Dec 7, 2011

Well, every bit helps, since none of us is an expert :). OK @Carreau, go ahead and fix these last few little things, and we'll merge this one!

Carreau added some commits Dec 1, 2011
@Carreau Carreau usability and cross browser compat for completer
	- dissmiss the completer, append what alredy type, **Plus** one caracter in
	  some cases

	  List of special caracter that are handle are in a given list `()[]./\-+`
	  for the moment. usefull for exaple when typing :
	  >>> np.s<tab>in(
	  and not having to type '(' twice.

	  they are handle separately has the [a-zA-Z] ones because otherwise they
	  will screw up the regexp, and are opt-in to avoid bugs with invisible
	  caracters send because some browser have 'keypress' event for meta keys

	  close #1080

		Note to this commit :
	  list of test for the completer across browser with --pylab=inline flag

	  #test direct one completion
	  plt.an<tab>       -> plt.annotate

	  #test filter,tab, only one completion
	  plt.a<tab>n<tab>  -> plt.annotate

	  # test partial common beggining
	  # test dismmised if user erase
	  plt.a<tab>nn<backspace><backspace>u<tab>                -> completer to `aut`
	  ........................................<tab><tab><tab> -> nothing should append
	  .......................................................<backspace><backspace<backspace> -> completer cancelled

	  #test dismiss if no more completion
	  plt.s<tab>c  -> completer 3 choices
	  ...........u -> dismissed whith what user have typed.  `plt.scu`

	  # test dismiss in no completion, special symbol
	  # opt-in list of caracters +-/\()[].
	  np<tab>.s          -> a 'dot' sould dismiss the completer and be appended
	  .........<tab>in(  -> np.sin(
	  np.s<tab>in[       -> np.sin[
c630b34
@Carreau Carreau Apply pep8 to js e480aee
@Carreau Carreau completer update code-miror on the fly
	Following @fperez advice, change the completer apparence to avoid user confusion.

	- Append what the user type in the completer in code-miror, (Almost) as if
	  codemirror still have focus
	- distinguish between "fixed" completion  part, which was sent to the kernel
	  (now written in bold) and filtering one,handled only in JS,that the user
	  can errase without dismissing the completer

	I changed the action of <Space> to dismiss the completer with what have
	already been typed and inserting a space instead of "picking" the currently
	hilighted option

	<Escape> will still dissmiss the completer and remove everything the user as
	typed since the completer invocation

	Note that while the completer is shown, code-mirror does not show any
	blinking cursor
5f3907c
@Carreau Carreau notebook: fix, only one completion autopick 3a94f81
@Carreau Carreau notebook: code Readability. Add dismissing symbols
	* As minrk suggested, changes list of char to string.split()
	* add also a few dismissing symbold like `,` `=` and `*` for
	  the completer
b1cbd5f
@Carreau
Member
Carreau commented Dec 7, 2011

fix, rebased and force pushed to be up to date with the recent PR merges.
I've added *,= to the list of dismissing caracters.

@fperez
Member
fperez commented Dec 7, 2011

Great, merging now so we can start stabilizing things for the 0.12 RC. Excellent work.

@fperez fperez merged commit 964bfe4 into ipython:master Dec 7, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment