Closed Bug 968896 Opened 10 years ago Closed 10 years ago

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

Categories

(DevTools :: Source Editor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: bbenvie, Assigned: bbenvie)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 17 obsolete files)

383.99 KB, patch
Optimizer
: review+
Details | Diff | Splinter Review
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.
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bbenvie
Depends on: 967813
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Forgot to add new files.
Attachment #8371601 - Attachment is obsolete: true
Also that patch isn't rebased on top of bug 967813 yet, which broke off changes to acorn paths and added walk.js.
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
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.
Attachment #8371602 - Attachment is obsolete: true
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Add back in the defs files.
Attachment #8378527 - Attachment is obsolete: true
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Update the styles to more closely match the devtools themes and support both light and dark.
Attachment #8378528 - Attachment is obsolete: true
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
* 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)
Attachment #8379881 - Attachment is obsolete: true
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.
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
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.
Attachment #8384858 - Attachment is obsolete: true
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Rebase, adds some tests for tern.
Attachment #8389946 - Attachment is obsolete: true
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Adds tests, cleans up integration with the existing autocompletion a little.
Attachment #8390785 - Attachment is obsolete: true
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
Attachment #8400815 - Flags: review?(vporof)
Attachment #8400815 - Flags: review?(scrapmachines)
Attachment #8400815 - Flags: review?(nfitzgerald)
Attachment #8400815 - Flags: review?(anton)
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
Attachment #8400815 - Flags: review?(scrapmachines) → feedback+
Attachment #8400815 - Flags: review?(nfitzgerald) → review+
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.
Attachment #8400815 - Flags: review?(anton) → review+
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.
Attachment #8400815 - Flags: review?(vporof) → review+
(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?
(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.
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
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.
Attachment #8400815 - Attachment is obsolete: true
Attachment #8402928 - Flags: review+
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Forgot to include latest changes.
Attachment #8402928 - Attachment is obsolete: true
Attachment #8402929 - Flags: review+
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
File save fail.
Attachment #8402929 - Attachment is obsolete: true
Attachment #8402937 - Flags: review+
> > @@ +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.
> > 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 ?
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
Moves styles to appropriate places.
Attachment #8402937 - Attachment is obsolete: true
Attachment #8407679 - Flags: review+
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.
Attached patch scratchpad-autocomplete.patch (obsolete) — Splinter Review
I'm dumb. This should fix it, going to wait out the try.

https://tbpl.mozilla.org/?tree=Try&rev=efc98fd71b95
Attachment #8407679 - Attachment is obsolete: true
Attachment #8407795 - Flags: review+
what's happening here?
Status: NEW → ASSIGNED
victor said he can help out with these failures.
Flags: needinfo?(vporof)
Un-needinfo-ing me for now, but I'm still looking into this.
Flags: needinfo?(vporof)
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
Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used for Spotlight!
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.
Attachment #8423349 - Attachment is obsolete: true
(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.
(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
We decided on Ctrl+Space.
(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.
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
Attachment #8407795 - Attachment is obsolete: true
Attachment #8423364 - Attachment is obsolete: true
Fixed the last bits of failure.

https://tbpl.mozilla.org/?tree=Try&rev=3f5384d46353
Attachment #8423421 - Attachment is obsolete: true
Attachment #8424867 - Flags: review+
try is green. Autocompletion in scratchpad !!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac7c3681ea82
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Adding dev-doc-needed so we doc this feature existing in Scratchpad.
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/
Flags: needinfo?(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.
Flags: needinfo?(wbamberg) → needinfo?(bgrinstead)
Looks great!  Also blocking this on Bug 1026560 to make sure we get the docs link working.
Depends on: 1026560
Flags: needinfo?(bgrinstead)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.