GLSP-1194: Do not send out selection changes if only the root changed

packages/client/src/base/selection-service.spec.ts

@@ -13,10 +13,10 @@
  *
  * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
  ********************************************************************************/
+import { Action, Disposable, GModelRoot, GNode, TYPES, initializeContainer } from '@eclipse-glsp/sprotty';
 import { AssertionError, expect } from 'chai';
 import { Container, injectable } from 'inversify';
 import * as sinon from 'sinon';
-import { Action, Disposable, GModelRoot, GNode, TYPES, initializeContainer } from '@eclipse-glsp/sprotty';
 import { defaultModule } from './default.module';
 import { IFeedbackActionDispatcher, IFeedbackEmitter } from './feedback/feedback-action-dispatcher';
 import { ISelectionListener, SelectFeedbackAction, SelectionService } from './selection-service';
@@ -217,13 +217,13 @@ describe('SelectionService', () => {
             selectionService.updateSelection(root, [], ['node4']);
             assertListener(listener, root, ['node2', 'node3'], ['node4'], 4);
         });
-        it('A registered listener should be notified of root changes.', () => {
+        it('A registered listener should NOT be notified of root changes.', () => {
             selectionService.updateSelection(root, [], []);
-            assertListener(listener, root, [], [], 1);
+            assertListener(listener, root, [], [], 0);
 
             const newRoot = createRoot('node1', 'newNode2', 'newNode3');
             selectionService.updateSelection(newRoot, [], []);
-            assertListener(listener, newRoot, [], [], 2);
+            assertListener(listener, newRoot, [], [], 0);
         });
         it('Selecting the same elements consecutively should not trigger a listener update.', () => {
             selectionService.updateSelection(root, ['node1'], []);
@@ -290,8 +290,10 @@ describe('SelectionService', () => {
         expectedCalled: number
     ): void {
         expect(listener.selectionChanged.callCount).to.be.equal(expectedCalled);
-        expect(listener.selectionChanged.lastCall.args[0]).to.be.deep.equals(expectedRoot);
-        expect(listener.selectionChanged.lastCall.args[1]).to.be.deep.equals(expectedSelection);
-        expect(listener.selectionChanged.lastCall.args[2]).to.be.deep.equals(expectedDeselection);
+        if (expectedCalled > 0) {
+            expect(listener.selectionChanged.lastCall.args[0]).to.be.deep.equals(expectedRoot);
+            expect(listener.selectionChanged.lastCall.args[1]).to.be.deep.equals(expectedSelection);
+            expect(listener.selectionChanged.lastCall.args[2]).to.be.deep.equals(expectedDeselection);
+        }
     }
 });

packages/client/src/base/selection-service.ts

@@ -96,41 +96,41 @@ export class SelectionService implements IGModelRootListener, Disposable {
         this.updateSelection(root, [], []);
     }
 
-    updateSelection(root: Readonly<GModelRoot>, select: string[], deselect: string[]): void {
-        if (root === undefined && select.length === 0 && deselect.length === 0) {
+    updateSelection(newRoot: Readonly<GModelRoot>, select: string[], deselect: string[]): void {
+        if (newRoot === undefined && select.length === 0 && deselect.length === 0) {
             return;
         }
         const prevRoot = this.root;
         const prevSelectedElementIDs = new Set(this.selectedElementIDs);
 
-        // update root
-        this.root = root;
+        this.root = newRoot;
 
-        // update selected element IDs and collect deselected elements
-        // - select all elements that are not deselected at the same time (no-op)
-        // - deselect all elements that are not selected at the same time (no-op) but was selected
+        // We only select elements that are not part of the deselection
         const toSelect = [...select].filter(selectId => deselect.indexOf(selectId) === -1);
+
+        // We only need to deselect elements that are not part of the selection
+        // If an element is part of both the select and deselect, it's state is not changed
         const toDeselect = [...deselect].filter(deselectId => select.indexOf(deselectId) === -1 && this.selectedElementIDs.has(deselectId));
-        for (const id of toDeselect) {
-            this.selectedElementIDs.delete(id);
-        }
-        for (const id of toSelect) {
-            this.selectedElementIDs.add(id);
-        }
 
+        // update selected element ids
+        toDeselect.forEach(toDeselectId => this.selectedElementIDs.delete(toDeselectId));
+        toSelect.forEach(toSelectId => this.selectedElementIDs.add(toSelectId));
+
+        // check if the newly or previously selected elements still exist in the updated root
         const deselectedElementIDs = new Set(toDeselect);
-        // see if selected elements still exist in the updated root
         for (const id of this.selectedElementIDs) {
-            const element = root.index.getById(id);
+            const element = newRoot.index.getById(id);
             if (element === undefined) {
+                // element to be selected does not exist in the root...
                 this.selectedElementIDs.delete(id);
-                if (prevRoot !== undefined && prevRoot.index.getById(id)) {
+                if (prevRoot?.index.getById(id)) {
+                    // ...but existed in the previous root, so we want to consider it deselected
                     deselectedElementIDs.add(id);
                 }
             }
         }
 
-        // only send out changes if there actually are changes, i.e., the root or the selected elements changed
+        // only send out changes if there actually are changes, i.e., any of the selected elements ids has changed
         const selectionChanged =
             prevSelectedElementIDs.size !== this.selectedElementIDs.size ||
             ![...prevSelectedElementIDs].every(value => this.selectedElementIDs.has(value));
@@ -142,10 +142,6 @@ export class SelectionService implements IGModelRootListener, Disposable {
                     deselectedElementsIDs: [...deselectedElementIDs]
                 })
             ]);
-        }
-
-        const rootChanged = prevRoot !== root;
-        if (rootChanged || selectionChanged) {
             // notify listeners after the feedback action
             this.notifyListeners(this.root, this.selectedElementIDs, deselectedElementIDs);
         }