From 40b006a7afc0dc168beba57c971663ce1326df45 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Wed, 7 Feb 2018 13:56:58 +0200 Subject: [PATCH] Fix Column.setSortable being overridden (#10604) --- server/src/main/java/com/vaadin/ui/Grid.java | 51 ++++++++++++------ .../component/grid/GridDeclarativeTest.java | 48 ++++++++--------- .../tests/server/component/grid/GridTest.java | 52 ++++++++++++++++++- 3 files changed, 108 insertions(+), 43 deletions(-) diff --git a/server/src/main/java/com/vaadin/ui/Grid.java b/server/src/main/java/com/vaadin/ui/Grid.java index 3a18de05c6..d289ee4897 100644 --- a/server/src/main/java/com/vaadin/ui/Grid.java +++ b/server/src/main/java/com/vaadin/ui/Grid.java @@ -56,6 +56,7 @@ import com.vaadin.data.provider.DataGenerator; import com.vaadin.data.provider.DataProvider; import com.vaadin.data.provider.GridSortOrder; import com.vaadin.data.provider.GridSortOrderBuilder; +import com.vaadin.data.provider.InMemoryDataProvider; import com.vaadin.data.provider.Query; import com.vaadin.data.provider.QuerySortOrder; import com.vaadin.event.ConnectorEvent; @@ -840,6 +841,7 @@ public class Grid extends AbstractListing implements HasComponents, return Stream.of(new QuerySortOrder(id, direction)); }; + private boolean sortable = true; private SerializableComparator comparator; private StyleGenerator styleGenerator = item -> null; private DescriptionGenerator descriptionGenerator; @@ -1151,11 +1153,12 @@ public class Grid extends AbstractListing implements HasComponents, */ public Column setId(String id) { Objects.requireNonNull(id, "Column identifier cannot be null"); - if (this.userId != null) { + if (userId != null) { throw new IllegalStateException( "Column identifier cannot be changed"); } - this.userId = id; + + userId = id; getGrid().setColumnId(id, this); updateSortable(); @@ -1163,8 +1166,11 @@ public class Grid extends AbstractListing implements HasComponents, } private void updateSortable() { - setSortable(getGrid().getDataProvider().isInMemory() - || getSortOrder(SortDirection.ASCENDING).count() != 0); + boolean inMemory = getGrid().getDataProvider().isInMemory(); + boolean hasSortOrder = getSortOrder(SortDirection.ASCENDING) + .count() != 0; + + getState().sortable = this.sortable && (inMemory || hasSortOrder); } /** @@ -1180,29 +1186,43 @@ public class Grid extends AbstractListing implements HasComponents, } /** - * Sets whether the user can sort this column or not. - *

- * By default, a grid using a in-memory data provider has its columns - * sortable by default. For a backend data provider, the columns are not - * sortable by default. + * Sets whether the user can sort this column or not. Whether the column + * is actually sortable after {@code setSortable(true)} depends on the + * {@link DataProvider} and the defined sort order for this column. When + * using an {@link InMemoryDataProvider} sorting can be automatic. * * @param sortable - * {@code true} if the column can be sorted by the user; - * {@code false} if not + * {@code true} to enable sorting for this column; + * {@code false} to disable it * @return this column */ public Column setSortable(boolean sortable) { - getState().sortable = sortable; + if (this.sortable != sortable) { + this.sortable = sortable; + updateSortable(); + } return this; } /** - * Gets whether the user can sort this column or not. + * Gets whether sorting is enabled for this column. * - * @return {@code true} if the column can be sorted by the user; + * @return {@code true} if the sorting is enabled for this column; * {@code false} if not */ public boolean isSortable() { + return sortable; + } + + /** + * Gets whether the user can actually sort this column. + * + * @return {@code true} if the column can be sorted by the user; + * {@code false} if not + * + * @since + */ + public boolean isSortableByUser() { return getState(false).sortable; } @@ -2070,8 +2090,7 @@ public class Grid extends AbstractListing implements HasComponents, * @return this column * @since 8.3 */ - public Column setHandleWidgetEvents( - boolean handleWidgetEvents) { + public Column setHandleWidgetEvents(boolean handleWidgetEvents) { getState().handleWidgetEvents = handleWidgetEvents; return this; } diff --git a/server/src/test/java/com/vaadin/tests/server/component/grid/GridDeclarativeTest.java b/server/src/test/java/com/vaadin/tests/server/component/grid/GridDeclarativeTest.java index 93966ed01f..546411f745 100644 --- a/server/src/test/java/com/vaadin/tests/server/component/grid/GridDeclarativeTest.java +++ b/server/src/test/java/com/vaadin/tests/server/component/grid/GridDeclarativeTest.java @@ -82,8 +82,8 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest { + "Id" + "", getComponentTag(), - heightMode.toString().toLowerCase(Locale.ROOT), - frozenColumns, heightByRows, + heightMode.toString().toLowerCase(Locale.ROOT), frozenColumns, + heightByRows, SelectionMode.MULTI.toString().toLowerCase(Locale.ROOT), getComponentTag()); @@ -169,27 +169,28 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest { public void columnAttributes() { Grid grid = new Grid<>(); - String secondColumnId = "id"; - Column column1 = grid.addColumn(Person::getFirstName) - .setCaption("First Name"); + String secondColumnId = "sortableColumn"; + Column notSortableColumn = grid + .addColumn(Person::getFirstName).setCaption("First Name"); Column column2 = grid.addColumn(Person::getLastName) .setId(secondColumnId).setCaption("Id"); - String caption = "test-caption"; - column1.setCaption(caption); + String caption = "not-sortable-column"; + notSortableColumn.setCaption(caption); boolean sortable = false; - column1.setSortable(sortable); + notSortableColumn.setSortable(sortable); boolean editable = true; - column1.setEditorComponent(new TextField(), Person::setLastName); - column1.setEditable(editable); + notSortableColumn.setEditorComponent(new TextField(), + Person::setLastName); + notSortableColumn.setEditable(editable); boolean resizable = false; - column1.setResizable(resizable); + notSortableColumn.setResizable(resizable); boolean hidable = true; - column1.setHidable(hidable); + notSortableColumn.setHidable(hidable); boolean hidden = true; - column1.setHidden(hidden); + notSortableColumn.setHidden(hidden); - String hidingToggleCaption = "toggle-caption"; + String hidingToggleCaption = "sortable-toggle-caption"; column2.setHidingToggleCaption(hidingToggleCaption); double width = 17.3; column2.setWidth(width); @@ -200,20 +201,15 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest { int expandRatio = 83; column2.setExpandRatio(expandRatio); - - String sortableSuffix = ""; - if (sortable) { - sortableSuffix = "='true'"; - } String design = String.format("<%s>" - + "" - + "" + + "" + + "" + "" + "" - + "" + "" - + "
%s%s
", getComponentTag(), sortableSuffix, resizable, - hidingToggleCaption, width, minWidth, maxWidth, expandRatio, - caption, "Id", getComponentTag()); + + "%s" + + "" + "", getComponentTag(), + sortable, resizable, hidingToggleCaption, width, minWidth, + maxWidth, expandRatio, caption, "Id", getComponentTag()); testRead(design, grid, true); testWrite(design, grid); @@ -705,7 +701,7 @@ public class GridDeclarativeTest extends AbstractListingDeclarativeTest { // column.getId() affects .isSortable() if ((id1 != null && id2 != null) || (id1 == null && id2 == null)) { assertEquals(baseError + "Sortable", col1.isSortable(), - col2.isSortable()); + col2.isSortable()); } assertEquals(baseError + "Editable", col1.isEditable(), col2.isEditable()); diff --git a/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java b/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java index 8e31e9acc6..01637760d8 100644 --- a/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java +++ b/server/src/test/java/com/vaadin/tests/server/component/grid/GridTest.java @@ -28,6 +28,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import org.easymock.Capture; +import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -38,8 +39,10 @@ import com.vaadin.data.ValidationException; import com.vaadin.data.ValueProvider; import com.vaadin.data.provider.DataCommunicator; import com.vaadin.data.provider.DataGenerator; +import com.vaadin.data.provider.DataProvider; import com.vaadin.data.provider.GridSortOrder; import com.vaadin.data.provider.QuerySortOrder; +import com.vaadin.data.provider.SortOrder; import com.vaadin.data.provider.bov.Person; import com.vaadin.event.selection.SelectionEvent; import com.vaadin.server.SerializableComparator; @@ -553,7 +556,7 @@ public class GridTest { public void setColumnOrder_byColumn_removedColumn() { thrown.expect(IllegalStateException.class); thrown.expectMessage("setColumnOrder should not be called " - + "with columns that are not in the grid."); + + "with columns that are not in the grid."); grid.removeColumn(randomColumn); grid.setColumnOrder(randomColumn, lengthColumn); @@ -732,4 +735,51 @@ public class GridTest { Column column1 = grid1.addColumn(ValueProvider.identity()); grid2.removeColumn(column1); } + + @Test + public void testColumnSortable() { + Column column = grid.addColumn(String::toString); + + // Use in-memory data provider + grid.setItems(Collections.emptyList()); + + Assert.assertTrue("Column should be initially sortable", + column.isSortable()); + Assert.assertTrue("User should be able to sort the column", + column.isSortableByUser()); + + column.setSortable(false); + + Assert.assertFalse("Column should not be sortable", + column.isSortable()); + Assert.assertFalse( + "User should not be able to sort the column with in-memory data", + column.isSortableByUser()); + + // Use CallBackDataProvider + grid.setDataProvider( + DataProvider.fromCallbacks(q -> Stream.of(), q -> 0)); + + Assert.assertFalse("Column should not be sortable", + column.isSortable()); + Assert.assertFalse("User should not be able to sort the column", + column.isSortableByUser()); + + column.setSortable(true); + + Assert.assertTrue("Column should be marked sortable", + column.isSortable()); + Assert.assertFalse( + "User should not be able to sort the column since no sort order is provided", + column.isSortableByUser()); + + column.setSortProperty("toString"); + + Assert.assertTrue("Column should be marked sortable", + column.isSortable()); + Assert.assertFalse( + "User should be able to sort the column with the sort order", + column.isSortableByUser()); + } + } -- 2.39.5