964356 - (expose-editor-prefs) Implement and expose preferences for editor defaults

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

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

Codemirror is magical in various ways so we should have prefs for more than just this. I was thinking:

* autoclosing brackets (see bug 963937)
* autoclosing quotes
* tab sizes
* spaces vs tab chars
* autoindent

If we do completion for keywords and local vars a la brackets editor, we might consider some controls for that as well.

Comment 1

[Anton Kovalyov (:anton)]

As I said in bug 963937 I don't see why would we want to have a pref for every single feature CodeMirror has since it makes the end user experience more complicated. We do have prefs for tab sizes and spaces/tabs already though.

Comment 2

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

I hate autoindent, it never does what I want it to. But it's probably very exciting to some people.

Please, let's at least expose an option for this (and the size+spaces/tabs), in the Options panel.

Comment 3

[Brian Grinstead [:bgrins]]

I agree that we should show the available prefs for common options in the options panel.  And we don't need a pref for everything option that CodeMirror exposes (there are a ton of options available), but certain things like tabs vs spaces, bracket / quote matching, maybe autoindent are things people would like to control.  Some of the preferences may not even map directly into a CodeMirror option (completion for local variables or similar), but from a user's point of view we may still want to expose it as a single preference.

That said, we should think about redesigning the options panel to organize the information a bit better before adding a new section header and a few new fields.  I believe that we could balance out the existing content a bit more, that there would be space for three columns of content on larger resolutions, and that there are probably some other UI changes that could improve the panel.

Comment 4

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

Brian - exactly what I was thinking (in fact I caught a mid-air collision with a similar comment).

I also agree that we need to expand the prefs UI ( maybe do in-content for the extra space? )

Comment 7

[Paul Rouget [:paul]]

Can we have a UI that would also work outside of the toolbox? We need it for the app manager, which doesn't always have a toolbox open. If we can't, please re-open bug 1008709.

Comment 8

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

I'm not sure what you mean by 'work outside of' but the idea is that we will expose global prefs that uniformly affect all uses of codemirror to provide the basics of a consistent editing experience. I don't want the app manager's embedded editor to have separate pref from the rest of the tools. Surely we can have some way to open the global prefs ui from your UI.

Comment 9

[Paul Rouget [:paul]]

"outside of", I mean the UI.

We need a panel to configure codemirror. We don't have a toolbox in the app manager, so we can't show the panel option.

Comment 10

[Paul Rouget [:paul]]

If this could come with a theme selector like we see here: http://codemirror.net/demo/theme.html - that would be great :)

Comment 11

[Paul Rouget [:paul]]

I'm willing to work on this if someone can guide me (I know nothing about the sourceeditor and cm).

Comment 12

[Paul Rouget [:paul]]

If I add these options to the option panel, is that enough?

>        indentation: [2 spaces / 4 spaces / 8 spaces / tab]
>            Set "devtools.editor.tabsize"
>            Set "devtools.editor.expandtab"
>        detect indentation: [ ]
>            Set "devtools.editor.detectindentation"
>        auto close brakets: [ ]
>            Set "devtools.editor.autoclosebrackets"

I also want to add theme support, but probably in a different bug.

Comment 13

[Paul Rouget [:paul]]

I might just do that:
>        tab size: [2 / 4 / 8]
>            Set "devtools.editor.tabsize"
>        expand tab [ ]
>            Set "devtools.editor.expandtab"
>        detect indentation: [ ]
>            Set "devtools.editor.detectindentation"
>        auto close brakets: [ ]
>            Set "devtools.editor.autoclosebrackets"

Comment 14

[Paul Rouget [:paul]]

Attachment: v0.1

Comment 15

[Girish Sharma [:Optimizer]]

Comment on attachment 8426299 [details] [diff] [review]
v0.1

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

LGTM

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8426299 [details] [diff] [review]
v0.1

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

1) An opened editor does not get the preferences applied.  On one hand I'd say we can handle that in a follow up, but on the other it kind of feels broken if it doesn't get updated.  There is a function in editor.js called resetIndentUnit that may actually handle all of these preferences - if that got called when one of these prefs changed that may solve it.

2) When I uncheck 'autoclose brackets' then I open the style editor and type 'body {' it still inserts the closing bracket (even after closing and reopening toolbox)

3) The "Expand Tabs" string in the UI may make more sense as "Indent Using Spaces"

4) There is also some weirdness that happens with the columns in a smaller toolbox - if you shrink the toolbox enough for editor prefs to jump to the bottom then the two columns at the top end up not taking up the full space and it looks and feels weird.  We don't need to tackle a rewrite of the UI here, but if you can think of something that could lay these out a bit better across different sizes that would be nice.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8426299 [details] [diff] [review]
v0.1

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

::: browser/devtools/framework/options-panel.css
@@ -20,5 @@
>  }
>  
>  .options-vertical-pane {
>    margin: 5px;
> -  width: calc(50% - 30px);

I think what would be nice (and probably easy) is if once the screen gets small enough to jump to two cols, then the two go back to 50%.  This would prevent a big gap of whitespace to the right of the two top cols

@@ -25,1 @@
>    min-width: 350px;

This min-width seems like it could be a bit smaller - it seems like it jumps to two cols a bit too early

Comment 18

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Comment on attachment 8426299 [details] [diff] [review]
> v0.1
> 
> Review of attachment 8426299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1) An opened editor does not get the preferences applied.  On one hand I'd
> say we can handle that in a follow up, but on the other it kind of feels
> broken if it doesn't get updated.  There is a function in editor.js called
> resetIndentUnit that may actually handle all of these preferences - if that
> got called when one of these prefs changed that may solve it.

I will have to play with that method. I wonder if it can handle edge cases correctly...
 
> 2) When I uncheck 'autoclose brackets' then I open the style editor and type
> 'body {' it still inserts the closing bracket (even after closing and
> reopening toolbox)

because style editor is not respecting the property yet.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: editor-prefs-v1

So, this patch does live pref changes and stuff. It also adds keybinding options. But here are a few caveats wrt live changing of prefs.

 - If you have tabs in your scratchpad/style editor and you uncheck "indent with spaces" but have the "auto detect indentation" checked, it will not matter a thing as the tabs will still be tabs.
 - If you have "indent with spaces" selected and then you change the tab size, it will not update the existing tabs in the code.
 - If you have "auto detect indentation" unchecked and then you change the "indent with spaces" pref, it will not convert your existing tabs to spaces and vice-versa.

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8428318 [details] [diff] [review]
editor-prefs-v1

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

Overall, I think the option switching is working quite well and I like the ui behavior of reverting from 3 col to 2 that happens here when it gets resized.  I think the caveats you list in Comment 19 are all fine, but we should have a sourceeditor test to make sure that the relevant options on an open editor instance are set properly after each pref changes.  Then if/when we decide to tackle some of these points in the future we can add to that.

::: browser/devtools/framework/options-panel.css
@@ +21,5 @@
>  
>  .options-vertical-pane {
>    margin: 5px;
> +  width: calc(100%/3 - 30px);
> +  min-width: 250px;

At least on osx, 260px seems a bit better, but I don't really care

::: browser/devtools/framework/toolbox-options.js
@@ +135,5 @@
>        enabledToolbarButtonsBox.appendChild(createCommandCheckbox(tool));
>      }
>    },
>  
> +  infaillibletGetBoolPref: function(key) {

spelling: infallible

::: browser/devtools/sourceeditor/editor.js
@@ +327,5 @@
>      el.appendChild(env);
>  
>      this.once("destroy", () => el.removeChild(env));
> +
> +    require("resource:///modules/devtools/gDevTools.jsm").gDevTools

Why not require gDevTools at the top of the file with other dependancies?

Comment 21

[Brian Grinstead [:bgrins]]

This should be easier now that Bug 1031472 has landed.  We won't need to listen to pref-changed anymore or add support for autocloseBrackets in the editor - should be able to simply add the controls to the options panel.

Comment 22

[Brian Grinstead [:bgrins]]

Attachment: editor-prefs.patch

Rebased and removed now-unnecessary editor changes.  Would be nice if we could share the strings with the Web IDE preference panel in Bug 1008709 to make sure that we show consistent option names and they only need to get localized once.

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: options-panel-with-editor-prefs.png

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: editor-prefs.patch

Exposes editor prefs in the options panel.  Adds a third column to the options panel to fit everything in it (which shrinks to 2 when the screen is resized).  The editor now listens to these pref change events (Bug 1031472), so the changes for this patch are pretty much limited to the options panel itself.  I've also added a test for the menulists on the options panel (default color unit, tab size, editor keybinding).

Optimizer, I hope you don't mind me stealing this bug.

Comment 25

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=8ac489abd84d

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba903b7f347f