start using new codemirror completion #4988

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
6 participants
@Carreau
Member

Carreau commented Feb 1, 2014

Should start fixing as you type completion.

This use the brand new completion-hint introduced by codemirror short after I wrote the completer.
it automatically handle the "as you type".

The only annoying things I see and I'm not sure how to handle :

  • We prevent event "downarrow" from propagating when the cursor is at the last char of codemirror, whic prevent the selection of next item in completer.
  • esc dismiss completer and escape to command mode (but not always)

TODO before merge :

  • remove path that say codemirror is up-to-date
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 1, 2014

Member

there are probably also some aligning bug to send upstream to codemirror when completing near edge of screen.

Member

Carreau commented Feb 1, 2014

there are probably also some aligning bug to send upstream to codemirror when completing near edge of screen.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 2, 2014

Member

This need a (one line) patch to CM, to work, and (should) fail gracefully on normal version. You cannot press tab to complete to the longuest prefix of completion without the patch.
It also trigger completion on . (dot) now.

Member

Carreau commented Feb 2, 2014

This need a (one line) patch to CM, to work, and (should) fail gracefully on normal version. You cannot press tab to complete to the longuest prefix of completion without the patch.
It also trigger completion on . (dot) now.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 5, 2014

Member

So IPython.Completer is gone now. I think this could affect @gibiansky, @BayardRock and in julia I suppose @malmaud that deal with codemirrors modes IIRC.

Just to keep you guys updated why this. Current completer was completely broken (preventing user to type) So I had to re-write most if not all of the things.

First thing, if you extend CodeCell.options_default.cm_config.extraKeys with something like "'$'": "completePassthrough" (yes single quote in double quote) it will invoke the completer on $ and insert $. It interesses some of you right ?
It is also much more easier to re-bind Tab to another shortcut.

Second <Tab><Tab> now with codemirror Master complete to longuestCommonSubstring, not only dealing with percent in %magics I think this should be tested. It fallback to pick current result on CM < 3.21.

Third, completion does not include analysis of surrounding tokens anymore for the time beeing, I'll need to rework some of the internal to allow that again.

@ellisonbg I still need help when completer is dismissed with esc not to switched to command mode.

This is a lot of change close to release but I think it is needed and the experience is much better that current and old completer.

This also layout ground to allow things like modification of completer like IfSharp does (icon in front of completion + tooltip for currently highlighted completion)

Please test and feedback ! Thanks !

Member

Carreau commented Feb 5, 2014

So IPython.Completer is gone now. I think this could affect @gibiansky, @BayardRock and in julia I suppose @malmaud that deal with codemirrors modes IIRC.

Just to keep you guys updated why this. Current completer was completely broken (preventing user to type) So I had to re-write most if not all of the things.

First thing, if you extend CodeCell.options_default.cm_config.extraKeys with something like "'$'": "completePassthrough" (yes single quote in double quote) it will invoke the completer on $ and insert $. It interesses some of you right ?
It is also much more easier to re-bind Tab to another shortcut.

Second <Tab><Tab> now with codemirror Master complete to longuestCommonSubstring, not only dealing with percent in %magics I think this should be tested. It fallback to pick current result on CM < 3.21.

Third, completion does not include analysis of surrounding tokens anymore for the time beeing, I'll need to rework some of the internal to allow that again.

@ellisonbg I still need help when completer is dismissed with esc not to switched to command mode.

This is a lot of change close to release but I think it is needed and the experience is much better that current and old completer.

This also layout ground to allow things like modification of completer like IfSharp does (icon in front of completion + tooltip for currently highlighted completion)

Please test and feedback ! Thanks !

@gibiansky

This comment has been minimized.

Show comment
Hide comment
@gibiansky

gibiansky Feb 5, 2014

@Carreau How does this change the messaging protocol, if at all? The only thing IHaskell did was use the complete_request and complete_reply messages - is this just a client-side change, or are those messages somehow affected?

@Carreau How does this change the messaging protocol, if at all? The only thing IHaskell did was use the complete_request and complete_reply messages - is this just a client-side change, or are those messages somehow affected?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 5, 2014

Member

@gibiansky only client side. I just thought you were patching to trigger completion on other keypress than tab.

Member

Carreau commented Feb 5, 2014

@gibiansky only client side. I just thought you were patching to trigger completion on other keypress than tab.

@ellisonbg ellisonbg added this to the 2.0 milestone Feb 5, 2014

@ellisonbg ellisonbg self-assigned this Feb 5, 2014

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 6, 2014

Member

Re-added options to complete from surrounding tokens. Deactivated on CM <= 3.21.1 because of bug.

Member

Carreau commented Feb 6, 2014

Re-added options to complete from surrounding tokens. Deactivated on CM <= 3.21.1 because of bug.

@ivanov

This comment has been minimized.

Show comment
Hide comment
@ivanov

ivanov Feb 6, 2014

Member

I've also been diving into CodeMirror / IPython interaction, so just adding myself to this PR to keep track of it.

Member

ivanov commented Feb 6, 2014

I've also been diving into CodeMirror / IPython interaction, so just adding myself to this PR to keep track of it.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 6, 2014

Member
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 7, 2014

Member

With master codemirror this should mostly work and be (depending on use cases) being equivalent to previous completer on 95% of cases.

The few edge cases I can think of are :

  • longuest common substring (lcs) is not at the beginning of the completions:
    • pandas.sandbox.<tab> show pandas and sandbox, lcs is and so tab once more complete to pandas.sandbox.and. In the other end you cannot force the lcs to start at beginning of word as you want the lcs of %matplotlib and matplotlib to be matplotlib and not empty string. Problem would be worse with subsequence instead of substring, but calculating the LCS of more than 2 string is hard one could calculate the LCS only on the union of from:to: range by extending the current completion with adjacent token but it's a small project on itself.
  • (current) lcs might not make sens on completion that do not have the same from:to: range and might be overzealous at replacing something it shouldn't, or not replacing it were it should have. I have no real world example, but I suppose we could come up with cases were numpy.random.norm<tab><tab> would complete to numpy.normal (dropping random, even maybe numpy) or to numpy.random.numpy.random.normal.

There are other annoying related things due to the fact that completion from the kernel do not come with a from:to: range and I use heuristics, so I'm not sure it is worth trying to that right now.

Member

Carreau commented Feb 7, 2014

With master codemirror this should mostly work and be (depending on use cases) being equivalent to previous completer on 95% of cases.

The few edge cases I can think of are :

  • longuest common substring (lcs) is not at the beginning of the completions:
    • pandas.sandbox.<tab> show pandas and sandbox, lcs is and so tab once more complete to pandas.sandbox.and. In the other end you cannot force the lcs to start at beginning of word as you want the lcs of %matplotlib and matplotlib to be matplotlib and not empty string. Problem would be worse with subsequence instead of substring, but calculating the LCS of more than 2 string is hard one could calculate the LCS only on the union of from:to: range by extending the current completion with adjacent token but it's a small project on itself.
  • (current) lcs might not make sens on completion that do not have the same from:to: range and might be overzealous at replacing something it shouldn't, or not replacing it were it should have. I have no real world example, but I suppose we could come up with cases were numpy.random.norm<tab><tab> would complete to numpy.normal (dropping random, even maybe numpy) or to numpy.random.numpy.random.normal.

There are other annoying related things due to the fact that completion from the kernel do not come with a from:to: range and I use heuristics, so I'm not sure it is worth trying to that right now.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 8, 2014

Member

@ivanov, do you want to bind completion to Ctrl+N and check that esc does not interfere with vim mode ?

Member

Carreau commented Feb 8, 2014

@ivanov, do you want to bind completion to Ctrl+N and check that esc does not interfere with vim mode ?

+ * TODO: remove once we update codemirror.
+ **/
+ var recent_cm = function(CodeMirror){
+ return true;

This comment has been minimized.

@Carreau

Carreau Feb 8, 2014

Member

Remove this line before merge. Use only when testing with dirty submodule and a master CM.

@Carreau

Carreau Feb 8, 2014

Member

Remove this line before merge. Use only when testing with dirty submodule and a master CM.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Feb 9, 2014

Member

Painfull rebase 😡.

Member

Carreau commented Feb 9, 2014

Painfull rebase 😡.

@Carreau Carreau closed this Feb 9, 2014

@Carreau Carreau reopened this Feb 13, 2014

@Carreau Carreau modified the milestones: 3.0, 2.0 Feb 13, 2014

@ellisonbg ellisonbg assigned Carreau and unassigned ellisonbg Mar 4, 2014

@jdfreder

This comment has been minimized.

Show comment
Hide comment
@jdfreder

jdfreder Mar 12, 2014

Member

Looks like this needs another rebase 😦 However, if I were you I would wait to rebase till #5297 gets merged, because it lightly touches the completer.

Member

jdfreder commented Mar 12, 2014

Looks like this needs another rebase 😦 However, if I were you I would wait to rebase till #5297 gets merged, because it lightly touches the completer.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Mar 13, 2014

Member

Sure, anyway this is 3.0 material.
I'll do that when I make a break in writting my dissertation.

Member

Carreau commented Mar 13, 2014

Sure, anyway this is 3.0 material.
I'll do that when I make a break in writting my dissertation.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Jun 22, 2014

Member

closing in favor of issue #6031.

Member

Carreau commented Jun 22, 2014

closing in favor of issue #6031.

@Carreau Carreau closed this Jun 22, 2014

@Carreau Carreau deleted the Carreau:new-completion branch Dec 15, 2014

@minrk minrk modified the milestones: 3.0, no action Feb 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment