#512 unselect a node in multi selection when holding shift key

[langonginc]

After #525

[langonginc]

Lines will be added in #528.

[thekingofcity]

Maybe it's a bit hard to find out the relation here. Need some time to think about different situations and what the operation should be. How about writing it to

if (!e.shiftKey) dispatch(clearSelected()); // remove all if this is not a multiple selection
else {
  if (selected.includes(node)) dispatch(removeSelected(node)); // remove current if it is already in the multiple selection
  else dispatch(addSelected(node)); // add current in the multiple selection
}

So basically we have 3 possible operations and none of them could overlap each other. The current code makes me wonder if if could be fired one by one :(

[langonginc]

L3 may be put in the if.

[thekingofcity]

Not quite sure what to put in the if. Any code suggestion?

[langonginc]

Oh, I'm sorry, L4 may be copy to the if like this:

        if (!e.shiftKey) {
            dispatch(clearSelected()); // remove all if this is not a multiple selection
            dispatch(addSelected(node)); // show current details
        } else {
            if (selected.includes(node))
                dispatch(removeSelected(node)); // remove current if it is already in the multiple selection
            else dispatch(addSelected(node)); // add current in the multiple selection
        }

Or, we won't be able to click to show one object's details.

[langonginc]

Hmm, there may be a problem with clearing the selected. We cannot move nodes that are selected if is more than one. We can delete the clear operation to avoid this.

[thekingofcity]

Also it looks like we can always setSelected([node]) in non multiple selection case 🤔 In that way, we can have one less include check. But it’s okay to merge without this change.

[thekingofcity]

Oh, we can not setSelected([node]) when the user clicks a previously selected one without holding shift. Previous selected is used to move multiple selected nodes. This should be left in the comment :(

[langonginc]

Oh, we can not setSelected([node]) when the user clicks a previously selected one without holding shift. Previous selected is used to move multiple selected nodes. This should be left in the comment :(

A, B, C are selected and I just click A again without press the shift key. We should clear selection and add A only? Is that?

[thekingofcity]

A, B, C are selected and I just click A again without pressing the shift key. -> Then we should not clear the multiple selections as users may drag before releasing the click and the selected nodes should be moved according to the drag.

So no-op is correct but a better comment should be left as // no-op as users may drag the previously selected node(s) in handlePointerMove.

[langonginc]

Oh, I got it.

[thekingofcity]

LGTM