Bug 152626 – Web Inspector: Timelines view doesn't remember how I like to sort things

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

[Saam Barati]

I like to sort things by "Total Time". But, every time I open/close or just refresh the inspector, it defaults to sorting things by "Start Time".
It would be nice if this preference was remembered.

[Radar WebKit Bug Importer]

<rdar://problem/24029666>

[Devin Rousso]

Created attachment 268068 [details]
Patch

[Devin Rousso]

Oh crap sorry @Matt I didn't notice that this was assigned to you.  My bad.

[Matt Baker]

No problem.

[Blaze Burg]

Comment on attachment 268068 [details]
Patch

r=me

In general I would prefer to give each data grid a well-known id, and let it handle the details of saving to a setting. However, there are no other uses for such an identifier (CSS id comes to mind), so this approach is fine for the time being.

[Joseph Pecoraro]

Comment on attachment 268068 [details]
Patch

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

Some comments above. I don't think any immediate concerns though, but long term they should be addressed.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:809
> -        var dataGrid = WebInspector.DataGrid.createSortableDataGrid(columnNames, flatValues);
> +        let dataGrid = WebInspector.DataGrid.createSortableDataGrid(columnNames, flatValues, "console-message-view-sort");

This one may be a bit too general. Each `console.table` will have a new Setting with the same conflicting identifier, which would cause conflicts. Not to mention each console.table saving some value to persistent storage if the user changes the sort.

Might be best to not include a setting here, or having it just be a Temporary / In Memory Only Setting. (Maybe a new WebInspector.Setting subclass).

> Source/WebInspectorUI/UserInterface/Views/DatabaseTableContentView.js:89
> -            this._dataGrid = WebInspector.DataGrid.createSortableDataGrid(columnNames, values);
> +            this._dataGrid = WebInspector.DataGrid.createSortableDataGrid(columnNames, values, "database-table-content-view-sort");

Same here. Unless we wanted to make the identifier more specific, "database-table-content-view-sort-{table-name}". But still, this might be overkill.

[Devin Rousso]

Created attachment 268251 [details]
Patch

So I reverted some of the changes to make it so that the setting object is "optional", in that the DataGrid will still work without it.

[Joseph Pecoraro]

Comment on attachment 268251 [details]
Patch

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

r=me!

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:238
> +        if (this.sortColumnIdentifier) {
> +            let newSortHeaderCellElement = this._headerTableCellElements.get(this.sortColumnIdentifier);

Since the getter now does nothing these two lines can just access the instance variable directly (_sortColumnIdentifier).

[Devin Rousso]

Created attachment 268267 [details]
Patch

(In reply to comment #8)
> > Source/WebInspectorUI/UserInterface/Views/DataGrid.js:238
> > +        if (this.sortColumnIdentifier) {
> > +            let newSortHeaderCellElement = this._headerTableCellElements.get(this.sortColumnIdentifier);
> 
> Since the getter now does nothing these two lines can just access the
> instance variable directly (_sortColumnIdentifier).

Oops.  Fixed it.

[WebKit Commit Bot]

Comment on attachment 268267 [details]
Patch

Clearing flags on attachment: 268267

Committed r194574: <http://trac.webkit.org/changeset/194574>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Timothy Hatcher]

Comment on attachment 268267 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ApplicationCacheFrameContentView.js:166
>          this._dataGrid.sortOrder = WebInspector.DataGrid.SortOrder.Ascending;

You remember the column, but not the sort order. It would be good to remember both.

[Devin Rousso]

Comment on attachment 268267 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ApplicationCacheFrameContentView.js:166
>>          this._dataGrid.sortOrder = WebInspector.DataGrid.SortOrder.Ascending;
> 
> You remember the column, but not the sort order. It would be good to remember both.

Just made another bug for this: https://bugs.webkit.org/show_bug.cgi?id=152811