Bug 158676 – Web Inspector: Add ability to show/hide DataGird columns

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Add ability to show/hide DataGird columns. In OS X Finder, the grid header's context menu has checkbox items for each (eligible) column.

* NOTES
 - It should be possible to disallow showing/hiding certain columns (e.g., the "Name" column in timeline grids)
 - Hidden columns should be persisted in a setting

[Radar WebKit Bug Importer]

<rdar://problem/26761573>

[Matt Baker]

Created attachment 281130 [details]
[Video] New UI

[Matt Baker]

Created attachment 281132 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 281132 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 281132

Committed r202009: <http://trac.webkit.org/changeset/202009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 281132 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:222
> +        // FIXME: Add sortColumnIdentifierSetting and sortOrderSetting as part of <webkit.org/b/158675>.

Same comment regarding FIXMEs with bugzilla links (seen below).

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:233
> +    get columnChooserEnabled() { return this._columnChooserEnabled; }
> +    set columnChooserEnabled(x) { this._columnChooserEnabled = x; }

We should initialize this in the constructor to false, for clarity.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:901
> +    showColumn(columnIdentifier, visible)

Hmm, is showColumn(id, false) really hiding the column? If so, we should rename this:

    setColumnVisible(columnIdentifier, visible)

That way its less of a mystery when "showColumn" gets called and its not shown.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1614
> +        if (!this._columnChooserEnabled)
> +            return;

Given that this method adds a bunch of things opportunistically to the context menu, we should probably avoid the early return style and nest everything in ifs, like you did with column.sortable above. That way when the next person adds something they won't need to indent everything.

> Source/WebInspectorUI/UserInterface/Views/LayoutTimelineView.js:82
> +        // FIXME: Remove once <webkit.org/b/158675> is fixed.

I think a better way to do this is to just include the bug and a description if needed:

    // FIXME: <https://webkit.org/b/158675> Web Inspector: DataGrid should manage its own settings

That way I know what the bugzilla bug is without having to follow the link.

[Joseph Pecoraro]

(In reply to comment #2)
> Created attachment 281130 [details]
> [Video] New UI

Looks good!