968896 - Add tern support to source editor to provide autocompletion and type inference

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Brandon Benvie [:benvie]]

We use CodeMirror and acorn now, so we now have the prerequisites to use tern and the tern CodeMirror plugin. Tern provides static analysis of code that can be used for things like autocompletion, type inference, variable renaming, etc.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

This is my initial patch which tarted as a weekend hack, so its integration with the sourceeditor is kind of shoehorned in. Issues:

* Keybindings I have hardcoded in. There's up to five things which could use shortcuts if we decided we want all of them: information (type inference, etc.), autocomplete (which we already have keys for), jump to definition, jump back, and variable rename.
* Not integrated with existing autocomplete code yet. I haven't looked into what's involved with this.
* The defs json files aren't working yet. These are definitions for the builtin JS functions for autocomplete and type inference. I've been trying to keep these files all together in toolkit/devtools/acorn, but this isn't as straightforward as it seems.
* Styling. I just used the default styles and didn't do any integration with the devtools theme.

Probably some other things I'm forgetting.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Forgot to add new files.

Comment 3

[Brandon Benvie [:benvie]]

Also that patch isn't rebased on top of bug 967813 yet, which broke off changes to acorn paths and added walk.js.

Comment 4

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Rebased, adds use of browser and ecma5 defs. Right now they're kind of hacked in as js files because I haven't been able to figure out how to include json files the way I want to.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Add back in the defs files.

Comment 6

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Update the styles to more closely match the devtools themes and support both light and dark.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

* Add a "jsAutocomplete" option for the editor which controls whether tern is used
* Some style cleanups
* localize keys (ctrl+space for complete, shift+space for type information)

Comment 8

[Girish Sharma [:Optimizer]]

Benvie, I know that using the inbuilt popup is not feasible in this case without a big rework on the term side, but at least can you move the js complete code inside the sourceeditor/autocomplete.js file, just like we have the css completion code ?
That way, the APIs to initiate completion will be same for both cases and no extra config will be required specially for js completion.

Comment 9

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

I've moved the js autocomplete stuff to autocomplete.js. Currently it's kind of annoying... you have an "autocomplete" option, and then after the editor is appended to something, you have to call editor.setupAutoCompletion(options). The problem is that autocompletion needs to be setup after the editor is attached to the window.

Just need to finish the tests.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Rebase, adds some tests for tern.

Comment 11

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Adds tests, cleans up integration with the existing autocompletion a little.

Comment 12

[Brandon Benvie [:benvie]]

Comment on attachment 8400815 [details] [diff] [review]
scratchpad-autocomplete.patch

Review time.

Girish: if you could take a look at the integration with your autocomplete stuff that'd be excellente!
Anton: just need a sanity check on what I've done to editor.js
Nick: toolkit/devtools modifications (addition of acorn)
Victor: general reviewish

Comment 13

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=feafd9265a62

Comment 14

[Girish Sharma [:Optimizer]]

Comment on attachment 8400815 [details] [diff] [review]
scratchpad-autocomplete.patch

Review of attachment 8400815 [details] [diff] [review]:
-----------------------------------------------------------------

I like it overall.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1585,5 @@
>        lineNumbers: true,
>        showTrailingSpace: Services.prefs.getBoolPref(SHOW_TRAILING_SPACE),
>        enableCodeFolding: Services.prefs.getBoolPref(ENABLE_CODE_FOLDING),
> +      contextMenu: "scratchpad-text-popup",
> +      autocomplete: true

Should (re)use a pref here to control completion.

::: browser/devtools/sourceeditor/editor.js
@@ +58,5 @@
>    "chrome://browser/content/devtools/codemirror/comment-fold.js",
>    "chrome://browser/content/devtools/codemirror/xml-fold.js",
> +  "chrome://browser/content/devtools/codemirror/foldgutter.js",
> +  "chrome://browser/content/devtools/codemirror/tern.js",
> +  "chrome://browser/content/devtools/codemirror/show-hint.js"

You are always loading these files into the editor, even if the mode is something other than JS. These should be loaded at the time of setting of autocompletion, by ./autocomplete.js

::: browser/devtools/styleeditor/StyleSheetEditor.jsm
@@ +267,1 @@
>        }

I think the method should take care of this check inside itself automatically. So that we can simply to sourceEditor.setupAutoCompletion(...)

::: browser/locales/en-US/chrome/browser/devtools/sourceeditor.properties
@@ +88,5 @@
> +autocomplete.commandkey=Space
> +
> +# LOCALIZATION NOTE  (showInformation.commandkey): This is the key to use to
> +# show more information, like type inference.
> +showInformation.commandkey=Shift-Space

Should say for both of these that a Ctrl or Cmd is automatically added.

::: toolkit/devtools/tern/browser.js
@@ +1,1 @@
> +module.exports = {

YAY, increase bc times :D

Comment 15

[Anton Kovalyov (:anton)]

Comment on attachment 8400815 [details] [diff] [review]
scratchpad-autocomplete.patch

Review of attachment 8400815 [details] [diff] [review]:
-----------------------------------------------------------------

I agree that we should probably load tern.js and show-hint.js on demand. Otherwise, editor changes look good.

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 8400815 [details] [diff] [review]
scratchpad-autocomplete.patch

Review of attachment 8400815 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/test/browser_scratchpad_autocomplete.js
@@ +14,5 @@
> +    .then(finish, console.error);
> +}
> +
> +
> +function* runTests([win, sp]) {

Can't we just standardize Task.js and have function@ (or whatever) be a task?

@@ +60,5 @@
> +    const p = editor.once(event);
> +    EventUtils.synthesizeKey(key, opts, editorWin);
> +    return p;
> +  }
> +}

I would love to see a few more tests, but I guess this is good enough for now.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +9,5 @@
>  const privates = new WeakMap();
>  
>  /**
>   * Prepares an editor instance for autocompletion, setting up the popup and the
>   * CSS completer instance.

Might want to update the documentation?

@@ +22,5 @@
> +  if (ed.config.mode == Editor.modes.js) {
> +    let defs = [
> +      "tern/browser",
> +      "tern/ecma5",
> +    ].map(require);

Cute.

::: browser/devtools/sourceeditor/codemirror/mozilla.css
@@ +113,5 @@
> +
> +.CodeMirror-hints {
> +  position: absolute;
> +  z-index: 10;
> +  overflow: hidden;

Should we go get a content css file now? Splitting content vs. theme css looks like a good idea now with so many rules.

https://wiki.mozilla.org/DevTools/CSSTips

@@ +168,5 @@
> +  color: white;
> +}
> +
> +.CodeMirror-Tern-completion {
> +  padding-left: 22px;

Nope.
-moz-padding-start.

@@ +175,5 @@
> +}
> +
> +.CodeMirror-Tern-completion:before {
> +  position: absolute;
> +  left: 2px;

This looks fugly to localize.

Comment 17

[Brandon Benvie [:benvie]]

(In reply to Girish Sharma [:Optimizer] from comment #14)
> Comment on attachment 8400815 [details] [diff] [review]
> scratchpad-autocomplete.patch
> 
> Review of attachment 8400815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like it overall.
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +1585,5 @@
> >        lineNumbers: true,
> >        showTrailingSpace: Services.prefs.getBoolPref(SHOW_TRAILING_SPACE),
> >        enableCodeFolding: Services.prefs.getBoolPref(ENABLE_CODE_FOLDING),
> > +      contextMenu: "scratchpad-text-popup",
> > +      autocomplete: true
> 
> Should (re)use a pref here to control completion.

I'll add a new pref for Scratchpad autocompletion specifically.

> ::: browser/devtools/sourceeditor/editor.js
> @@ +58,5 @@
> >    "chrome://browser/content/devtools/codemirror/comment-fold.js",
> >    "chrome://browser/content/devtools/codemirror/xml-fold.js",
> > +  "chrome://browser/content/devtools/codemirror/foldgutter.js",
> > +  "chrome://browser/content/devtools/codemirror/tern.js",
> > +  "chrome://browser/content/devtools/codemirror/show-hint.js"
> 
> You are always loading these files into the editor, even if the mode is
> something other than JS. These should be loaded at the time of setting of
> autocompletion, by ./autocomplete.js

I'll add a "loadSource" method to Editor and load those files on demand in autocomplete.js.


> ::: browser/devtools/styleeditor/StyleSheetEditor.jsm
> @@ +267,1 @@
> >        }
> 
> I think the method should take care of this check inside itself
> automatically. So that we can simply to sourceEditor.setupAutoCompletion(...)

Yeah I think the way it is now is awkward. I'll change it.


> ::: browser/locales/en-US/chrome/browser/devtools/sourceeditor.properties
> @@ +88,5 @@
> > +autocomplete.commandkey=Space
> > +
> > +# LOCALIZATION NOTE  (showInformation.commandkey): This is the key to use to
> > +# show more information, like type inference.
> > +showInformation.commandkey=Shift-Space
> 
> Should say for both of these that a Ctrl or Cmd is automatically added.

Ah yes, I'll add this (for autocomplete only, showInformation doesn't use ctrl/cmd).


> ::: toolkit/devtools/tern/browser.js
> @@ +1,1 @@
> > +module.exports = {
> 
> YAY, increase bc times :D

Hmm?

Comment 18

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vporof][:vp] from comment #16)
> Comment on attachment 8400815 [details] [diff] [review]
> scratchpad-autocomplete.patch
> 
> Review of attachment 8400815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/scratchpad/test/browser_scratchpad_autocomplete.js
> @@ +14,5 @@
> > +    .then(finish, console.error);
> > +}
> > +
> > +
> > +function* runTests([win, sp]) {
> 
> Can't we just standardize Task.js and have function@ (or whatever) be a task?

You mean at the language level? This is planned for ES7 as async functions [1][2].

[1] https://github.com/lukehoban/ecmascript-asyncawait
[2] http://wiki.ecmascript.org/doku.php?id=strawman:async_functions


> ::: browser/devtools/sourceeditor/codemirror/mozilla.css
> @@ +113,5 @@
> > +
> > +.CodeMirror-hints {
> > +  position: absolute;
> > +  z-index: 10;
> > +  overflow: hidden;
> 
> Should we go get a content css file now? Splitting content vs. theme css
> looks like a good idea now with so many rules.
> 
> https://wiki.mozilla.org/DevTools/CSSTips

Ah yeah, I just totally forgot to do the work of splitting up the CSS into the appropriate places. I'll fix that.


> @@ +175,5 @@
> > +}
> > +
> > +.CodeMirror-Tern-completion:before {
> > +  position: absolute;
> > +  left: 2px;
> 
> This looks fugly to localize.

Hmm I hadn't thought about that. The circles are color coded and have one letter. Do you think we need to localize this? I guess it would involve adding a bunch of new entries to the editor localization, one for each of the types.

Comment 19

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Addresses Optimizer's and some of Victor's feedback. The remaining bits are fixing up the CSS to live in the appropriate places and potentially addressing localization.

I just realized that part of this is the massive amount of content that lives in the defs files (ecma5.js and browser.js). I don't know how we can localize that, it's a huge amount of stuff. I'm not sure what to do about that.

Comment 20

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Forgot to include latest changes.

Comment 21

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

File save fail.

Comment 22

[Victor Porof [:vporof][:vp]]

> > @@ +175,5 @@
> > > +}
> > > +
> > > +.CodeMirror-Tern-completion:before {
> > > +  position: absolute;
> > > +  left: 2px;
> > 
> > This looks fugly to localize.

I mean the actual "left", which is "right" in RTL.

Comment 23

[Girish Sharma [:Optimizer]]

> > YAY, increase bc times :D
> 
> Hmm?

I just meant that more bc tests = increased times, in the midst of so many test suite changes. Its a good thing :)

I am just curious, how does the show-hint implementation play along with multi cursor in CM4 ?

Comment 24

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

Moves styles to appropriate places.

Comment 25

[Brandon Benvie [:benvie]]

That's the patch I want to land but for some reason I can't run tests locally so I'm not landing it yet.

Comment 26

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/59f4f732f6d7

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-dt failures.
https://hg.mozilla.org/integration/fx-team/rev/09847ac2ab60

https://tbpl.mozilla.org/php/getParsedLog.php?id=37945943&tree=Fx-Team

Comment 28

[Brandon Benvie [:benvie]]

Attachment: scratchpad-autocomplete.patch

I'm dumb. This should fix it, going to wait out the try.

https://tbpl.mozilla.org/?tree=Try&rev=efc98fd71b95

Comment 29

[Rob Campbell [:rc] (:robcee)]

what's happening here?

Comment 30

[Rob Campbell [:rc] (:robcee)]

victor said he can help out with these failures.

Comment 31

[Victor Porof [:vporof][:vp]]

Un-needinfo-ing me for now, but I'm still looking into this.

Comment 32

[Girish Sharma [:Optimizer]]

Attachment: scratchpad-autocomplete with metaKey

I think I have a fix for those failures. OSX does not have ctrlKey. Switched to metaKey. Lets see.

https://tbpl.mozilla.org/?tree=Try&rev=7eb686c3bd1f

Comment 33

[Victor Porof [:vporof][:vp]]

Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used for Spotlight!

Comment 34

[Girish Sharma [:Optimizer]]

Attachment: scratchpad-autocomplete with accelKey

Whoops, I mean, accelKey not metaKey.

new try : https://tbpl.mozilla.org/?tree=Try&rev=ace3e9edc2c4

(In reply to Victor Porof [:vporof][:vp] from comment #33)
> Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used
> for Spotlight!

I have not decided that that shortcut. It was a combined decision by various people.

All I did in my latest patch was to dispatch the accelKey+space shortcut instead of ctrlKey+space as the latter would never even fire the autocompletion on osx.

Comment 35

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #34)
> Created attachment 8423364 [details] [diff] [review]
> scratchpad-autocomplete with accelKey
> 
> Whoops, I mean, accelKey not metaKey.
> 
> new try : https://tbpl.mozilla.org/?tree=Try&rev=ace3e9edc2c4
> 
> (In reply to Victor Porof [:vporof][:vp] from comment #33)
> > Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used
> > for Spotlight!
> 
> I have not decided that that shortcut. It was a combined decision by various
> people.

Which various people? We need to have this conversation again. No editor that I know of uses Cmd+Space for autocompletion. It's a very bad choice on OS X.

Comment 36

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #35)
> (In reply to Girish Sharma [:Optimizer] from comment #34)
> > Created attachment 8423364 [details] [diff] [review]
> > scratchpad-autocomplete with accelKey
> > 
> > Whoops, I mean, accelKey not metaKey.
> > 
> > new try : https://tbpl.mozilla.org/?tree=Try&rev=ace3e9edc2c4
> > 
> > (In reply to Victor Porof [:vporof][:vp] from comment #33)
> > > Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used
> > > for Spotlight!
> > 
> > I have not decided that that shortcut. It was a combined decision by various
> > people.
> 
> Which various people? We need to have this conversation again. No editor
> that I know of uses Cmd+Space for autocompletion. It's a very bad choice on
> OS X.

I am not having that discussion because it was not my decision.

If you have any other shortcut in mind, feel free to comment.

FTR : http://irclog.gr/#browse/irc.mozilla.org/devtools/309444

Comment 37

[Victor Porof [:vporof][:vp]]

We decided on Ctrl+Space.

Comment 38

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vporof][:vp] from comment #37)
> We decided on Ctrl+Space.

I think the patch also has that only. I am trying out more logging to see why the tests fail.

Comment 39

[Girish Sharma [:Optimizer]]

Attachment: scratchpad-autocomplete with ctrl+space for all

Turns out the patch was not actually having ctrl+space for osx. But it was assuming so in the test. (Thus when I switched to accelKey, all tests pass).

I changed the shortcut to ctrl+space for osx too. Waiting for a green try..

https://tbpl.mozilla.org/?tree=Try&rev=05d672fe2534

Comment 40

[Girish Sharma [:Optimizer]]

Attachment: final scratchpad-autocompletion

Fixed the last bits of failure.

https://tbpl.mozilla.org/?tree=Try&rev=3f5384d46353

Comment 41

[Girish Sharma [:Optimizer]]

try is green. Autocompletion in scratchpad !!

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ac7c3681ea82

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac7c3681ea82

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Adding dev-doc-needed so we doc this feature existing in Scratchpad.

Comment 45

[Brian Grinstead [:bgrins]]

Will, this new feature in scratchpad allows showing a list of autocompletion suggestions in JS.  The list is triggered with ctrl+space (in all OS).  Type information on the current symbol can be shown with shift+space.  It is using the tern code-analysis engine to generate the suggestions and type information: http://ternjs.net/

Comment 46

[Will Bamberg [:wbamberg]]

(In reply to Brian Grinstead [:bgrins] from comment #45)
> Will, this new feature in scratchpad allows showing a list of autocompletion
> suggestions in JS.  The list is triggered with ctrl+space (in all OS).  Type
> information on the current symbol can be shown with shift+space.  It is
> using the tern code-analysis engine to generate the suggestions and type
> information: http://ternjs.net/

bgrins: does this look all right? https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Code_completion_and_inline_documentation

Awesome feature.

Comment 47

[Brian Grinstead [:bgrins]]

Looks great!  Also blocking this on Bug 1026560 to make sure we get the docs link working.