Bug 191977 – Web Inspector: Table selection should be handled by a SelectionController

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

[Matt Baker]

Summary:
Table selection should be handled by a SelectionController. This is a step toward adding multiple-selection to TreeOutline in https://webkit.org/b/191483.

Notes:
The controller and its delegate (a Table or TreeOutline):

class SelectionController
{
    constructor(delegate)

    get allowsMultipleSelection()
    set allowsMultipleSelection(flag)

    get lastSelectedItem()
    get selectedItems()

    get numberOfItems()

    hasSelectedItem(index)
    selectItem(index, extendSelection = false)
    deselectItem(index)

    selectAll()
    deselectAll()
    removeSelectedItems()

    clear()

    didInsertItem(index);
    didRemoveItem(index);

    handleKeyDown(event)
    handleItemMouseDown(index, event)    
}

class SelectionControllerDelegate
{
    // Required

    selectionControllerNumberOfItems(controller)
    selectionControllerNextSelectableIndex(controller, index)
    selectionControllerPreviousSelectableIndex(controller, index)

    // Optional

    selectionControllerSelectionDidChange(controller, deselectedItems, selectedItems)
}

[Radar WebKit Bug Importer]

<rdar://problem/46253093>

[Matt Baker]

Created attachment 355667 [details]
Patch

[EWS Watchlist]

Comment on attachment 355667 [details]
Patch

Attachment 355667 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10157964

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 355673 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 355667 [details]
Patch

Attachment 355667 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10157983

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 355667 [details]
Patch

Attachment 355667 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10158190

New failing tests:
inspector/table/table-remove-rows.html

[EWS Watchlist]

Created attachment 355678 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Matt Baker]

Comment on attachment 355667 [details]
Patch

r-. Need to fix Table.prototype._removeRows.

[Devin Rousso]

Comment on attachment 355667 [details]
Patch

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

r-, due to test failures (as discussed offline), and the lack of handling item addition/removal (e.g. `didInsertItem` and `didRemoveItem`)

Otherwise, looks good!  Was there a reason you decided on going forward with an index-based approach vs an object-based approach, like we discussed before Thanksgiving?  I think the object-based approach will be much more flexible for `WI.TreeOutline` and would likely require far less work to implement vs an index-based approach.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:33
> +        this._delegate = delegate;

NIT: would this really be considered a delegate?  Perhaps "view" or something else like that to better explain that this is the "controlled object"?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:42
> +        console.assert(this._delegate.selectionControllerNextSelectableIndex, "SelectionController delegate must implement selectionControllerNextSelectableIndex.");

`selectionControllerPreviousSelectableIndex`?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:70
> +    get lastSelectedItem() { return this._lastSelectedIndex; }
> +    get selectedItems() { return this._selectedIndexes; }

Style: put all one-line getters at the top of the Public section

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:149
> +        this._lastSelectedIndex = newSelectedItems.lastIndex;

Should we also treat the last index as the anchor for shift-selection?  This seems to be how Finder does it.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:187
> +    clear()

NIT: I'd prefer `reset`, as `clear` is too similar to "clear selection or "deselect all" in my mind

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:197
> +            return false;

Is there a reason this function needs to return a boolean?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:223
> +        if (event.commandOrControlKey) {

Nice!

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:359
> +        let deselectedItems = oldSelectedIndexes.difference(indexes);
> +        let selectedItems = indexes.difference(oldSelectedIndexes);
> +        this._delegate.selectionControllerSelectionDidChange(this, deselectedItems, selectedItems);

I like this new approach!

> Source/WebInspectorUI/UserInterface/Views/Table.js:347
> +        if (!oldSelectedItems.equals(this._selectionController.selectedItems))

Why is this check needed?

> Source/WebInspectorUI/UserInterface/Views/Table.js:594
> +        if (index === this.numberOfRows - 1)

NIT: for sanity's sake, we should either add an assert or change this to be a `>=`

> Source/WebInspectorUI/UserInterface/Views/Table.js:601
> +        if (index === 0)

NIT: for sanity's sake, we should either add an assert or change this to be a `<=`

> Source/WebInspectorUI/UserInterface/Views/Table.js:1244
> +        this._selectionController.handleKeyDown(event);

This won't call `revealRow` when moving Up/Down.  I think that's an issue if we try to go Up/Down without a previous selection, as the item to select may be outside what's been cached.

Perhaps there should be a `selectionControllerRevealIndex` delegate call?

[Matt Baker]

Comment on attachment 355667 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:33
>> +        this._delegate = delegate;
> 
> NIT: would this really be considered a delegate?  Perhaps "view" or something else like that to better explain that this is the "controlled object"?

In practice it's a view, but delegate is very general which I like here.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:70
>> +    get selectedItems() { return this._selectedIndexes; }
> 
> Style: put all one-line getters at the top of the Public section

I'll change it here, but in general I don't think this should be a hard and fast rule. Sometimes it makes sense to group properties semantically.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:149
>> +        this._lastSelectedIndex = newSelectedItems.lastIndex;
> 
> Should we also treat the last index as the anchor for shift-selection?  This seems to be how Finder does it.

Nice observation. In fact, it looks like Finder only does this in the absence of an existing shift-selection anchor; I'll match that behavior.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:187
>> +    clear()
> 
> NIT: I'd prefer `reset`, as `clear` is too similar to "clear selection or "deselect all" in my mind

`reset` is better.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:197
>> +            return false;
> 
> Is there a reason this function needs to return a boolean?

There is. TreeOutline will use the return value. I should have added the return value in that patch, but let's just keep this.

>> Source/WebInspectorUI/UserInterface/Views/Table.js:1244
>> +        this._selectionController.handleKeyDown(event);
> 
> This won't call `revealRow` when moving Up/Down.  I think that's an issue if we try to go Up/Down without a previous selection, as the item to select may be outside what's been cached.
> 
> Perhaps there should be a `selectionControllerRevealIndex` delegate call?

Great catch. We can resolve this in Table, without extending the delegate:

 selectionControllerSelectionDidChange(controller, deselectedItems, selectedItems)
 {
     if (deselectedItems.size)
         this._toggleSelectedRowStyle(deselectedItems, false);
     if (selectedItems.size)
         this._toggleSelectedRowStyle(selectedItems, true);

+    if (selectedItems.size === 1) {
+        let rowIndex = selectedItems.firstIndex;
+        if (!this._isRowVisible(rowIndex))
+            this.revealRow(rowIndex);
+    }

     if (this._delegate.tableSelectionDidChange)
         this._delegate.tableSelectionDidChange(this);
 }

[Matt Baker]

Created attachment 355735 [details]
Patch

[Matt Baker]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 355667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355667&action=review
> 
> r-, due to test failures (as discussed offline), and the lack of handling
> item addition/removal (e.g. `didInsertItem` and `didRemoveItem`)
> 
> Otherwise, looks good!  Was there a reason you decided on going forward with
> an index-based approach vs an object-based approach, like we discussed
> before Thanksgiving?  I think the object-based approach will be much more
> flexible for `WI.TreeOutline` and would likely require far less work to
> implement vs an index-based approach.

An object-based approach has the advantage of being slightly simpler, in that `didRemoveItem` and `didInsertItem` aren't needed to keep SelectionController's stored indexes in synch with the view. This has the consequence that the selected items are no longer in order, however, which is an issue for copying rows from Table.

The index-based approach was also complete, and I didn't want to rock the boat at the 11th hour. ;)

[Devin Rousso]

(In reply to Matt Baker from comment #13)
> The index-based approach was also complete, and I didn't want to rock the boat at the 11th hour. ;)
No worries!  Was just wondering :)

I think an object-based approach will be more flexible, but in the spirit of getting things done, this works just as well =D

[Devin Rousso]

Comment on attachment 355735 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:42
> +        console.assert(this._delegate.selectionControllerNextSelectableIndex, "SelectionController delegate must implement selectionControllerNextSelectableIndex.");

`selectionControllerPreviousSelectableIndex`?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:195
> +    didRemoveItem(index)

What happened to `didInsertItem`?  Don't we need to do effectively the same logic but instead add one to each index?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:200
> +        while (index = this._selectedIndexes.indexGreaterThan(index)) {

So this is relying on the falsey nature of `NaN` to return when there is no greater index?

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:225
> +        }

There should be a default return value after this.

[Matt Baker]

Created attachment 355747 [details]
Patch

[Matt Baker]

Comment on attachment 355735 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:195
>> +    didRemoveItem(index)
> 
> What happened to `didInsertItem`?  Don't we need to do effectively the same logic but instead add one to each index?

Individual items aren't inserted into Table, only removed. TreeOutline inserts individual items, so it will be added in https://webkit.org/b/191483.

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:200
>> +        while (index = this._selectedIndexes.indexGreaterThan(index)) {
> 
> So this is relying on the falsey nature of `NaN` to return when there is no greater index?

Yep.

[Devin Rousso]

Comment on attachment 355747 [details]
Patch

r=me, nice work :)

[WebKit Commit Bot]

Comment on attachment 355747 [details]
Patch

Clearing flags on attachment: 355747

Committed r238563: <https://trac.webkit.org/changeset/238563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.