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.

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

Implement basic code completion #1189

Merged
merged 31 commits into from Jun 11, 2013
Merged

Implement basic code completion #1189

merged 31 commits into from Jun 11, 2013

Conversation

gjtorikian
Copy link
Contributor

For #110.

Features include:

  • Multiple key bindings: Ctrl-Space|Shift-Space|Alt-Space
  • Keyboard navigation of autocompletion list
  • Everything is done in a worker
  • Clicking a different line detaches autocompletion
  • Fuzzy matching support
  • Inclusion of all Mode keywords from a higlighter's createKeywordMapper
  • Rematch on character deletion and insertion

I can think of a few improvements: enable autocompletion by default (without keybinding, based on a preference), and better integration of Mode highlighting rules into the autocompletion list, emmet support--but this is good enough for now.

Seems to work well on a file of 100k LOC (Ace unminified + jQuery unminified + APF unminified).

I made a few required changes to lib/ace/worker/mirror.js. The first is the ability to attach arbitrary additional data to the worker (beyond just the document value). The second is that currently, wokers fire on every doc change. I wanted the worker to only fire once, and then end itself. so I added the option to turn off deferredUpdate.

Current Issues

Given these please let me know if a WIP PR is not appropriate and I'll close this and move the discussion back to the issues.

  • This PR introduces a failing test, but I can't figure out why it's not functioning:
Test run aborted. Not all tests could be run!
ReferenceError: Worker is not defined
    at new WorkerClient (/Users/gjtorikian/Developer/ace/lib/ace/worker/worker_client.js:65:24)
    at Object.<anonymous> (/Users/gjtorikian/Developer/ace/lib/ace/autocomplete.js:43:14)
    at global.define (/Users/gjtorikian/Developer/ace/node_modules/amd-loader/lib/amd-loader.js:79:28)
    at Object.<anonymous> (/Users/gjtorikian/Developer/ace/lib/ace/autocomplete.js:31:1)
    at Module._compile (module.js:449:26)
    at Module.module.constructor._compile (/Users/gjtorikian/Developer/ace/node_modules/amd-loader/lib/amd-loader.js:11:31)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:362:17)
  • For large results (I've seen it at ~300), the popup has issues populating. This is not dependent on document size, but size of matches.

@matthew-dean
Copy link

HOLY SHIT THIS IS ACTUALLY HAPPENING.

                                      .-. _)/
                                          (0,0) .\
                                           (u)   ()
      .-.                           _\)  .-="=-.//  
     (o,o)                            \,//==\=== 
      (e)                              ()  =====            .-.
    .-="=-.  \(_           .-.         _____ =,=           (a.a)
   //==I==\\,/            (d.b)       ()--___(0V0)  (/_     (=)
  ()  ="=  ()              (u)        ||()----'      \, ___.="==-._
   \`(0V0)               .-="-.       |' \\           ()---` ==\==\\
  /|) ||\\              //==/=\\    =="   \'                   ="= ()
      || \\  ==.       () ==== ()_/_    =="               ____(0V0) \`
 jgs  ()  ()    \,      `\"=      `                      ()---` // (|\
     //  //      \\ ___(0);`               \)/ .-.       ||    //
    '/  '/        ()---'  \\                /,(o,o)      |'   ()
    "== "==                \\              ()  (w)     =="     \\
                            ()      /_ ___  \\,=",              \`
         .-.               //       '-()-()   =/=\\            =="
        (o.o)             '/         //\\||  ==== ()           .-.   \(_
         (n)              "==       /`  \\|  ="=  `|          (-.-)  ,/
       .-="=-.  \)                =="    `(0V0)    '--         (-)  ()
      // =T= \\,/                                            .-="=.//
     () ==|== ()                                            //==I==`
      \  ="=                                           _\__()  ===
      /)(0V0)                                 \) .-.   -' `   (0V0)
        // \\                                 //(e.e)        // //
       //   \\      .-.  \)                  ()  (=)        // //
      ()     ()    (o.o) ,|                   \\.="=.      () ()
       \\    ||     (m)  ()               ()   ==/==\\      \\ \\
  jgs   \'   '|     ="=//                //\\  ===  ()       \` \`
      =="     "==  //=T=   ()           /`  \(0V0) _/      ==" =="
                  _\` === //\\        =="      || (|\      ,==
   (/_   .-.       /\ (0V0)  `\                ||          .\
    \,  (o.o)           \\    "==              ()   /_____  \\
     ()  (=)             ()   ==.      .==      \\  ` '--()  ()
      \\.="=-.  (|/     //     /,     ,/         \`       \\ ||
        ==|==\\,/      '/     //     //         =="        \\||      \)/
         ===  ()       "==   ()     ()            .-.      (0A0)    ,/
         =.=                  \\   //            (a,a)      ="=    ()
        (0V0)____              \\ //              (o)        ==\= //
         \\ ----()\            (0A0)       \(/  .-="=.        .==='
          \\      `\            =,=          \,//==/=\\     //  (w)
           ()       "==        =====          () ==== ()   ()  (o'o)
          //                  .==I==.            ="=  |'    `\  '-'
         '/                 //  (=)  \\     ____(0V0)/|\     /|)
         "==               ()  (d'b)  ()   ()----`//
                            \`  '-'  `/     \\   //
                           /|\       /\\     \` ()
                                           =="   \\
                                                  \`
                                                =="


@danyaPostfactum
Copy link
Contributor

Is it hard to implement fuzzy filtering ?
I think suggestions should be shown on Ctrl+Space even when a prefix is empty.
Ace-based popup works a bit buggy.

But this is a very good start 👍

@faceleg
Copy link

faceleg commented Jan 6, 2013

👍 this is like every Christmas and birthday since the dawn of time occurring simultaneously.

@@ -928,6 +928,39 @@ var EditSession = function(text, mode) {

_self.$modes[mode] = new module.Mode();
_self.$modes[mode].$id = mode;

// this is for autocompletion to pick up regexp'ed keywords
Copy link
Member

Choose a reason for hiding this comment

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

this must go somewhere else, edit session is already too big

@nightwing
Copy link
Member

Fixed ace based popup, but not sure if keeping it is a good idea, would be good to find a nice listView with virtual viewport to use instead of it.
Using worker makes sense for doing something complicated alongside typing, but after pressing ctrl+space it just adds unneeded overhead, i think it would be better to do filtering and sorting in ui thread, just be cautious to not process big list at once, it should work fine for up to 20003000 suggestions.

@matthew-dean
Copy link

So, is this getting merged in? Agree with @faceleg. Although, I'd add that this is also like having the Easter Bunny choose you alone to receive all chocolate. FOR LIFE.

@joshholat
Copy link

+1, would love to see this merged in.

@lddubeau
Copy link

Another vote to see this merged in, in whatever form is best for Ace's architecture.

I've put in a pull request to integrate changes that would allow this branch to pass the tests. #1247

The issue was that Worker exists only when running in a browser. edit_session.js just avoids starting a worker if typeof Worker is undefined or if require.noWorker is true. I've just reused these conditions.

@gjtorikian
Copy link
Contributor Author

@lddubeau I'm going to merge master in and tweak it around a little. I thank you for your patch, but based on @nightwing's comments earlier I think I should drop the worker execution.

@anoopnair
Copy link

@MarkMurphy I see the exact same error as well!

@JoshuaGross
Copy link

My attempt at fixing the Uncaught TypeError: Cannot read property '$tokenizer' of undefined in edit_session.js:989 error, as well as an error when there were 0 keywords for completion.
Last two commits here: https://github.com/JoshuaGross/ace/pull/new/03122013-jg-codecomplete-bugfix
Sorry, not sure how to submit it as a proper pull request, it's not showing ajaxorg/ace as a remote base (it's just trying to create a pull request against my own master). If anyone has suggestions I'll submit a proper PR.

@wcandillon
Copy link
Contributor

I just played around with the feature and it looks great.
+1 for having it merged.

In addition with the code-snippet feature that has just been merge, it's a killer!

@anoopnair
Copy link

Hi - Do we have an ETA on when this might me merged to master? Thanks.

@matthew-dean
Copy link

Does this need more testing? Code tweaks? What's preventing this making it in?

@notslang
Copy link

notslang commented Jun 6, 2013

+1!

@lennartcl
Copy link
Contributor

@gjtorikian @nightwing Alright. Really nice work guys. This looks good enough to go in.

lennartcl added a commit that referenced this pull request Jun 11, 2013
@lennartcl lennartcl merged commit fa13f37 into master Jun 11, 2013
@lennartcl lennartcl deleted the feature/codecomplete branch June 11, 2013 00:09
@devknoll
Copy link

node ./Makefile.dryice.js fails because /tool/snippets.tmpl.js does not exist.

@nightwing
Copy link
Member

Added it in d2f7247

@devknoll
Copy link

👍 Is there any way to code complete automatically rather than with a key binding?

@nightwing
Copy link
Member

not yet, since that's not very useful with general word based completion

in simplest form it can be something like

editor.commands.on("afterExec", function(e) {
    if (e.command.name == "insertstring") {
        if (e.args == "<" || e.args == "." )
            e.editor.execCommand("startAutocomplete")
    }
})

@akshaypundle
Copy link

Is there a way I can define what all is in scope for the currently edited file so that code completion works not only for the symbols defined in the current source, but maybe in a library that I want to include?

@cscheid cscheid mentioned this pull request Sep 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet