Fix Grid not using getId on select all

[tsuoanttila]

Fixes #11083

---

This change is 

[mehdi-vaadin]

What about doing some refactoring and change the type of selection to HashMap where the ids can be kept as key and the objects as values? It may be better in terms of performance.

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @tsuoanttila)

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 436 at r1 (raw file):

Quoted 6 lines of code…

        addedItems.removeIf(item -> removedItems.remove(item));
 
        if (selection.containsAll(addedItems)
                && Collections.disjoint(selection, removedItems)) {
            return;
        }

This part should also be changed.

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 442 at r1 (raw file):

removedItems.isEmpty()

Although removedItems is not empty, nothing may be removed from selection, therefore allSelected shouldn't be set to false. So, it may be better to set allSelected after removing items and make sure that at least one item is really removed.

[mehdi-vaadin]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tsuoanttila)

[tsuoanttila]

In the default case of we'd store the element into the hashmap twice, since by default the id is the item itself (which is true when the equals and hashcode are implemented properly). Would need to investigate if it makes some of the already existing update logic more complex.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tsuoanttila)

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 436 at r1 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

        addedItems.removeIf(item -> removedItems.remove(item));
 
        if (selection.containsAll(addedItems)
                && Collections.disjoint(selection, removedItems)) {
            return;
        }

This part should also be changed.

This code block attempts to be a shortcut for the case where there is actually no changes at all. In the case of actually having different objects is a valid concern, as updating the selection also updates the current cache of selected items.

I'm not even sure if it ever makes sense to shortcut it at all.

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 442 at r1 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

removedItems.isEmpty()

Although removedItems is not empty, nothing may be removed from selection, therefore allSelected shouldn't be set to false. So, it may be better to set allSelected after removing items and make sure that at least one item is really removed.

This PR is not about universally solving the case of all selection updates. Yes, without digging deeper into the code I can't know if there's something maybe broken with this or maybe not. The issue of this PR is about the select all not using the getId to properly identify the selected items.

If there's an issue with this, it should be made into a ticket and handled in it's own PR.

[mehdi-vaadin]

You are right. it needs more investigation.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @mehdi-vaadin)

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 436 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

This code block attempts to be a shortcut for the case where there is actually no changes at all. In the case of actually having different objects is a valid concern, as updating the selection also updates the current cache of selected items.

I'm not even sure if it ever makes sense to shortcut it at all.

When using getId is ignored here to find out whether we have any change or not, then we may end up in return and doing nothing. Of course, only in some rare cases.
One more thing is that it looks confusing since two different methods are used to compare objects.

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 442 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

This PR is not about universally solving the case of all selection updates. Yes, without digging deeper into the code I can't know if there's something maybe broken with this or maybe not. The issue of this PR is about the select all not using the getId to properly identify the selected items.

If there's an issue with this, it should be made into a ticket and handled in it's own PR.

OK

[tsuoanttila]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @tsuoanttila)

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 436 at r1 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

When using getId is ignored here to find out whether we have any change or not, then we may end up in return and doing nothing. Of course, only in some rare cases.
One more thing is that it looks confusing since two different methods are used to compare objects.

True. In the case of removing a not-equals item we might skip it, but not sure how we would end up in that situation. Will fix this.

[tsuoanttila]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @mehdi-vaadin)

---

server/src/main/java/com/vaadin/ui/components/grid/MultiSelectionModelImpl.java, line 436 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

True. In the case of removing a not-equals item we might skip it, but not sure how we would end up in that situation. Will fix this.

Done.

[mehdi-vaadin]

Reviewed 3 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved