Automatically reload all preferences in source editor

RESOLVED FIXED in Firefox 33

Status

DevTools
Source Editor
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
Firefox 33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
We need to provide a way to reload the option for autoclose brackets, kind of like resetIndentUnit does for whitespace-related properties.  This would read the devtools.editor.autoclosebrackets preference and remember the original config that was passed in (it currently resets that value if autoclose was false on initialization).

See https://bugzilla.mozilla.org/show_bug.cgi?id=1029511#c4 and https://bug964356.bugzilla.mozilla.org/attachment.cgi?id=8428318 for more information.
(Assignee)

Updated

4 years ago
Summary: Allow reload of autocloseBrackets option in source editor → Automatically reload all preferences in source editor
(Assignee)

Updated

4 years ago
Blocks: 964356
(Assignee)

Comment 1

4 years ago
Created attachment 8449452 [details] [diff] [review]
editor-reload-prefs.patch

This exposes a function that reloads all of the editor prefs, and automatically calls it when any of them change.  It also adds a test to this effect.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8449452 - Flags: review?(vporof)
(Assignee)

Comment 2

4 years ago
Created attachment 8449455 [details] [diff] [review]
reload-editor-prefs.patch

Initial patch forgot to call clearUserPref at the end of the test
Attachment #8449452 - Attachment is obsolete: true
Attachment #8449452 - Flags: review?(vporof)
Attachment #8449455 - Flags: review?(vporof)
(Assignee)

Comment 3

4 years ago
Created attachment 8449457 [details] [diff] [review]
editor-reload-prefs.patch

Hah, wrong patch.  I think this one will be right.
Attachment #8449455 - Attachment is obsolete: true
Attachment #8449455 - Flags: review?(vporof)
Attachment #8449457 - Flags: review?(vporof)
Comment on attachment 8449457 [details] [diff] [review]
editor-reload-prefs.patch

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

Nice!

::: browser/devtools/sourceeditor/autocomplete.js
@@ +340,5 @@
> +/**
> + * Returns whether autocompletion is enabled for this editor.
> + * Used for testing
> + */
> +function isAutocompletionEnabled({ ed }) {

I would rename this function, since, afaict, `privates` can store pretty much anything, not just autocompletion prefs.

::: browser/devtools/sourceeditor/editor.js
@@ +28,5 @@
>  const RE_JUMP_TO_LINE = /^(\d+):?(\d+)?/;
>  
>  const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
>  const events  = require("devtools/toolkit/event-emitter");
> +const { PrefObserver } = require("devtools/styleeditor/utils");

We should really make this a shared util, maybe it'd go along nicely with ViewHelpers.Prefs inside ViewHelpers.jsm? Followup material for sure.

@@ +413,5 @@
> +      useAutoClose ? this.config.autoCloseBracketsSaved : false);
> +
> +    // If alternative keymap is provided, use it.
> +    const keyMap = Services.prefs.getCharPref(KEYMAP);
> +    if (keyMap === "emacs" || keyMap === "vim" || keyMap === "sublime")

Nit: a const VALID_KEYMAPS = new Set(["emacs", "vim", "sublime]); and if (VALID_KEYMAPS.has(keyMap)) ... would be nicer here imo.
Attachment #8449457 - Flags: review?(vporof) → review+
(Assignee)

Comment 5

4 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #4)
> Comment on attachment 8449457 [details] [diff] [review]
> editor-reload-prefs.patch
> 
> Review of attachment 8449457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +340,5 @@
> > +/**
> > + * Returns whether autocompletion is enabled for this editor.
> > + * Used for testing
> > + */
> > +function isAutocompletionEnabled({ ed }) {
> 
> I would rename this function, since, afaict, `privates` can store pretty
> much anything, not just autocompletion prefs.

I'm not sure I follow - privates is a WeakMap declared inside of this file. The only time is a key is set is inside initializeAutoCompletion and the only time a key is deleted is inside of the destroy callbacks.
(Assignee)

Comment 6

4 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #4)
> ::: browser/devtools/sourceeditor/editor.js
> @@ +28,5 @@
> >  const RE_JUMP_TO_LINE = /^(\d+):?(\d+)?/;
> >  
> >  const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> >  const events  = require("devtools/toolkit/event-emitter");
> > +const { PrefObserver } = require("devtools/styleeditor/utils");
> 
> We should really make this a shared util, maybe it'd go along nicely with
> ViewHelpers.Prefs inside ViewHelpers.jsm? Followup material for sure.

I agree - the PrefObserver would be great functionality to share.  We could move it into ViewHelpers.Prefs as you suggest, or make a new ViewHelpers.PrefObserver to prevent any overlap with the API.  Heather, what do you think?

As a note, we also currently have a 'pref-changed' event emit gDevTools that is kind of confusing because it fires only when prefs are changed through the options panel.  We could also make gDevTools use this utility and observe on the "devtools." branch to it will emit more consistently.
Flags: needinfo?(fayearthur)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> I agree - the PrefObserver would be great functionality to share.  We could
> move it into ViewHelpers.Prefs as you suggest, or make a new
> ViewHelpers.PrefObserver to prevent any overlap with the API.  Heather, what
> do you think?
> 
> As a note, we also currently have a 'pref-changed' event emit gDevTools that
> is kind of confusing because it fires only when prefs are changed through
> the options panel.  We could also make gDevTools use this utility and
> observe on the "devtools." branch to it will emit more consistently.

I like all of this. ViewHelpers doesn't sound like the right file, maybe something more generic.
Flags: needinfo?(fayearthur)
(Assignee)

Comment 8

4 years ago
Created attachment 8450145 [details] [diff] [review]
editor-reload-prefs.patch

OK so when running the tests I realized there were a lot of places that were not properly destroying editors and were waiting to leak for something like this.  I'm a bit surprised that they weren't already leaking with the iframe inside of the editor.

This new version updates netmonitor, shader editor, markup view, and scratchpad to make sure things are being cleaned up.

It also removes any external calls to setupAutoCompletion and instead introduces autocompleteOpts for use with the style editor (so that it gets initialized with the correct options).  This is actually nicer, because before you'd have to do something like `editor({autocomplete:true}); editor.appendTo(foo).then(() => {editor.setupAutoCompletion(); });` and now you can just do `editor({autocomplete: true}); editor.appendTo(foo);`.
Attachment #8449457 - Attachment is obsolete: true
Attachment #8450145 - Flags: review?(vporof)
Comment on attachment 8450145 [details] [diff] [review]
editor-reload-prefs.patch

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

Looks good, modulo the addition of _editors maps. See below.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +129,5 @@
> +
> +    for (let ed of this._editors.values()) {
> +      ed.destroy();
> +    }
> +    this._editors.clear();

I don't understand why we need an extra _editors map? Why not just:

_destroyPanes: Task.async(function*() {
  ...
  for (let promise of this._editorPromises) {
    let editor = yield promise;
    editor.clear();
  }
}),

::: browser/devtools/shadereditor/shadereditor.js
@@ +364,5 @@
>      this._toggleListeners("off");
> +    for (let ed of this._editors.values()) {
> +      ed.destroy();
> +    }
> +    this._editors.clear();

Same here.
Attachment #8450145 - Flags: review?(vporof) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #9) 
> I don't understand why we need an extra _editors map? Why not just:
> 
> _destroyPanes: Task.async(function*() {
>   ...
>   for (let promise of this._editorPromises) {
>     let editor = yield promise;
>     editor.clear();
>   }
> }),

I was assuming async destroy functions would be an antipattern - if the toolbox is closing, then it is closing whether we want it to wait or not, right?  Or does it wait for async destroys to finish?
(Assignee)

Comment 11

4 years ago
Created attachment 8451625 [details] [diff] [review]
editor-reload-prefs.patch

OK, this version seems to fix all the leaks: https://tbpl.mozilla.org/?tree=Try&rev=b4e40098a291.

Heather, I had to make some changes to StyleSheetEditor.jsm to prevent leaking tests.  this.sourceEditor is only set once it is appended and ready, which is what we want. However, this can cause leaky tests if the test ends before it has been appended.  So the change is setting a `this._sourceEditor` property immediately after instantiating it so we have a reference to clean it up.  Also, I'm binding the editor events to named functions instead of anonymous so I can clean those up in destroy as well.  Let me know what you think.

Victor, for shadereditor.js simply putting the destroy in a yield didn't work, because in browser_se_first-run.js the call to _toggleListeners in destroy is the first call to _getEditor, so it actually instantiates new editors that never get appended (since it is in the middle of the destroy function).  The fix I put in place it keeping a bit to track whether it is destroyed, and if so then resolve immediately in _getEditor.  Another option would be do what the previous patch did (keep track of the pre-appended editors in a map).
Attachment #8450145 - Attachment is obsolete: true
Attachment #8451625 - Flags: review?(vporof)
Attachment #8451625 - Flags: review?(fayearthur)
Comment on attachment 8451625 [details] [diff] [review]
editor-reload-prefs.patch

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

This is fine.
Attachment #8451625 - Flags: review?(vporof) → review+
Comment on attachment 8451625 [details] [diff] [review]
editor-reload-prefs.patch

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

Style Editor change looks good.
Attachment #8451625 - Flags: review?(fayearthur) → review+
(Assignee)

Comment 14

4 years ago
Created attachment 8451905 [details] [diff] [review]
editor-reload-prefs.patch

OK, rebased and updated commit message
Attachment #8451625 - Attachment is obsolete: true
Attachment #8451905 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2296b7e4de6a
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2296b7e4de6a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33

Updated

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