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

RESOLVED FIXED in Firefox 32

Status

RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: bbenvie, Assigned: bbenvie)

Tracking

({dev-doc-complete})

Trunk
Firefox 32
dev-doc-complete
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 17 obsolete attachments)

383.99 KB, patch
Optimizer
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8371601 [details] [diff] [review]
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.
Assignee: nobody → bbenvie
(Assignee)

Updated

5 years ago
Depends on: 967813
(Assignee)

Comment 2

5 years ago
Created attachment 8371602 [details] [diff] [review]
scratchpad-autocomplete.patch

Forgot to add new files.
Attachment #8371601 - Attachment is obsolete: true
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 8378527 [details] [diff] [review]
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.
Attachment #8371602 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 8378528 [details] [diff] [review]
scratchpad-autocomplete.patch

Add back in the defs files.
Attachment #8378527 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 8379881 [details] [diff] [review]
scratchpad-autocomplete.patch

Update the styles to more closely match the devtools themes and support both light and dark.
Attachment #8378528 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 8384858 [details] [diff] [review]
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)
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.
Depends on: 435164
(Assignee)

Comment 9

5 years ago
Created attachment 8389946 [details] [diff] [review]
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.
Attachment #8384858 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 8390785 [details] [diff] [review]
scratchpad-autocomplete.patch

Rebase, adds some tests for tern.
Attachment #8389946 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 8400815 [details] [diff] [review]
scratchpad-autocomplete.patch

Adds tests, cleans up integration with the existing autocompletion a little.
Attachment #8390785 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
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+
(Assignee)

Comment 17

5 years ago
(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?
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 8402928 [details] [diff] [review]
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.
Attachment #8400815 - Attachment is obsolete: true
Attachment #8402928 - Flags: review+
(Assignee)

Comment 20

5 years ago
Created attachment 8402929 [details] [diff] [review]
scratchpad-autocomplete.patch

Forgot to include latest changes.
Attachment #8402928 - Attachment is obsolete: true
Attachment #8402929 - Flags: review+
(Assignee)

Comment 21

5 years ago
Created attachment 8402937 [details] [diff] [review]
scratchpad-autocomplete.patch

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 ?
(Assignee)

Comment 24

5 years ago
Created attachment 8407679 [details] [diff] [review]
scratchpad-autocomplete.patch

Moves styles to appropriate places.
Attachment #8402937 - Attachment is obsolete: true
Attachment #8407679 - Flags: review+
(Assignee)

Comment 25

5 years ago
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.
(Assignee)

Comment 28

5 years ago
Created attachment 8407795 [details] [diff] [review]
scratchpad-autocomplete.patch

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)
Created attachment 8423349 [details] [diff] [review]
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
Cmd+Space for autocompletion on mac is a terrible idea! Cmd+Space is used for Spotlight!
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.

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.
Created attachment 8423421 [details] [diff] [review]
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
Attachment #8407795 - Attachment is obsolete: true
Attachment #8423364 - Attachment is obsolete: true
Created attachment 8424867 [details] [diff] [review]
final scratchpad-autocompletion

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/integration/fx-team/rev/ac7c3681ea82
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ac7c3681ea82
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Keywords: dev-doc-needed
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)
Keywords: dev-doc-needed → dev-doc-complete

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.