Bug 149120 – Web Inspector: Preferences for Text Editor behavior

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
When adding a location for global preferences, some useful preferences would be to modify Text Editor behavior:

Some examples would be:

  - tab/indent size (default 4 spaces)
  - line wrapping (default off)
  - show whitespace characters (default off)

[Radar WebKit Bug Importer]

<rdar://problem/22686894>

[Devin Rousso]

Created attachment 288484 [details]
Patch

[Devin Rousso]

Created attachment 288485 [details]
[Image] After Patch is applied

Obviously not a perfect representation, but it's a start :)

[Nikita Vasilyev]

Created attachment 288490 [details]
[Image] Sublime Text indentation settings

Sublime Text, among many other code editors, has text indentation settings at the top bottom corner of the editor window.

I'd prefer Web Inspector to have a similar settings menu at the top left corner, right before "{}" and [T] icons.

[Nikita Vasilyev]

(In reply to comment #3)
> Created attachment 288485 [details]
> [Image] After Patch is applied
> 
> Obviously not a perfect representation, but it's a start :)

I don't think any of settings on the screenshot should be in the settings tab.
I think these settings should be accessible from a text editor view.

I don't think we should have a setting to hide line numbers. They don't take too much space. Besides, where do we show breakpoints?

I think we should always match brackets (and parenthesis). I don't think we should have a setting for this.

This doesn't allow to change tab character width. See Sublime Text settings above. Tab character width can be controlled via `tab-size` CSS property. We currently use `tab-size: 4`.

[Devin Rousso]

(In reply to comment #5)
> I don't think we should have a setting to hide line numbers. They don't take
> too much space. Besides, where do we show breakpoints?

Good point.  I did not think of that.

> I think we should always match brackets (and parenthesis). I don't think we
> should have a setting for this.

Yeah, that's fair.  I just figured that if we were adding settings we may as well let the user configure what they wanted.  But this may be a bit much.

> This doesn't allow to change tab character width. See Sublime Text settings
> above. Tab character width can be controlled via `tab-size` CSS property. We
> currently use `tab-size: 4`.

I completely forgot about "tab-size"!  I don't agree that we should display it on the TextEditor view, however, since I don't think we have the space for it.  Considering how many items are already in the navigation bar (Navigation sidebar, back/forward, Path components, {}, [T], [C], Details sidebar), there isn't much room for another editor, especially when both sidebars are shown (I often see the Path components be truncated on my 21" monitor with WebInspector already being half the page).  I don't think that many user's will want to change indentation settings per file (which would introduce it's own set of complications), so I think it makes more sense to keep these settings in SettingsTabContentView.

[Devin Rousso]

Created attachment 288524 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #6)
> I don't agree that we should display
> it on the TextEditor view, however, since I don't think we have the space
> for it.  Considering how many items are already in the navigation bar
> (Navigation sidebar, back/forward, Path components, {}, [T], [C], Details
> sidebar), there isn't much room for another editor, especially when both
> sidebars are shown (I often see the Path components be truncated on my 21"
> monitor with WebInspector already being half the page).  I don't think that
> many user's will want to change indentation settings per file (which would
> introduce it's own set of complications), so I think it makes more sense to
> keep these settings in SettingsTabContentView.

Screenshot from IRC: https://i.imgur.com/4ZkH921.png

As a first iteration, I think it's fine to have these preferences in the Settings tab.
If we find some of them commonly used (e.g. text wrapping), we may pluck them from
the Settings tab and move to the editor's navigation bar.

[Devin Rousso]

Created attachment 288752 [details]
Patch

So I tried reworking _generateCallFramesSection to only resolve once every scope has updated all of its properties (thus only creating DetailsSection instances if a scope has at least one property), but it causes a noticeable delay to the rendering of the scope chain.  This is most likely due to the sheer number of calls to resolve PropertyDescriptor values for RemoteObject.  I think that a current "middle-ground" solution may be to have placeholder elements be added to the DOM (basically blank <div>) that we can remove if there are no properties or replace with a DetailsSection.

[Devin Rousso]

Comment on attachment 288752 [details]
Patch

OK whoops totally uploaded this patch to the wrong bug ⚆_⚆

[Joseph Pecoraro]

Comment on attachment 288524 [details]
Patch

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

r- because of the leak.

I didn't look closely at the refactoring for TabItems, so that will need a closer review.

> Source/WebInspectorUI/ChangeLog:8
> +        * Localizations/en.lproj/localizedStrings.js:

LocalizedStrings should not be treated as binary. Either someone checked in a non-utf8 version, or you updated it with a non-utf8 version.

> Source/WebInspectorUI/ChangeLog:45
> +        * UserInterface/Views/ConsoleTabContentView.js:
> +        (WebInspector.ConsoleTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.
> +
> +        * UserInterface/Views/DebuggerTabContentView.js:
> +        (WebInspector.DebuggerTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.
> +
> +        * UserInterface/Views/ElementsTabContentView.js:
> +        (WebInspector.ElementsTabContentView):
> +        Now uses WebInspector.GeneralTabBarItem.

I would combine the 8 repeats of this into one group with the 1 comment to avoid the duplication and wasted newlines. Everyone has their own ChangeLog style. I suggest not being afraid to change the order of lines in here to make an easier to read and still be useful for reviewers.

> Source/WebInspectorUI/UserInterface/Base/Setting.js:107
> +WebInspector.settings = Object.create(null);
> +WebInspector.settings.enableLineWrapping = new WebInspector.Setting("enable-line-wrapping", false);
> +WebInspector.settings.indentUnit = new WebInspector.Setting("indent-unit", 4);

For this kind of thing I think it reads easier without the duplication:

    WebInspector.settings = {
        enableLineWrapping: new WebInspector.Setting(...),
        ...
    };

The Object.create(null) is probably overkill here, since we won't be looking up keys in the object and have to avoid things like "toString".

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:74
> +            let editorContainer = container.createChild("div", "setting-editor");

With all the TextEditor stuff, it took me a while to realize that the use of "editor" in here wasn't a TextEditor. Maybe we should call it a "control", in common with "form control". Or "widget".

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:54
> +        WebInspector.settings.indentWithTabs.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("indentWithTabs", WebInspector.settings.indentWithTabs.value);
> +        });
> +
> +        WebInspector.settings.indentUnit.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("indentUnit", WebInspector.settings.indentUnit.value);
> +        });
> +
> +        WebInspector.settings.enableLineWrapping.addEventListener(WebInspector.Setting.Event.Changed, (event) => {
> +            this._codeMirror.setOption("lineWrapping", WebInspector.settings.enableLineWrapping.value);
> +        });

This would cause every TextEditor that is created to never be garbage collected.

A WebInspector.Object's list of event listeners & targets are lists of strong references. When a View instance adds itself as an event listener on a _global object_. It should remember to remove itself as an event listener from that global object when the View is closed. Otherwise, the View will never be shown again, but will continue to be kept alive because the global object stays alive, and its list of listeners contains the non-visible View.

Here the global WebInspector.Object are each of the `WebInspector.settings.X`. And the listener function captures `this` keeping the `TextEditor` view alive.

For ContentViews we solve this through a very easy pattern. The ContentView interface has shown(), hidden(), closed() methods to be called during the view's lifetime. TextEditor is not a ContentView, but it is always owned by ContentViews. And those ContentViews should appropriately be calling textEditor.close() in closed(). And it looks like they do. However, TextContentView calls _textEditor.close() which doesn't even exist, so that would be a runtime error right now!

Lets do a few things:

  1. Add a TextEditor.prototype.close to detach these listeners
  2. Appropriately call super.close() in SourceCodeTextEditor.prototype.close
  3. Ideally we would update some of the ContentView's closed() methods to call super.closed() even though the base is empty.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:127
> +        let linePrefixText = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            linePrefixText = "\t";

Maybe we should have a global helper for this, since it seems like it is and will be a common operation:

    WebInspector.indentString = function()
    {
        if (WebInspector.settings.indentWithTabs.value)
            return "\t";
        return " ".repeat(WebInspector.settings.indentUnit.value);
    }

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:35
> +        if (!indentString) {
> +            if (WebInspector.settings.indentWithTabs.value)

This is incorrect. EsprimaFormatter lives in a Worker, and does not have access to the WebInspector namespace.

In this code, this should remain a parameter with a reasonable default. In WebInspector land, all of the callers use workerProxy.formatJavaScript(...). They should consult the setting for the appropriate indentString (which I see they do above).

I suppose our tests all pass because they provide an indentString to formatJavaScript. However the WebInspectorUI/Tools/Formatting/index.html tool would probably fail to pretty print anything. Either way, easy fix by just not changing this file.

[Devin Rousso]

Created attachment 288871 [details]
Patch

[Devin Rousso]

Created attachment 288873 [details]
[Image] After Patch is applied

[Build Bot]

Comment on attachment 288871 [details]
Patch

Attachment 288871 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2074939

New failing tests:
inspector/css/generate-css-rule-string.html

[Build Bot]

Created attachment 288887 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288871 [details]
Patch

Attachment 288871 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2074944

New failing tests:
inspector/css/generate-css-rule-string.html

[Build Bot]

Created attachment 288889 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Devin Rousso]

Created attachment 288890 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 288890 [details]
Patch

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

r- for the Tab Bar issue.

When I apply this there are very weird issues with when the New Tab button and the Settings Tab button are showing. They both always occupy the same space at the right of the tab bar. So if the button is the settings tab button, to instead get the new tab button you have to click the gear and then the new tab button appears. Likewise if the button is the new tab button but you want the settings tab button. This is definitely confusing. Both should always be visible.

In old demos from Tim Hatcher (April 2015) the setting tab button was pinned to the left, and worked fine with tab modifications. I think that makes a lot of sense.

Also, I think clicking the setting tab button should just show the SettingTabContentView and highlight the Setting tab button as the actively selected tab. It should not a new Setting tab and select that tab. I think this is very important. If the user wants to go to settings that should always be in the same spot and behave as they expect (the always available tab at the left, the settings gear button). If we start making it a "tab" then the user needs to search their list of tabs for the setting tab, and we are introducing a lot of unnecessary friction.

For this reason I avoided reviewing the GeneralTabItem / PinnedTabBarItem refactoring, because I don't think its giving us the behavior we want. We want the Settings tab bar item to behave like a normal tab bar item, but with a few special properties: (1) always be pinned to the left, (2) always have the small style of just the icon, (3) the tab bar item cannot be "closed" the user should just switch to another tab. Note we are always guaranteed to have at least one other tab (the new tab tab).

---

Thanks for adding super across ContentViews for shown/hidden/closed. I wonder if that fixes any subtle bugs!

> Source/WebInspectorUI/UserInterface/Base/Main.js:233
> +    function setTabSize() {
> +        document.body.style.tabSize = WebInspector.settings.indentUnit.value;
> +    }
> +    WebInspector.settings.indentUnit.addEventListener(WebInspector.Setting.Event.Changed, setTabSize);
> +    setTabSize();

It is not clear to me what the intent of what this setting is for.

CodeMirror has a few settings:
https://codemirror.net/doc/manual.html

    - tabSize - the number of spaces a tab represents
    - indentUnit - the number of spaces to auto-indent when editing and you insert a newline and expect indentation
    - indentWithTabs - with the auto-indent when editing, replace the leading groups of `tabSize` spaces with a tab character.

Setting the tabSize on document.body does not affect CodeMirror. It affects anywhere we show code (Debugger Popover for a Function) that is not in a CodeMirror editor.

Likewise with a setting named "Indent width" it is not clear to me that this is actually the tab width. I think we can assume safely that users mostly expect their indentation to be a single tab. So we could get away with a single setting ("Tab width"). But maybe we should have settings for both.

Xcode even breaks this out into 3 options:

    Prefer ident using (Spaces | Tabs)
    Tab width: 4
    Indent width: 4

> Source/WebInspectorUI/UserInterface/Base/Main.js:1053
> +WebInspector.indentString = function() {

Style: Brace on newline.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:361
> +    CodeMirror.defineOption("showWhitespaceCharacters", false, function(cm, val, old) {

Style: We shouldn't abbreviate names like "val". That said, "cm" is fine, its canonical.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:55
> +            console.error("Unkown PinnedTabBarItem mode");

Typo: Unkown

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:817
> +        let indentString = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            indentString = "\t";

Nit: let indentString = WebInspector.indentString();

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:839
> +        let indentString = " ".repeat(WebInspector.settings.indentUnit.value);
> +        if (WebInspector.settings.indentWithTabs.value)
> +            indentString = "\t";

Nit: let indentString = WebInspector.indentString();

[Devin Rousso]

Comment on attachment 288890 [details]
Patch

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

I agree that the action to view settings is very simple, but I don't think we always want to display a gear icon on a pinned settings tab.  I think that users won't change their settings that often, so making it always visible would just clutter things.

Also, do you have a link to the mock from Tim?  I think it may help to just be able to see what he was talking about :)

>> Source/WebInspectorUI/UserInterface/Base/Main.js:233
>> +    setTabSize();
> 
> It is not clear to me what the intent of what this setting is for.
> 
> CodeMirror has a few settings:
> https://codemirror.net/doc/manual.html
> 
>     - tabSize - the number of spaces a tab represents
>     - indentUnit - the number of spaces to auto-indent when editing and you insert a newline and expect indentation
>     - indentWithTabs - with the auto-indent when editing, replace the leading groups of `tabSize` spaces with a tab character.
> 
> Setting the tabSize on document.body does not affect CodeMirror. It affects anywhere we show code (Debugger Popover for a Function) that is not in a CodeMirror editor.
> 
> Likewise with a setting named "Indent width" it is not clear to me that this is actually the tab width. I think we can assume safely that users mostly expect their indentation to be a single tab. So we could get away with a single setting ("Tab width"). But maybe we should have settings for both.
> 
> Xcode even breaks this out into 3 options:
> 
>     Prefer ident using (Spaces | Tabs)
>     Tab width: 4
>     Indent width: 4

Clearly I misunderstood this, because I thought that CodeMirror used actual tab characters.  It seems like it inserts the corresponding number of spaces as the "tabSize" option value.  I think your suggestion makes sense :)

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:361
>> +    CodeMirror.defineOption("showWhitespaceCharacters", false, function(cm, val, old) {
> 
> Style: We shouldn't abbreviate names like "val". That said, "cm" is fine, its canonical.

I was following the signature defined in codemirror.js, but I agree.  I'll change it.

[Devin Rousso]

Created attachment 289019 [details]
Patch

[Devin Rousso]

Created attachment 289020 [details]
[Image] After Patch is applied

[Matt Baker]

(In reply to comment #22)
> Created attachment 289020 [details]
> [Image] After Patch is applied

Looking very nice! A couple things:

- The gear should be on the right, so that it isn't given so much priority in the UI.
- Simple on/off settings don't need ": Visible" text (see screenshot)

[Matt Baker]

Created attachment 289022 [details]
[Image] Simplified on/off settings

Also added 6px margin between checkbox and label.

[Matt Baker]

(In reply to comment #24)
> Created attachment 289022 [details]
> [Image] Simplified on/off settings
> 
> Also added 6px margin between checkbox and label.

I'm not convinced we should use a two column layout, where labels are right aligned and values left aligned.

[Devin Rousso]

(In reply to comment #25)
> (In reply to comment #24)
> > Created attachment 289022 [details]
> > [Image] Simplified on/off settings
> > 
> > Also added 6px margin between checkbox and label.
> 
> I'm not convinced we should use a two column layout, where labels are right
> aligned and values left aligned.

I am trying to follow the Syntax/Indentation settings in Xcode.  The issue is that Xcode has so many more settings (that are usually grouped by the text on the left) that we don't have (either we can't do it because of CodeMirror or because it isn't very useful).

[Devin Rousso]

Created attachment 289044 [details]
[Image] Settings icon on the right

So I tried just moving the element in the DOM tree to the left (I'm lazy and tired...sue me), and I (surprisingly) don't like how this looks.  I think that having two pinned tab icons next to each-other is very confusing.

Also, the logistics of getting another pinned tab that actually represents a ContentView and not just a button onto the tab bar (and to the right of the new tab item) looks to be harder than I thought.

I think it makes the most sense to either leave the settings where it currently is (on the left) or switch the positions of the two pinned tab items (new tab is left and settings is right).

[Devin Rousso]

Created attachment 292292 [details]
Patch

[Devin Rousso]

Created attachment 292976 [details]
Patch

[Timothy Hatcher]

Comment on attachment 292976 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:433
> +    if (openTabTypes.includes("settings")) {
> +        openTabTypes.remove("settings");
> +        this._openTabsSetting.value = openTabTypes;
> +    }

Is this still needed?

> Source/WebInspectorUI/UserInterface/Base/Main.js:1041
> +

Can remove this line sine the return comes after a conditional return.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:60
> +    static modeInfo(mode)
> +    {
> +        switch (mode) {
> +        case WebInspector.PinnedTabBarItem.Mode.NewTab:
> +            return {
> +                image: "Images/NewTabPlus.svg",
> +                title: WebInspector.UIString("Create a new tab"),
> +            };
> +
> +        case WebInspector.PinnedTabBarItem.Mode.Settings:
> +            return {
> +                image: "Images/Gear.svg",
> +                title: WebInspector.UIString("Open Settings"),
> +            };
> +
> +        default:
> +            console.error("Unknown PinnedTabBarItem mode");
> +            return {};
> +        }
> +    }

This is kind of a layering violation. These details could be passed into the constructor from the separate views or whatever.

> Source/WebInspectorUI/UserInterface/Views/PinnedTabBarItem.js:73
> +        if (this._mode === WebInspector.PinnedTabBarItem.Mode.NewTab) {
> +            const shouldAnimate = true;
> +            WebInspector.showNewTabTab(shouldAnimate);
> +        }

The only real use of mode seems to be here. This could be handled externally with an event listener.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:198
> +        let wasLastNormalTab = this._tabBarItems.indexOf(tabBarItem) === this._tabBarItems.length - 3;

This magic number should have a comment or ideally be computed. I know the old code was also a little magical too.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:348
> +            tabBarItem = this._tabBarItems[this._tabBarItems.length - 3];

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:648
> +        newIndex = Math.min(this._tabBarItems.length - 3, newIndex);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:156
> +        console.assert(this._recentTabContentViews.length === this._tabBar.tabBarItems.length - 2);

It would be good to have a central place to check this and not have these numbers sprinkled around. Why do we not need the check for this._tabBar.newTabItem anymore?

[Devin Rousso]

Comment on attachment 292976 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:156
>> +        console.assert(this._recentTabContentViews.length === this._tabBar.tabBarItems.length - 2);
> 
> It would be good to have a central place to check this and not have these numbers sprinkled around. Why do we not need the check for this._tabBar.newTabItem anymore?

The checks for newTabItem were originally used to check whether there were any pinned tabs in the TabBar.  Since the newTabItem was given to the TabBar by its "parent" TabBrowser, it wasn't guaranteed that it existed on tabBar.  Now, since it is always created by the TabBar, we just need to check whether the number of recent (meaning opened) tabs is equal to the number of normal tab bar items inside the TabBar.  Since there are two pinned tab bar items, I subtract 2 from that count.

[Devin Rousso]

Created attachment 293255 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 293255 [details]
Patch

Rejecting attachment 293255 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 293255, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
InspectorUI/UserInterface/Views/TabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TextEditor.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2396304

[Devin Rousso]

Created attachment 293259 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 293259 [details]
Patch

Clearing flags on attachment: 293259

Committed r208091: <http://trac.webkit.org/changeset/208091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 293259 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorOverrides.css:178
> +.show-invalid-characters .CodeMirror .cm-invalidchar {
>      display: none;
>  }

This appears to be the inverse of what we want. I'll fix this up.