Skip to content

Commit

Permalink
fix: not to call count callback for lazy Grid with multi-select (#433)
Browse files Browse the repository at this point in the history 
Selecting any row of the Grid with lazy loading and multi-select mode does not trigger the count callback to be invoked. Instead, it gets the item count from the cache for defined item count, and doesn't show the select all checkbox at all and skips checkbox state update for unknown item count.

Fixes: #411
(cherry picked from commit 030ef43)
  • Loading branch information
@mshabarov
mshabarov authored Nov 26, 2020
1 parent e8ab6d1 commit 3a5f1a1
Show file tree
Hide file tree
Showing 5 changed files with 359 additions and 39 deletions.
34 changes: 23 additions & 11 deletions ...n-tests/src/main/java/com/vaadin/flow/component/grid/it/GridMultiSelectionColumnPage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.vaadin.flow.component.html.H2;
import com.vaadin.flow.component.html.NativeButton;
import com.vaadin.flow.data.provider.CallbackDataProvider;
import com.vaadin.flow.data.provider.DataProvider;
import com.vaadin.flow.data.provider.Query;
import com.vaadin.flow.router.Route;

Expand All @@ -39,6 +38,10 @@ public class GridMultiSelectionColumnPage extends Div {

public static final int ITEM_COUNT = 1000;

static final String IN_MEMORY_GRID_ID = "in-memory-grid";
static final String DEFINED_ITEM_COUNT_LAZY_GRID_ID = "defined-item-count-lazy-grid";
static final String UNKNOWN_ITEM_COUNT_LAZY_GRID_ID = "unknown-item-count-lazy-grid";

private Div message;

/**
Expand All @@ -48,7 +51,8 @@ public GridMultiSelectionColumnPage() {
message = new Div();
message.setId("selected-item-count");

createLazyGrid();
createDefinedItemCountLazyGrid();
createUnknownItemCountLazyGrid();
createInMemoryGrid();
createGridWithSwappedDataProvider();

Expand All @@ -58,26 +62,34 @@ public GridMultiSelectionColumnPage() {
setAutoWidthIsTrueOfSelectionColumn();
}

private void createLazyGrid() {
private void createDefinedItemCountLazyGrid() {
Grid<String> lazyGrid = new Grid<>();
lazyGrid.setItems(DataProvider.fromCallbacks(query -> {
return IntStream
.range(query.getOffset(),
query.getOffset() + query.getLimit())
.mapToObj(Integer::toString);
}, query -> ITEM_COUNT));
lazyGrid.setItems(query -> IntStream
.range(query.getOffset(), query.getOffset() + query.getLimit())
.mapToObj(Integer::toString), query -> ITEM_COUNT);
setUp(lazyGrid);
lazyGrid.setId("lazy-grid");
lazyGrid.setId(DEFINED_ITEM_COUNT_LAZY_GRID_ID);

add(new H2("Lazy grid"), lazyGrid);
}

private void createUnknownItemCountLazyGrid() {
Grid<String> unknownItemCountLazyGrid = new Grid<>();
unknownItemCountLazyGrid.setItems(query -> IntStream
.range(query.getOffset(), query.getOffset() + query.getLimit())
.mapToObj(Integer::toString));
setUp(unknownItemCountLazyGrid);
unknownItemCountLazyGrid.setId(UNKNOWN_ITEM_COUNT_LAZY_GRID_ID);

add(new H2("Unknown Item Count Lazy grid"), unknownItemCountLazyGrid);
}

private void createInMemoryGrid() {
Grid<String> grid = new Grid<>();
grid.setItems(
IntStream.range(0, ITEM_COUNT).mapToObj(Integer::toString));
setUp(grid);
grid.setId("in-memory-grid");
grid.setId(IN_MEMORY_GRID_ID);
add(new H2("In-memory grid"), grid);
}

Expand Down
69 changes: 59 additions & 10 deletions ...tests/src/test/java/com/vaadin/flow/component/grid/it/GridMultiSelectionColumnPageIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,40 @@
import org.openqa.selenium.By;
import org.openqa.selenium.WebElement;

import com.vaadin.tests.AbstractComponentIT;
import com.vaadin.flow.component.grid.testbench.GridElement;
import com.vaadin.flow.testutil.TestPath;
import com.vaadin.tests.AbstractComponentIT;

@TestPath("vaadin-grid/grid-multi-selection-column")
public class GridMultiSelectionColumnPageIT extends AbstractComponentIT {

private static final String SELECT_ALL_CHECKBOX_ID = "selectAllCheckbox";

@Test
public void selectAllCheckbox() {
open();
WebElement lazyGrid = findElement(By.id("lazy-grid"));
WebElement definedItemCountLazyGrid = findElement(By.id(
GridMultiSelectionColumnPage.DEFINED_ITEM_COUNT_LAZY_GRID_ID));
Assert.assertEquals(
"Defined Item Count Lazy grid selectAllCheckbox should be hidden by default",
"true",
definedItemCountLazyGrid
.findElement(By.id(SELECT_ALL_CHECKBOX_ID))
.getAttribute("hidden"));

WebElement unknownItemCountLazyGrid = findElement(By.id(
GridMultiSelectionColumnPage.UNKNOWN_ITEM_COUNT_LAZY_GRID_ID));
Assert.assertEquals(
"lazy grid selectAllCheckbox should be hidden by default",
"true", lazyGrid.findElement(By.id("selectAllCheckbox"))
"Unknown Item Count Lazy grid selectAllCheckbox should be hidden by default",
"true",
unknownItemCountLazyGrid
.findElement(By.id(SELECT_ALL_CHECKBOX_ID))
.getAttribute("hidden"));

WebElement grid = findElement(By.id("in-memory-grid"));
WebElement grid = findElement(
By.id(GridMultiSelectionColumnPage.IN_MEMORY_GRID_ID));
WebElement selectAllCheckbox = grid
.findElement(By.id("selectAllCheckbox"));
.findElement(By.id(SELECT_ALL_CHECKBOX_ID));
Assert.assertNull(
"in-memory grid selectAllCheckbox should be visible by default",
selectAllCheckbox.getAttribute("hidden"));
Expand Down Expand Up @@ -73,7 +89,7 @@ public void selectCheckboxMultiSelectionMode() {
// Test switch selection mode from single to multi mode before adding the grid to DOM
WebElement gridSelectionMode = findElement(By.id("in-testing-multi-selection-mode-grid"));
WebElement selectAllCheckbox_selectionMode = gridSelectionMode
.findElement(By.id("selectAllCheckbox"));
.findElement(By.id(SELECT_ALL_CHECKBOX_ID));
WebElement message_selectionMode = findElement(By.id("selected-item-count"));
Assert.assertEquals(true, selectAllCheckbox_selectionMode.isDisplayed());
selectAllCheckbox_selectionMode.click();
Expand All @@ -96,7 +112,8 @@ public void selectCheckboxMultiSelectionMode() {
@Test
public void noSelectOnRowItemClick() {
open();
WebElement grid = findElement(By.id("in-memory-grid"));
WebElement grid = findElement(
By.id(GridMultiSelectionColumnPage.IN_MEMORY_GRID_ID));
// click the first row's cell that corresponds to the text column
grid.findElements(By.tagName("vaadin-grid-cell-content")).stream()
.filter(element -> "0".equals(element.getText())).findFirst()
Expand All @@ -116,7 +133,7 @@ public void gridWithSwappedDataProvider_selectAllIsNotVisible_swappingDataProvid

WebElement grid = findElement(By.id("swapped-grid"));
WebElement selectAllCheckbox = grid
.findElement(By.id("selectAllCheckbox"));
.findElement(By.id(SELECT_ALL_CHECKBOX_ID));

Assert.assertEquals("The selectAllCheckbox should be hidden by default",
"true", selectAllCheckbox.getAttribute("hidden"));
Expand All @@ -142,7 +159,7 @@ public void gridWithSwappedDataProvider_selectAllIsForcedVisible_noSelectionEven

WebElement grid = findElement(By.id("swapped-grid"));
WebElement selectAllCheckbox = grid
.findElement(By.id("selectAllCheckbox"));
.findElement(By.id(SELECT_ALL_CHECKBOX_ID));

executeScript("arguments[0].selectAllHidden = false",
selectAllCheckbox);
Expand All @@ -164,4 +181,36 @@ public void setAutoWidthOfSelectionColumnIsTrue() {
String autoWidth = gridSelectionMode.getAttribute("autoWidth");
Assert.assertTrue("autoWidth should be true", Boolean.parseBoolean(autoWidth));
}

@Test // https://github.com/vaadin/vaadin-flow-components/issues/411
public void selectRow_gridWithUnknownItemCount_noExceptionThrown() {
open();
GridElement grid = $(GridElement.class).id(
GridMultiSelectionColumnPage.UNKNOWN_ITEM_COUNT_LAZY_GRID_ID);

WebElement firstRowSelector = grid
.findElements(By.tagName("vaadin-checkbox")).get(0);

// Select any row
firstRowSelector.click();

// Un-select that row
firstRowSelector.click();

// Scroll to a random row outside the initial requested range [0..200]
grid.scrollToRow(199);
grid.scrollToRow(299);

// Select the first visible row checkbox
WebElement outsideRangeRowSelector = grid
.findElements(By.tagName("vaadin-checkbox")).get(0);

// Select that row
outsideRangeRowSelector.click();
// Un-select that row
outsideRangeRowSelector.click();

// Check that the count callback not invoked -> no exception thrown
checkLogsForErrors();
}
}
27 changes: 23 additions & 4 deletions ...id-flow/src/main/java/com/vaadin/flow/component/grid/AbstractGridMultiSelectionModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.vaadin.flow.component.ComponentEventListener;
import com.vaadin.flow.component.ComponentUtil;
import com.vaadin.flow.component.grid.Grid.AbstractGridExtension;
import com.vaadin.flow.data.provider.DataCommunicator;
import com.vaadin.flow.data.provider.DataProvider;
import com.vaadin.flow.data.provider.Query;
import com.vaadin.flow.data.provider.hierarchy.HierarchicalDataProvider;
Expand Down Expand Up @@ -115,13 +116,27 @@ public void selectFromClient(T item) {

long size = 0;

final DataProvider<T, ?> dataProvider = getGrid()
.getDataCommunicator().getDataProvider();
if (!isSelectAllCheckboxVisible()) {
// Skip changing the state of Select All checkbox if it was
// meant to be hidden
return;
}

final DataCommunicator<T> dataCommunicator = getGrid()
.getDataCommunicator();

final DataProvider<T, ?> dataProvider = dataCommunicator
.getDataProvider();

// Avoid throwing an IllegalArgumentException in case of
// HierarchicalDataProvider
if (!(dataProvider instanceof HierarchicalDataProvider)) {
size = dataProvider.size(new Query<>());
if (dataProvider.isInMemory()) {
size = dataProvider.size(new Query<>());
} else if (dataCommunicator.isDefinedSize()) {
// Use a cached value of items count for defined size
size = dataCommunicator.getItemCount();
}
}

selectionColumn.setSelectAllCheckboxState(size == selected.size());
Expand Down Expand Up @@ -300,7 +315,11 @@ public boolean isSelectAllCheckboxVisible() {
case HIDDEN:
return false;
case VISIBLE:
return true;
// Don't show the Select All Checkbox for undefined size, even if
// the visible property is chosen. Select All Checkbox's state
// changing requires a size query, which is not expected for
// undefined size
return getGrid().getDataCommunicator().isDefinedSize();
default:
throw new IllegalStateException(String.format(
"Select all checkbox visibility is set to an unsupported value: %s",
Expand Down
40 changes: 26 additions & 14 deletions ...aadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/GridMultiSelectionModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,33 +38,35 @@ public interface GridMultiSelectionModel<T>
* row for the selection column.
* <p>
* Default value is {@link #DEFAULT}, which means that the select all is
* only visible if an in-memory data provider is used
* {@link DataProvider#isInMemory()}.
* only visible if an in-memory data is used.
*/
public enum SelectAllCheckboxVisibility {

/**
* Shows the select all checkbox, regardless of data provider used.
* Shows the select all checkbox, if in-memory data is used.
* <p>
* <b>For a lazy data provider, selecting all will result in to all rows
* being fetched from backend to application memory!</b>
* For lazy data, the checkbox is only shown when a count callback has
* been provided. For lazy data with unknown count, the checkbox will
* never be shown.
* <p>
* <b>For lazy data, selecting all will result in to all rows being
* fetched from backend to application memory!</b>
*/
VISIBLE,

/**
* Never shows the select all checkbox, regardless of data provider
* used.
* Never shows the select all checkbox, regardless of data is in-memory
* or not (lazy).
*/
HIDDEN,

/**
* By default select all checkbox depends on the grid's dataprovider.
* By default, the visibility of the select all checkbox depends on how
* the Grid's items are fetched:
* <ul>
* <li>Visible, if the data provider is in-memory</li>
* <li>Hidden, if the data provider is NOT in-memory (lazy)</li>
* <li>Visible, if the data is in-memory</li>
* <li>Hidden, if the data is NOT in-memory (lazy)</li>
* </ul>
*
* @see DataProvider#isInMemory()
*/
DEFAULT;
}
Expand Down Expand Up @@ -93,7 +95,12 @@ Registration addMultiSelectionListener(
* <p>
* The default value is {@link SelectAllCheckboxVisibility#DEFAULT}, which
* means that the checkbox is only visible if the grid's data provider is
* in- memory.
* in-memory.
* <p>
* The select all checkbox will never be shown if the Grid uses lazy loading
* with unknown item count, i.e. no items count query provided to it, and
* even setting {@link SelectAllCheckboxVisibility#VISIBLE} won't make it
* visible.
*
* @param selectAllCheckBoxVisibility
* the visiblity mode to use
Expand All @@ -114,7 +121,12 @@ void setSelectAllCheckboxVisibility(
/**
* Returns whether the select all checkbox will be visible with the current
* setting of
* {@link #setSelectAllCheckboxVisibility(SelectAllCheckboxVisibility)}.
* {@link #setSelectAllCheckboxVisibility(SelectAllCheckboxVisibility)} and
* the type of data set to the Grid (in-memory or lazy).
* <p>
* The select all checkbox will never be shown if the Grid uses lazy loading
* with unknown item count, meaning that no count callback has been
* provided.
*
* @return {@code true} if the checkbox will be visible with the current
* settings
Expand Down
Loading

0 comments on commit 3a5f1a1

Please sign in to comment.