Bug 69986 - Web Inspector: Make indent configurable
Summary: Web Inspector: Make indent configurable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords:
Depends on:
Blocks: 70181 70406
  Show dependency treegraph
 
Reported: 2011-10-12 18:39 PDT by Nikita Vasilyev
Modified: 2011-10-24 06:52 PDT (History)
12 users (show)

See Also:


Attachments
Indent settings mockup (50.98 KB, image/png)
2011-10-12 18:39 PDT, Nikita Vasilyev
no flags Details
My first take on this. No auto-detection for now. Will make a separate bug. (3.57 KB, patch)
2011-10-16 15:19 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Indent auto-detection has been added (6.29 KB, patch)
2011-10-18 15:41 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Another attempt (3.89 KB, patch)
2011-10-20 12:29 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
N number of spaces mockup (53.92 KB, image/png)
2011-10-21 02:11 PDT, Nikita Vasilyev
no flags Details
"8 spaces" option added. Use <select> instead of <input type=radio> (5.50 KB, patch)
2011-10-21 15:45 PDT, Nikita Vasilyev
pfeldman: review+
Details | Formatted Diff | Diff
Use === instead of == (5.46 KB, patch)
2011-10-21 18:59 PDT, Nikita Vasilyev
pfeldman: review+
Details | Formatted Diff | Diff
Mockup: select (53.37 KB, image/png)
2011-10-21 19:01 PDT, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2011-10-12 18:39:55 PDT
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.
Comment 1 Nikita Vasilyev 2011-10-16 07:24:50 PDT
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.
Comment 2 Pavel Feldman 2011-10-16 10:07:20 PDT
> 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).
Comment 3 Nikita Vasilyev 2011-10-16 13:40:33 PDT
(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?
Comment 4 Nikita Vasilyev 2011-10-16 15:19:02 PDT
Created attachment 111187 [details]
My first take on this. No auto-detection for now. Will make a separate bug.
Comment 5 Nikita Vasilyev 2011-10-18 15:41:21 PDT
Created attachment 111511 [details]
Indent auto-detection has been added

Can I have a review, please?
Comment 6 Vsevolod Vlasov 2011-10-18 17:25:42 PDT
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).
Comment 7 Nikita Vasilyev 2011-10-19 02:28:15 PDT
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.
Comment 8 Pavel Feldman 2011-10-19 02:51:40 PDT
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.
Comment 9 Nikita Vasilyev 2011-10-19 03:59:09 PDT
(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.
Comment 10 Pavel Feldman 2011-10-19 04:56:51 PDT
> 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.
Comment 11 Nikita Vasilyev 2011-10-20 12:29:05 PDT
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
Comment 12 Yury Semikhatsky 2011-10-21 01:12:10 PDT
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.
Comment 13 Nikita Vasilyev 2011-10-21 02:11:27 PDT
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.
Comment 14 Vsevolod Vlasov 2011-10-21 04:21:42 PDT
(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?
Comment 15 Pavel Feldman 2011-10-21 12:16:30 PDT
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",
...
}
Comment 16 Nikita Vasilyev 2011-10-21 13:18:12 PDT
(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>?
Comment 17 Pavel Feldman 2011-10-21 13:22:28 PDT
> 
> Are you suggesting to add one more <input type=radio>?

You could do <select> as in "Open links in..." setting.
Comment 18 Nikita Vasilyev 2011-10-21 15:45:18 PDT
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.
Comment 19 Pavel Feldman 2011-10-21 15:54:09 PDT
(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.
Comment 20 Pavel Feldman 2011-10-21 15:57:23 PDT
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 ===
Comment 21 Nikita Vasilyev 2011-10-21 18:59:50 PDT
Created attachment 112055 [details]
Use === instead of ==
Comment 22 Nikita Vasilyev 2011-10-21 19:01:15 PDT
Created attachment 112056 [details]
Mockup: select
Comment 23 Pavel Feldman 2011-10-22 11:58:59 PDT
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.
Comment 24 Nikita Vasilyev 2011-10-24 05:02:03 PDT
(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?
Comment 25 Pavel Feldman 2011-10-24 06:52:02 PDT
Committed r98236: <http://trac.webkit.org/changeset/98236>