Bug 158676 - Web Inspector: Add ability to show/hide DataGird columns
Summary: Web Inspector: Add ability to show/hide DataGird columns
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
Keywords: InRadar
Depends on:
Blocks: 158675
  Show dependency treegraph
Reported: 2016-06-12 15:19 PDT by Matt Baker
Modified: 2016-06-13 16:31 PDT (History)
7 users (show)

See Also:

[Video] New UI (1.27 MB, video/mp4)
2016-06-12 15:19 PDT, Matt Baker
no flags Details
[Patch] Proposed Fix (15.39 KB, patch)
2016-06-12 15:39 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-06-12 15:19:02 PDT
Add ability to show/hide DataGird columns. In OS X Finder, the grid header's context menu has checkbox items for each (eligible) column.

 - 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
Comment 1 Radar WebKit Bug Importer 2016-06-12 15:19:17 PDT
Comment 2 Matt Baker 2016-06-12 15:19:40 PDT
Created attachment 281130 [details]
[Video] New UI
Comment 3 Matt Baker 2016-06-12 15:39:07 PDT
Created attachment 281132 [details]
[Patch] Proposed Fix
Comment 4 WebKit Commit Bot 2016-06-13 15:40:18 PDT
Comment on attachment 281132 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 281132

Committed r202009: <http://trac.webkit.org/changeset/202009>
Comment 5 WebKit Commit Bot 2016-06-13 15:40:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Joseph Pecoraro 2016-06-13 16:31:00 PDT
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.
Comment 7 Joseph Pecoraro 2016-06-13 16:31:30 PDT
(In reply to comment #2)
> Created attachment 281130 [details]
> [Video] New UI

Looks good!