Closed Bug 1029511 Opened 7 years ago Closed 7 years ago

Allow toggling of autocomplete option in source editor

Categories

(DevTools :: Source Editor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 4 obsolete files)

We should provide an easy way to reapply any editor preferences via projecteditor.reloadEditorPrefs() or similar
Attached patch reload-editor-prefs-WIP.patch (obsolete) — Splinter Review
WIP - still need to handle autocomplete pref since there isn't a current way to destroy that functionality
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attached patch reload-editor-prefs-WIP.patch (obsolete) — Splinter Review
working, needs tests
Attachment #8445137 - Attachment is obsolete: true
Attached patch reload-editor-prefs.patch (obsolete) — Splinter Review
We currently have a couple of limitations with autocompletion as an option for the editor:

1) It is something that you can only turn on - there is no way to disable it
1a) There is no way to call setOption("autocomplete") /  getOption("autocomplete") even though it comes through in the config just like all the other options which can be accessed via setOption/getOption
2) The autocompletion module never really fully cleans up after itself on destroying
3) When switching modes, the autocompletion mode remains the same.  So if you switched from JS->CSS then tern would still be running.
4) You have to wait until after the editor is appended before setting it.

This patch attempts to solve 1-3.
Attachment #8445220 - Attachment is obsolete: true
Attachment #8445386 - Flags: review?(vporof)
This needs to introduce a preference (devtools.editor.autocomplete).
Also, it doesn't work for autoclose brackets. See the work optimizer did here: https://bug964356.bugzilla.mozilla.org/attachment.cgi?id=8428318

And maybe in a followup: can we expose a keybinding pref? (emacs/vim)
Component: Developer Tools: WebIDE → Developer Tools: Style Editor
Summary: Project Editor: Reload editor preferences → Allow toggling of autocomplete option in source editor
Component: Developer Tools: Style Editor → Developer Tools: Source Editor
Blocks: 1031472
Attached patch autocomplete-toggle.patch (obsolete) — Splinter Review
Added a pref, updated tests
Attachment #8445386 - Attachment is obsolete: true
Attachment #8445386 - Flags: review?(vporof)
Attachment #8447392 - Flags: review?(vporof)
Comment on attachment 8447392 [details] [diff] [review]
autocomplete-toggle.patch

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

I'm not intimately familiar with how Tern should be initialized/destroyed, but this looks elegant and symmetrical and works properly.

::: browser/app/profile/firefox.js
@@ +1435,5 @@
>  pref("devtools.editor.expandtab", true);
>  pref("devtools.editor.keymap", "default");
>  pref("devtools.editor.autoclosebrackets", true);
>  pref("devtools.editor.detectindentation", true);
> +pref("devtools.editor.autocomplete", true);

Would there be any benefit in handling this pref on the fly? For example, if we ever get an option in the toolbox for this, ticking it off should make autocompletion go away in the editor immediately. But this can definitely be followup material.
Attachment #8447392 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Would there be any benefit in handling this pref on the fly? For example, if
> we ever get an option in the toolbox for this, ticking it off should make
> autocompletion go away in the editor immediately. But this can definitely be
> followup material.

For sure.  I'd like to have a reloadPrefs function on the editor that reloads the state of all preffable options (kind of like resetIndentUnit does for whitespace options).  Then we can have a listener for gDevTools pref-changed that updates all of these options in one place.  I may just add this in Bug 1031472 after adding ability to toggle the autoCloseBrackets option.
Fixed two small issues in tests:
1) Resolved a leak from browser_editor_autocomplete_basic.js that was resolved by calling popup.destroy() inside of the autocomplete destroy function.  This required no longer setting selectedIndex in the destroy function for autocomplete-popup (which was triggering an error in the test 'document.commandDispatcher is undefined').  Instead I'm just removing the DOM elements in destroy which has the same effect.  This error did not happen in style editor tests, so I'm thinking it may have to do with the editor test running in a popup window.
2) Changed the assertion in browser_styleeditor_autocomplete.js to make sure that the return value of getAutocompletionPopup() is null instead of just making sure that getAutocompletionPopup is undefined (it isn't anymore).

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a9017cd0225a
Attachment #8447392 - Attachment is obsolete: true
Attachment #8448134 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1de8e8c99817
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.