Implement multi selection in Grid #2522

ahie · 2017-09-27T14:26:12Z

Additionally adds shorthand methods to Grid
for managing selection.

Closes #2424

This change is

ahie · 2017-09-27T14:28:19Z

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):
Still WIP tests and select all support. Otherwise ready for review.

Comments from Reviewable

denis-anisimov · 2017-09-28T05:24:04Z

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 321 at r1 (raw file):

grid.getDataCommunicator().reset();

Why ?
DataCommunicator will update data on reset. How does it relate to the selection update ?

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/GridNoneSelectionModel.java, line 78 at r1 (raw file):

throw new UnsupportedOperationException(
"This selection model doesn't allow selection, cannot add selection listeners to it"

Does it confirm to the FW8 code ?

I don't see why I can't add a listener to the model. I will never get events - that's the only difference.
(I can have a code that change the model on the fly from one to another. They have common interface, so I can just add a listener regardless of the real model implementation. And I don't want to catch an exception in this case).

flow-data/src/main/java/com/vaadin/data/selection/SingleSelectionListener.java, line 48 at r1 (raw file):

     */
    public void selectionChange(SingleSelectionEvent<L, T> event);
}

new line

Comments from Reviewable

denis-anisimov · 2017-09-28T05:24:22Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

flow-data/src/main/java/com/vaadin/data/selection/MultiSelectionEvent.java, line 133 at r1 (raw file):

        return copy;
    }
}

new line

Comments from Reviewable

Additionally adds shorthand methods to Grid for managing selection. Closes #2424

ahie · 2017-09-28T06:18:37Z

Review status: 9 of 11 files reviewed at latest revision, 5 unresolved discussions.

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 321 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

grid.getDataCommunicator().reset();

Why ?
DataCommunicator will update data on reset. How does it relate to the selection update ?

Rows will get pushed again to the client, with the updated selection information. After #2524 is implemented this should really only call refreshItem on the items whose selection changed.

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/GridNoneSelectionModel.java, line 78 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

throw new UnsupportedOperationException(
"This selection model doesn't allow selection, cannot add selection listeners to it"

Does it confirm to the FW8 code ?

I don't see why I can't add a listener to the model. I will never get events - that's the only difference.
(I can have a code that change the model on the fly from one to another. They have common interface, so I can just add a listener regardless of the real model implementation. And I don't want to catch an exception in this case).

Yes, FW8 also behaves in this way. You are right, but I don't think we should change it, especially with addSelectionListener convenience method being directly in Grid.

flow-data/src/main/java/com/vaadin/data/selection/MultiSelectionEvent.java, line 133 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

new line

Done.

flow-data/src/main/java/com/vaadin/data/selection/SingleSelectionListener.java, line 48 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

new line

Done.

Comments from Reviewable

denis-anisimov · 2017-09-28T06:53:41Z

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 321 at r1 (raw file):

Previously, ahie (Aleksi Hietanen) wrote…

Rows will get pushed again to the client, with the updated selection information. After #2524 is implemented this should really only call refreshItem on the items whose selection changed.

So the selection is a part of the item DTO ?
It's not a separate call with only selected items ?

Comments from Reviewable

ahie · 2017-09-28T10:18:50Z

Review status: 9 of 13 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

a discussion (no related file):

Previously, ahie (Aleksi Hietanen) wrote…

Still WIP tests and select all support. Otherwise ready for review.

Considering creating a separate issue for select all and dynamically adding/removing a selection column on the client side. Currently the client side grid just blows up if you attempt to add a <grid-selection-column> dynamically. Better to have at least some form of multi selection than none at all I would say.

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 321 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

So the selection is a part of the item DTO ?
It's not a separate call with only selected items ?

That's correct and also closer to how it was done in FW8 with data generators.

Comments from Reviewable

denis-anisimov · 2017-09-28T12:28:29Z

Reviewed 4 of 5 files at r3.
Review status: 13 of 14 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

denis-anisimov · 2017-09-28T12:28:54Z

Reviewed 1 of 5 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

denis-anisimov · 2017-09-29T05:37:36Z

Reviewed 3 of 4 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

vaadin-bot · 2017-09-29T06:30:13Z

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java

+
+                    @Override
+                    public MultiSelect<Grid<T>, T> asMultiSelect() {
+                        return new MultiSelect<Grid<T>, T>() {


Reduce this anonymous class number of lines from 34 to at most 20, or make it a named class.

vaadin-bot · 2017-09-29T06:30:13Z

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java

@@ -215,7 +257,170 @@ public Registration addValueChangeListener(
        MULTI {
            @Override
            protected <T> GridSelectionModel<T> createModel(Grid<T> grid) {
-                throw new UnsupportedOperationException("Not implemented yet.");
+                return new GridMultiSelectionModel<T>() {


Reduce this anonymous class number of lines from 142 to at most 20, or make it a named class.

vaadin-bot · 2017-09-29T06:30:14Z

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java

-                throw new UnsupportedOperationException("Not implemented yet.");
+                return new GridMultiSelectionModel<T>() {
+
+                    Set<T> selected = new LinkedHashSet<>();


Make "selected" transient or serializable.

vaadin-bot · 2017-09-29T06:30:15Z

flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java

@@ -191,6 +199,8 @@ public T getValue() {
                            @Override
                            public Registration addValueChangeListener(
                                    ValueChangeListener<Grid<T>, T> listener) {
+                                Objects.requireNonNull(listener,
+                                        "listener cannot be null");


Define a constant instead of duplicating this literal "listener cannot be null" 6 times.

… into 2424-grid-multi-select

ahie · 2017-09-29T12:00:55Z

Review status: 10 of 15 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

a discussion (no related file):

Previously, ahie (Aleksi Hietanen) wrote…

Considering creating a separate issue for select all and dynamically adding/removing a selection column on the client side. Currently the client side grid just blows up if you attempt to add a <grid-selection-column> dynamically. Better to have at least some form of multi selection than none at all I would say.

Created a new issue about this: #2546.

Comments from Reviewable

denis-anisimov · 2017-09-30T07:38:15Z

Reviewed 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

Comments from Reviewable

… into 2424-grid-multi-select

vaadin-bot · 2017-10-02T15:51:09Z

SonarQube analysis reported 5 issues

1 critical
3 major
1 info

Watch the comments in this conversation to review them.

1 extra issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

Grid.java#L165: Potentially ambiguous invocation of either an outer or inherited method com.vaadin.ui.grid.Grid$SelectionMode$1$1.deselectAll() in com.vaadin.ui.grid.Grid$SelectionMode$1$1.remove()

denis-anisimov · 2017-10-02T16:49:24Z

Reviewed 1 of 2 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

Comments from Reviewable

ahie added the in progress label Sep 27, 2017

Implement multi selection in Grid

d8a1f76

Additionally adds shorthand methods to Grid for managing selection. Closes #2424

ahie force-pushed the 2424-grid-multi-select branch from c6a386e to d8a1f76 Compare September 28, 2017 06:17

Merge remote-tracking branch 'origin/master' into 2424-grid-multi-select

4ae3538

Add tests

596df96

ahie changed the title ~~[WIP] Implement multi selection in Grid~~ Implement multi selection in Grid Sep 28, 2017

Add newline

25d1e02

ahie force-pushed the 2424-grid-multi-select branch from c64261b to 25d1e02 Compare September 28, 2017 12:56

ahie added 3 commits September 28, 2017 16:15

Add change detection of flow-data to travis script

144e1a3

Javadoc fix

94d832f

Merge master into 2424-grid-multi-select

e5a1b40

Merge master into 2424-grid-multi-select

05e22b4

vaadin-bot reviewed Sep 29, 2017

View reviewed changes

ahie added 3 commits September 29, 2017 13:22

SQ fixes

461e216

Merge branch '2424-grid-multi-select' of https://github.com/vaadin/flow…

67336b9

… into 2424-grid-multi-select

Merge master into 2424-grid-multi-select

65bb560

ahie added 2 commits October 2, 2017 08:31

Merge remote-tracking branch 'origin/master' into 2424-grid-multi-select

ac5e866

Merge branch '2424-grid-multi-select' of https://github.com/vaadin/flow…

286b887

… into 2424-grid-multi-select

ahie added 4 commits October 9, 2017 09:47

Merge remote-tracking branch 'origin/master' into 2424-grid-multi-select

d1358fd

Merge remote-tracking branch 'origin/master' into 2424-grid-multi-select

786f58f

Merge master into 2424-grid-multi-select

784f485

Merge master into 2424-grid-multi-select

33807da

ahie merged commit 78d6bbb into master Oct 10, 2017

ahie deleted the 2424-grid-multi-select branch October 10, 2017 12:34

ahie removed the in progress label Oct 10, 2017

ZheSun88 modified the milestone: 1.0.0.alpha5 Oct 12, 2017

SomeoneToIgnore added this to the 1.0.0.alpha6 milestone Oct 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multi selection in Grid #2522

Implement multi selection in Grid #2522

ahie commented Sep 27, 2017 •

edited by Legioth

ahie commented Sep 27, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

ahie commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

ahie commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 29, 2017

vaadin-bot Sep 29, 2017

vaadin-bot Sep 29, 2017

vaadin-bot Sep 29, 2017

vaadin-bot Sep 29, 2017

ahie commented Sep 29, 2017

denis-anisimov commented Sep 30, 2017

vaadin-bot commented Oct 2, 2017

denis-anisimov commented Oct 2, 2017

Implement multi selection in Grid #2522

Implement multi selection in Grid #2522

Conversation

ahie commented Sep 27, 2017 • edited by Legioth

ahie commented Sep 27, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

ahie commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

ahie commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 28, 2017

denis-anisimov commented Sep 29, 2017

vaadin-bot Sep 29, 2017

Choose a reason for hiding this comment

vaadin-bot Sep 29, 2017

Choose a reason for hiding this comment

vaadin-bot Sep 29, 2017

Choose a reason for hiding this comment

vaadin-bot Sep 29, 2017

Choose a reason for hiding this comment

ahie commented Sep 29, 2017

denis-anisimov commented Sep 30, 2017

vaadin-bot commented Oct 2, 2017

1 extra issue

denis-anisimov commented Oct 2, 2017

ahie commented Sep 27, 2017 •

edited by Legioth