Bug 191483 – Web Inspector: TreeOutline should re-use multiple-selection logic from Table

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

[Matt Baker]

Summary:
TreeOutline should re-use multiple-selection logic from Table.

This will involve creating a new class, ItemSelectionController, which owns an IndexSet of selected items and provides a set of selection operations.

[Radar WebKit Bug Importer]

<rdar://problem/45953305>

[Matt Baker]

Created attachment 355791 [details]
Patch

[Devin Rousso]

Comment on attachment 355791 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:-211
> -        if (event.key === "a" && event.commandOrControlKey()) {

Oops :P

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-293
> -        element.treeElement.selectOnMouseDown(event);

Is `selectOnMouseDown` called anywhere else?  I know it's used by `WI.DOMTreeElement` and `WI.ShaderProgramTreeElement`.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:514
>          let deselectedElement = treeOutline.selectedTreeElement;

This variable is never used, so you could remove it.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:517
> +            treeOutline.selectTreeElementInternal(this, suppressOnSelect, selectedByUser);

This will have a slightly different effect than what was here before.  It will no longer fire an event when the element is already selected, which was not the case previously (it would fire an event even it was already selected (`allowsRepeatSelection` had to have also been `true`).  You should move this outside of the `if` (or just remove the `if` altogether):

    this.selected = true;
    treeOutline.selectTreeElementInternal(this, suppressOnSelect, selectedByUser);

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:59
> +        this._treeElementIndexCache = new Map;

Perhaps this would be a good opportunity to create an `IndexMap` (some sort of specialized `BiMap`)?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:102
> +        const extendSelection = false;

This defaults to `false`, so you don't need to specify it in the `selectItem` call.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:278
> +            this.treeOutline._selectionController.didInsertItem(insertionIndex);

Wow we do this all over the place... why is the `this.treeOutline` this needed?  Would `this._selectionController` not suffice?  Do we nest `WI.TreeOutline` somewhere?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:576
> +        if (this.selectedTreeElement) {

This check isn't necessary, as you should've already bailed above if `!this.selectedTreeElement`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:631
> +            handled = this._selectionController.handleKeyDown(event);

Ah I see how you're using the return value.  Nice!

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:921
> +            

???

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1015
> +            let closestIndex = this._treeElementIndexCache.get(current);

Is there a real need for this cache, other than in an effort to boost performance?  It seems to me that since we clear it each time we remember/forget `WI.TreeElement`s, there's not much to be gained by having it.  I'd just remove it unless it's necessary.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1038
> +        for (let treeElement = this.children[0]; treeElement; treeElement = treeElement.traverseNextTreeElement(skipUnrevealed, null, dontPopulate), ++current) {

Style: wow this is ugly.  I'd rework this as a `while`:

    const skipUnrevealed = false;
    const stayWithin = null;
    const dontPopulate = true;

    let current = 0;
    let treeElement = this.children[0];
    while (treeElement) {
        if (current === index)
            return treeElement;

        treeElement = treeElement.traverseNextTreeElement(skipUnrevealed, stayWithin, dontPopulate);
        ++current;
    }

[Matt Baker]

Comment on attachment 355791 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:59
>> +        this._treeElementIndexCache = new Map;
> 
> Perhaps this would be a good opportunity to create an `IndexMap` (some sort of specialized `BiMap`)?

I considered this. Once the dust settles I'll take a look at cleaning up/optimizing TreeOutline a bit.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:921
>> +            
> 
> ???

Oops, this is old.

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1015
>> +            let closestIndex = this._treeElementIndexCache.get(current);
> 
> Is there a real need for this cache, other than in an effort to boost performance?  It seems to me that since we clear it each time we remember/forget `WI.TreeElement`s, there's not much to be gained by having it.  I'd just remove it unless it's necessary.

I'd like to leave it in for now. We can revisit/remove it later if needed.

[Matt Baker]

Created attachment 355823 [details]
Patch

[Devin Rousso]

Comment on attachment 355823 [details]
Patch

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

r=me, awesome work!

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:192
> +        return true;

Add "// Overridden by subclasses if needed."

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:278
> +        if (this.allowsMultipleSelection) {
> +            let insertionIndex = this._indexOfTreeElement(child.previousSibling) || 0;
> +            this.treeOutline._selectionController.didInsertItem(insertionIndex);
> +        }

I think this should be always called.  Whenever we add a new item, even if we're not allowing multiple selection, we should update the index of any selected item(s) anywhere in the tree.  As an example, if I've selected item 3, and I insert at item 1, my selection should be updated to item 4.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:602
> +                if (this.selectedTreeElement.expanded) {
> +                    nextSelectedElement = this.selectedTreeElement.children[0];
> +                    while (nextSelectedElement && !nextSelectedElement.selectable)
> +                        nextSelectedElement = nextSelectedElement.nextSibling;
> +                    handled = nextSelectedElement ? true : false;

This doesn't match Finder's functionality.  Are we sure we want to add this?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:835
> +        while (treeElement = treeElement.traverseNextTreeElement(skipUnrevealed, null, dontPopulate)) {

NIT: use `const stayWithin = null;` for this argument instead.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:852
> +        while (treeElement = treeElement.traversePreviousTreeElement(skipUnrevealed, null, dontPopulate)) {

Ditto (835)

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:862
> +    selectTreeElementInternal(treeElement, suppressNotification = false, selectedByUser = false)

I'd rather you use an optional object instead of defaulted parameters:

    selectTreeElementInternal(treeElement, {suppressNotification, selectedByUser} = {})

[Matt Baker]

Created attachment 355830 [details]
Patch

[Matt Baker]

Created attachment 355831 [details]
Patch

[Devin Rousso]

Comment on attachment 355831 [details]
Patch

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

r=me, well done!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:-302
> -        element.select();

Considering your changes to `WI.TreeOutline`, you could remove this entire event listener.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:601
> +                if (this.selectedTreeElement.expanded) {
> +                    nextSelectedElement = this.selectedTreeElement.children[0];
> +                    while (nextSelectedElement && !nextSelectedElement.selectable)
> +                        nextSelectedElement = nextSelectedElement.nextSibling;
> +                    handled = nextSelectedElement ? true : false;

This doesn't match Finder's functionality.  Are we sure we want to add this?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:873
> +    selectTreeElementInternal(treeElement, suppressNotification = false, selectedByUser = false)

I'd rather you use an optional object instead of defaulted parameters:

    selectTreeElementInternal(treeElement, {suppressNotification, selectedByUser} = {})

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:875
> +        if (this._processingSelectionControllerSelectionDidChange)

All of this early-return interleaving is a bit crazy.  We definitely need some tests for all of these different cases 😅

[WebKit Commit Bot]

Comment on attachment 355831 [details]
Patch

Clearing flags on attachment: 355831

Committed r238599: <https://trac.webkit.org/changeset/238599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

Next time please consider doing something like this behind the experimental flag.

This broke debugger completely and introduced several bugs in the Elements tab.