963937 - Make it possible to pref off autoclose brackets in Scratchpad

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[dagger.bugzilla]

Attachment: useAutoClose.patch

Bug 940713 turned on automatic closing of brackets in the source code editor. Personally I find it really annoying and would like to be able to just turn it off. Patch attached.

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Nice. I had been meaning to log bug 964356 to track adding various preferences, this is a good obvious start.

Had you had any thought on how where we might provide a UI for editor prefs?

Comment 2

[Anton Kovalyov (:anton)]

Comment on attachment 8365531 [details] [diff] [review]
useAutoClose.patch

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

Patch looks good except for the comment above. Addons using Editor object directly should be able to set whatever options and ignore whatever prefs they'd like.

::: browser/devtools/sourceeditor/editor.js
@@ +162,5 @@
>    if (keyMap === "emacs" || keyMap === "vim")
>      this.config.keyMap = keyMap;
>    // Overwrite default config with user-provided, if needed.
>    Object.keys(config).forEach((k) => {
> +    // Don't re-enable auto close if it was preffed off.

We don't need this. If an addon uses our Editor and would like to ignore user prefs, it should be up to them.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jeff Griffiths (:canuckistani) from comment #1)
> Had you had any thought on how where we might provide a UI for editor prefs?

I was thinking about precisely that the other day and I think we could do a lot worse than just adding an Editor section in the options panel.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Panos Astithas [:past] from comment #3)
> (In reply to Jeff Griffiths (:canuckistani) from comment #1)
> > Had you had any thought on how where we might provide a UI for editor prefs?
> 
> I was thinking about precisely that the other day and I think we could do a
> lot worse than just adding an Editor section in the options panel.

That seems like the least disruptive path for the near-term, I agree. I do worry that it won't scale. Google has a ton of stuff in their options panel, for example, and I think it's quite cramped and awkward scrolling such a small viewport so often.

Comment 5

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

(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> That seems like the least disruptive path for the near-term, I agree. I do
> worry that it won't scale. Google has a ton of stuff in their options panel,
> for example, and I think it's quite cramped and awkward scrolling such a
> small viewport so often.

What we have now is almost as bad :) But that's a different bug, and not caused by too many options (they're just poorly organized).

Comment 6

[dagger.bugzilla]

> We don't need this.

In that case, what to do with the Style Editor? It currently passes |autoCloseBrackets: "{}()[]"| and thus will turn autoclosing on regardless of what the pref's set to.

Style Editor could read the pref... but I'm actually not sure why ' and " shouldn't be autoclosed for CSS, so maybe it should just not pass an autoCloseBrackets at all?

The other option would be to split the setting into autoCloseBrackets (list of characters) and autoCloseEnabled (true/false), and combine them in the Editor constructor to produce the argument that closebrackets.js expects. Doing it this way will let any Editor consumers configure the list of brackets without needing to read devtools.editor.autocloseenabled, but it'll involve a bit of fiddly code.

Comment 7

[Anton Kovalyov (:anton)]

> In that case, what to do with the Style Editor?

Hm, in this case I think we should split autoCloseEnabled and autoCloseBrackets as you said.

Comment 8

[dagger.bugzilla]

Attachment: useAutoClose-v2.patch

Okay, it turned out not to be very fiddly. If autoCloseEnabled is set to false (via pref or an Editor consumer), autoCloseBrackets is forced to false. Otherwise, autoCloseBrackets uses whatever the consumer specifies, or ()[]{}''"" by default.

(I used ()[]{}''"" rather than true because I felt that having "autoCloseBrackets: true, autoCloseEnabled: useAutoClose" would be confusing -- it looks like there's two options for enabling it. This way it's clear that autoCloseBrackets is intended to configure the list of brackets to use.)

Comment 9

[Anton Kovalyov (:anton)]

Comment on attachment 8367654 [details] [diff] [review]
useAutoClose-v2.patch

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

r+ with the condition that you move bracket closing configuration statement as stated in my inline comment.

::: browser/devtools/sourceeditor/editor.js
@@ +186,5 @@
>        this.config.gutters = this.config.lineNumbers ? ["CodeMirror-linenumbers"] : [];
>        this.config.gutters.push("CodeMirror-foldgutter");
>      }
>    }
> +  // Configure automatic bracket closing.

Don't you have the same behavior as in your previous patch here? If the pref is off and I do `new Editor({ autoCloseEnabled: true })` it will still be off since this code runs after `forEach` above is done applying custom configuration parameters. I think you need to put this statement before that `forEach` block. Looks good otherwise.

Comment 10

[dagger.bugzilla]

I don't think so... this.config will be initialized with autoCloseEnabled = false, and then the forEach() will overwrite that with true. Then the "Configure automatic bracket closing" code runs and sets this.config.autoCloseBrackets appropriately (which in this case means to leave it set to ()[]{}''"", which will enable autoclosing).

If that code went before the forEach(), then specifying `new Editor({ autoCloseEnabled: true })` (or false) would never do anything, since autoCloseEnabled is only checked by that bit of code, not by the CM module.

Comment 11

[Anton Kovalyov (:anton)]

Oh yeah, you're right. My bad.

Comment 12

[dagger.bugzilla]

Attachment: useAutoClose-v2.patch for checkin

Phew ;) Thanks for review.

Patch for checkin with updated commit message (no other changes).

Comment 13

[Matthew N. [:MattN]]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ca82e581873