Bug 154050 – Web Inspector: DataGrid Header Cells should have Context Menu for Sorting

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

[Joseph Pecoraro]

* SUMMARY
DataGrid Header Cells should have Context Menu for Sorting

* STEPS TO REPRODUCE
1. Inspect this page
2. Show Network tab
3. Right click a DataGrid header
  => expected sort options

* NOTES
- Should check if column is sortable

[Devin Rousso]

Created attachment 271270 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24642716>

[Radar WebKit Bug Importer]

<rdar://problem/24642717>

[Joseph Pecoraro]

Comment on attachment 271270 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:956
> +    _selectRowWithSortOrder(columnIdentifier, forcedSortOrder)
> +    {
> +        let determinedSortOrder;
> +        if (this.sortColumnIdentifier === columnIdentifier) {
> +            if (this.sortOrder !== WebInspector.DataGrid.SortOrder.Descending)
> +                determinedSortOrder = WebInspector.DataGrid.SortOrder.Descending;
> +            else
> +                determinedSortOrder = WebInspector.DataGrid.SortOrder.Ascending;
> +        } else
> +            this.sortColumnIdentifier = columnIdentifier;
> +
> +        this.sortOrder = forcedSortOrder || determinedSortOrder;
> +    }

I think readability of this function is really poor.

After reading the code below, the "toggling" case should really only happen in the clicked code and doesn't need to be here.

I'd recommend something like:

    _toggledSortOrder()
    {
        return this._sortOrder !== WebInspector.DataGrid.SortOrder.Descending ? WebInspector.DataGrid.SortOrder.Descending : WebInspector.DataGrid.SortOrder.Ascending;
    }

    _selectSortColumnAndSetOrder(columnIdentifier, sortOrder)
   {
        this.sortColumnIdentifier = columnIdentifier;
        this.sortOrder = sortOrder;
   }

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1068
> +        this._selectRowWithSortOrder(cell.columnIdentifier);

Where this would be:

    let sortOrder = this._sortColumnIdentifier === cell.columnIdentifier ? this._toggledSortOrder() : this.sortOrder;
    this._selectSortColumnAndSetOrder(cell.columnIdentifier, sortOrder);

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1189
> +        if (cell && cell.columnIdentifier && cell.classList.contains(WebInspector.DataGrid.SortableColumnStyleClassName)) {

I think inside this block you should append a separator, to put the sort options in their own group.

    contextMenu.appendSeparator()

Note, that OS X Mail has a "Sort By > ..." submenu. That could also be added here, with a Check Box next to the current sorted column and a Check Box next to Ascending / Descending. But what you just implemented was exactly what I was thinking when I filed this bug.

[Devin Rousso]

Created attachment 271276 [details]
Patch

Nice suggestion Joe!

[WebKit Commit Bot]

Comment on attachment 271276 [details]
Patch

Clearing flags on attachment: 271276

Committed r196549: <http://trac.webkit.org/changeset/196549>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> Nice suggestion Joe!

I was super stoked to see this get fixed. I filed it intending to fix it quickly and totally forgot about it. Then I saw you had a patch up and couldn't stop smiling =). It was exactly what I was looking for!