Bug 69986 – Web Inspector: Make indent configurable

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 110790 [details]
Indent settings mockup

When edit CSS or JS, pressing tab inserts a tab character. It's 8 spaces wide. Would be nice to have a setting to change the indent.

Also, I think it's feasible to auto-detect indent style used in the file. If auto-detection fails, fall back to the user setting.

[Nikita Vasilyev]

I'm about to tackle the problem. I've noticed editing doesn't work in WebKit ToT. The pencil icon is grayed out. Why?

It does work in Chrome Canary. So, I guess I have to checkout and build Chromium as well.

[Pavel Feldman]

> It does work in Chrome Canary. So, I guess I have to checkout and build Chromium as well.

No need to build Chromium - you can use debug front-end build instead: http://code.google.com/chrome/devtools/docs/contributing.html.

I'll find out why editing does not work with ToT. It should (at least for CSS).

[Nikita Vasilyev]

(In reply to comment #2)
> No need to build Chromium - you can use debug front-end build instead: http://code.google.com/chrome/devtools/docs/contributing.html.

That's so much easier. Thanks!

> I'll find out why editing does not work with ToT. It should (at least for CSS).

It does work for CSS. Why it doesn't work for JS?

[Nikita Vasilyev]

Created attachment 111187 [details]
My first take on this. No auto-detection for now. Will make a separate bug.

[Nikita Vasilyev]

Created attachment 111511 [details]
Indent auto-detection has been added

Can I have a review, please?

[Vsevolod Vlasov]

Comment on attachment 111511 [details]
Indent auto-detection has been added

View in context: https://bugs.webkit.org/attachment.cgi?id=111511&action=review

When we are done with this patch, we could probably implement some auto-indentation for new lines (enter pressed).

> Source/WebCore/ChangeLog:11
> +        No new tests. (OOPS!)

Bots won't let you land the patch having "OOPS" in ChangeLog.
We should probably have autoIndent logic test.

> Source/WebCore/inspector/front-end/Settings.js:84
> +    this.indent = this.createSetting("indent", "    ");

defaultIndent?

> Source/WebCore/inspector/front-end/SettingsScreen.js:53
> +    p = this._appendSection(WebInspector.UIString("Editing"));

Please add all new UIStrings to Source/WebCore/English.lproj/localizedStrings.js.

> Source/WebCore/inspector/front-end/SourceFrame.js:839
> +        if (!this._textModel._indent && WebInspector.settings.autodetectIndent.get()) {

Please don't use braces for one-liners.

> Source/WebCore/inspector/front-end/SourceFrame.js:844
> +    detectIndent: function()

I think we would get more reliable results if we look on the line immediately following the line ending with a bracket "{" (probably followed by whitespaces). Otherwise we could be confused by all sorts of ascii art in the comments in the head of the file.
While working for JS and CSS, that won't work for HTML though.

> Source/WebCore/inspector/front-end/SourceFrame.js:850
> +            var rIndent = /^[ |\t]+/gm;

I don't think you need 'm' flag here since you are already matching this RegExp against lines.
Looks like you are mixing two approaches here - iterating over the lines and matching against multi-line RegExp in the loop.

> Source/WebCore/inspector/front-end/SourceFrame.js:859
> +                if (indent === " ")

Please use braces for blocks with more than one line (including comments).

[Nikita Vasilyev]

Thanks for the review!

I've made a separate bug for auto-detection feature (https://bugs.webkit.org/show_bug.cgi?id=70406) because auto-detection shouldn't hold 70181.

I haven't wrote any Inspector's tests yet, I'll dig it.

>> Source/WebCore/inspector/front-end/Settings.js:84
>> +    this.indent = this.createSetting("indent", "    ");
>
>defaultIndent?

Yes. I think 4 spaces is the most used indent in JS and CSS.

[Pavel Feldman]

Comment on attachment 111511 [details]
Indent auto-detection has been added

View in context: https://bugs.webkit.org/attachment.cgi?id=111511&action=review

>> Source/WebCore/inspector/front-end/Settings.js:84
>> +    this.indent = this.createSetting("indent", "    ");
> 
> defaultIndent?

You should create this setting from within the TextEditorModel.

> Source/WebCore/inspector/front-end/SettingsScreen.js:58
> +    p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Auto-detect indent"), WebInspector.settings.autodetectIndent));

Auto-detect should be one of the options ("Auto"). Otherwise two active settings "4 spaces" and "auto-detect" will confuse user.

> Source/WebCore/inspector/front-end/SourceFrame.js:831
> +        this.setupIndent();

You should do this lazily.

> Source/WebCore/inspector/front-end/SourceFrame.js:837
> +    setupIndent: function()

This should be a part of the TextEditorModel.

>> Source/WebCore/inspector/front-end/SourceFrame.js:839
>> +        if (!this._textModel._indent && WebInspector.settings.autodetectIndent.get()) {
> 
> Please don't use braces for one-liners.

Then accessing _indent would be fine.

>> Source/WebCore/inspector/front-end/SourceFrame.js:844
>> +    detectIndent: function()
> 
> I think we would get more reliable results if we look on the line immediately following the line ending with a bracket "{" (probably followed by whitespaces). Otherwise we could be confused by all sorts of ascii art in the comments in the head of the file.
> While working for JS and CSS, that won't work for HTML though.

Ditto, should be in TextModel.

> Source/WebCore/inspector/front-end/SourceFrame.js:848
> +        for (var lineNumber = 0; lineNumber < linesCount; lineNumber++) {

You want to put lineNumber < linesCount && lineNumber < 100 here so that you don't kill CPU on GWT-generated code.

>> Source/WebCore/inspector/front-end/SourceFrame.js:850
>> +            var rIndent = /^[ |\t]+/gm;
> 
> I don't think you need 'm' flag here since you are already matching this RegExp against lines.
> Looks like you are mixing two approaches here - iterating over the lines and matching against multi-line RegExp in the loop.

No abbreviations in WebKit: const indentRegex =. Also, var firstChar = line.charAt(0); if (firstChar === " " || firstChar === "\t") would be infinitely faster.

> Source/WebCore/inspector/front-end/SourceFrame.js:861
> +                    continue;

I now know that this is not infinite loop, but it is worth mentioning in the comments.

> Source/WebCore/inspector/front-end/TextEditorModel.js:237
> +            return WebInspector.settings.indent.get();

Moving detection and setting logic into TextEditorModel will remove this code as well.

[Nikita Vasilyev]

(In reply to comment #8)
> (From update of attachment 111511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111511&action=review
> 
> >> Source/WebCore/inspector/front-end/Settings.js:84
> >> +    this.indent = this.createSetting("indent", "    ");
> > 
> > defaultIndent?
> 
> You should create this setting from within the TextEditorModel.
> 
> > Source/WebCore/inspector/front-end/SettingsScreen.js:58
> > +    p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Auto-detect indent"), WebInspector.settings.autodetectIndent));
> 
> Auto-detect should be one of the options ("Auto"). Otherwise two active settings "4 spaces" and "auto-detect" will confuse user.

I agree, it is confusing, but the idea is "auto-detect indent style used in the file. If auto-detection fails, fall back to the user setting.".

JS/CSS files may not have any indents. They may be empty or minified. In this case, indent must not be equals empty string. We could fall back to the default indent, but I'm not sure it's a good idea.


> > Source/WebCore/inspector/front-end/SourceFrame.js:831
> > +        this.setupIndent();
> 
> You should do this lazily.

I do:

    setupIndent: function()
    {
        if (!this._textModel._indent && ...

It wouldn't matter, though, if we move away from this solution to the one proposed by Vsevolod:

"I think we would get more reliable results if we look on the line immediately following the line ending with a bracket "{" (probably followed by whitespaces). Otherwise we could be confused by all sorts of ascii art in the comments in the head of the file.


> No abbreviations in WebKit: const indentRegex =. Also, var firstChar = line.charAt(0); if (firstChar === " " || firstChar === "\t") would be infinitely faster.

I'm not following this one. I could use `firstChar = line.charAt(0); if (firstChar === " " || firstChar === "\t")`, but I have to use RegExp anyway to check an amount of spaces.

[Pavel Feldman]

> I agree, it is confusing, but the idea is "auto-detect indent style used in the file. If auto-detection fails, fall back to the user setting.".
> 
> JS/CSS files may not have any indents. They may be empty or minified. In this case, indent must not be equals empty string. We could fall back to the default indent, but I'm not sure it's a good idea.

I'd fall back to 4 in that case.

> It wouldn't matter, though, if we move away from this solution to the one proposed by Vsevolod:
> 
> "I think we would get more reliable results if we look on the line immediately following the line ending with a bracket "{" (probably followed by whitespaces). Otherwise we could be confused by all sorts of ascii art in the comments in the head of the file.

Not all the languages have {. Imagine we support source mappings and start editing coffeescript.

> > No abbreviations in WebKit: const indentRegex =. Also, var firstChar = line.charAt(0); if (firstChar === " " || firstChar === "\t") would be infinitely faster.
> 
> I'm not following this one. I could use `firstChar = line.charAt(0); if (firstChar === " " || firstChar === "\t")`, but I have to use RegExp anyway to check an amount of spaces.

Yep, but it is way faster for the non-matching lines. Regexes are not that fast.

[Nikita Vasilyev]

Created attachment 111827 [details]
Another attempt

(In reply to comment #6)
> > Source/WebCore/inspector/front-end/SettingsScreen.js:53
> > +    p = this._appendSection(WebInspector.UIString("Editing"));
> 
> Please add all new UIStrings to Source/WebCore/English.lproj/localizedStrings.js.

Done.

(In reply to comment #8)
> (From update of attachment 111511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111511&action=review
> 
> >> Source/WebCore/inspector/front-end/Settings.js:84
> >> +    this.indent = this.createSetting("indent", "    ");
> > 
> > defaultIndent?
> 
> You should create this setting from within the TextEditorModel.

Done.

I'll fix all the other staff you've mentioned in https://bugs.webkit.org/show_bug.cgi?id=70406

[Yury Semikhatsky]

Comment on attachment 111827 [details]
Another attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review

> Source/WebCore/inspector/front-end/SettingsScreen.js:55
> +        [ "    ", WebInspector.UIString("4 spaces") ],

Can we make this a text field with a number of spaces? I can easily imagine a page using 8 spaces indent for instance.

[Nikita Vasilyev]

Created attachment 111926 [details]
N number of spaces mockup

(In reply to comment #12)
> (From update of attachment 111827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review
> 
> > Source/WebCore/inspector/front-end/SettingsScreen.js:55
> > +        [ "    ", WebInspector.UIString("4 spaces") ],
> 
> Can we make this a text field with a number of spaces? I can easily imagine a page using 8 spaces indent for instance.

I've thought to make it like [indent-setting-n-spaces.png], but I haven't found any CSS or JS that uses an indent other than 2/4 spaces or a tab. So I think it would be better to keep UI simpler.

[Vsevolod Vlasov]

(In reply to comment #13)
> Created an attachment (id=111926) [details]
> N number of spaces mockup
> 
> (In reply to comment #12)
> > (From update of attachment 111827 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review
> > 
> > > Source/WebCore/inspector/front-end/SettingsScreen.js:55
> > > +        [ "    ", WebInspector.UIString("4 spaces") ],
> > 
> > Can we make this a text field with a number of spaces? I can easily imagine a page using 8 spaces indent for instance.
> 
> I've thought to make it like [indent-setting-n-spaces.png], but I haven't found any CSS or JS that uses an indent other than 2/4 spaces or a tab. So I think it would be better to keep UI simpler.

8 spaces indentation certainly happens in css and html sometimes.
http://www.google.com/codesearch#search/&q=%22%20%20%20%20%20%20%20%20margin%22%20file:%5C.css&type=cs

(In reply to comment #7)
> Thanks for the review!
> 
> I've made a separate bug for auto-detection feature (https://bugs.webkit.org/show_bug.cgi?id=70406) because auto-detection shouldn't hold 70181.
> 
> I haven't wrote any Inspector's tests yet, I'll dig it.
> 
> >> Source/WebCore/inspector/front-end/Settings.js:84
> >> +    this.indent = this.createSetting("indent", "    ");
> >
> >defaultIndent?
> 
> Yes. I think 4 spaces is the most used indent in JS and CSS.

I meant the name "indent" seems to generic for me. Maybe editorTabIndent?

[Pavel Feldman]

Comment on attachment 111827 [details]
Another attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review

A couple of nits and you are good to go.

>>>> Source/WebCore/inspector/front-end/SettingsScreen.js:55
>>>> +        [ "    ", WebInspector.UIString("4 spaces") ],
>>> 
>>> Can we make this a text field with a number of spaces? I can easily imagine a page using 8 spaces indent for instance.
>> 
>> I've thought to make it like [indent-setting-n-spaces.png], but I haven't found any CSS or JS that uses an indent other than 2/4 spaces or a tab. So I think it would be better to keep UI simpler.
> 
> 8 spaces indentation certainly happens in css and html sometimes.
> http://www.google.com/codesearch#search/&q=%22%20%20%20%20%20%20%20%20margin%22%20file:%5C.css&type=cs
> 
> (In reply to comment #7)

Lets add 8 as well.

> Source/WebCore/inspector/front-end/TextEditorModel.js:70
> +    WebInspector.settings.indent = WebInspector.settings.createSetting(WebInspector.UIString("indent"), "    ");

Nit: you don't need to define this setting on WebInspector.settings object, but given how generic text editor is, this is probably fine. I would make the name more descriptive though: .textEditorIndent?

Please introduce enum for values and use it form within settings screen:

WebInspector.TextEditorModel.Indent = {
    TwoSpaces: "  ",
    TabCharacter: "\t",
...
}

[Nikita Vasilyev]

(In reply to comment #15)
> (From update of attachment 111827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111827&action=review
> 
> A couple of nits and you are good to go.
> 
> >>>> Source/WebCore/inspector/front-end/SettingsScreen.js:55
> >>>> +        [ "    ", WebInspector.UIString("4 spaces") ],
> >>> 
> >>> Can we make this a text field with a number of spaces? I can easily imagine a page using 8 spaces indent for instance.
> >> 
> >> I've thought to make it like [indent-setting-n-spaces.png], but I haven't found any CSS or JS that uses an indent other than 2/4 spaces or a tab. So I think it would be better to keep UI simpler.
> > 
> > 8 spaces indentation certainly happens in css and html sometimes.
> > http://www.google.com/codesearch#search/&q=%22%20%20%20%20%20%20%20%20margin%22%20file:%5C.css&type=cs
> > 
> > (In reply to comment #7)
> 
> Lets add 8 as well.

Are you suggesting to add one more <input type=radio>?

[Pavel Feldman]

> 
> Are you suggesting to add one more <input type=radio>?

You could do <select> as in "Open links in..." setting.

[Nikita Vasilyev]

Created attachment 112037 [details]
"8 spaces" option added. Use <select> instead of <input type=radio>

(In reply to comment #17)
> > 
> > Are you suggesting to add one more <input type=radio>?
> 
> You could do <select> as in "Open links in..." setting.

"Open links in..." select uses WebInspector.HandlerRegistry... I've spent almost couple hours scratching my head trying to figure out how to it make my select work with it. Then I've given up and just made a _createSelectSetting method that looks very similar to _createRadioSetting.

[Pavel Feldman]

(In reply to comment #18)
> Created an attachment (id=112037) [details]
> "8 spaces" option added. Use <select> instead of <input type=radio>
> 
> (In reply to comment #17)
> > > 
> > > Are you suggesting to add one more <input type=radio>?
> > 
> > You could do <select> as in "Open links in..." setting.
> 
> "Open links in..." select uses WebInspector.HandlerRegistry... I've spent almost couple hours scratching my head trying to figure out how to it make my select work with it. Then I've given up and just made a _createSelectSetting method that looks very similar to _createRadioSetting.

Sorry, I was referring to the ui, not the code. HandlerRegistry is a nasty indirection we need to cure.

[Pavel Feldman]

Comment on attachment 112037 [details]
"8 spaces" option added. Use <select> instead of <input type=radio>

View in context: https://bugs.webkit.org/attachment.cgi?id=112037&action=review

Code looks good, thanks for doing this. Please remove the tests OOPS line and attach a screenshot so that I could cq+.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

Please remove this line.

> Source/WebCore/inspector/front-end/SettingsScreen.js:140
> +            if (settingValue == option[0])

use ===

[Nikita Vasilyev]

Created attachment 112055 [details]
Use === instead of ==

[Nikita Vasilyev]

Created attachment 112056 [details]
Mockup: select

[Pavel Feldman]

Comment on attachment 112055 [details]
Use === instead of ==

Thanks for doing this. We will need to remove line break between label and select both for "open in" and this "tab" settings.

[Nikita Vasilyev]

(In reply to comment #23)
> (From update of attachment 112055 [details])
> Thanks for doing this. We will need to remove line break between label and select both for "open in" and this "tab" settings.

Should I do it in this bug? Is that the reason why the path isn't in a commit queue yet?

[Pavel Feldman]

Committed r98236: <http://trac.webkit.org/changeset/98236>