From 3cc2d07e76646cf5af48ea92d010ad488375ac7a Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Sun, 28 Sep 2025 09:15:02 +0300 Subject: [PATCH 1/3] fix: Grid column respects custom reversed comparator and improves serialization (#3926) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Store the original Comparator object instead of just method references - Preserve SerializableComparator instances when provided - Respect custom reversed() implementations for descending sort - Fix serialization issues when non-serializable comparators are used The column now stores the full comparator object, allowing it to: 1. Use custom reversed() implementations when sorting descending 2. Preserve serialization capability when SerializableComparator is provided 3. Only create method references as a fallback for non-serializable comparators Fixes #3926 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../com/vaadin/flow/component/grid/Grid.java | 18 ++- .../flow/component/grid/GridColumnTest.java | 135 ++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 3e1cafebe93..bcbd2acdd52 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -445,7 +445,7 @@ public static class Column extends AbstractColumn> { return Stream.of(new QuerySortOrder(key, direction)); }; - private SerializableComparator comparator; + private Comparator comparator; private Registration columnDataGeneratorRegistration; private Registration editorDataGeneratorRegistration; @@ -735,7 +735,7 @@ public Element getElement() { public Column setComparator(Comparator comparator) { Objects.requireNonNull(comparator, "Comparator must not be null"); setSortable(true); - this.comparator = comparator::compare; + this.comparator = comparator; return this; } @@ -782,7 +782,19 @@ public SerializableComparator getComparator( "No comparator defined for sorted column."); setSortable(true); boolean reverse = sortDirection != SortDirection.ASCENDING; - return reverse ? comparator.reversed()::compare : comparator; + + if (reverse) { + Comparator reversed = comparator.reversed(); + if (reversed instanceof SerializableComparator) { + return (SerializableComparator) reversed; + } + return reversed::compare; + } + + if (comparator instanceof SerializableComparator) { + return (SerializableComparator) comparator; + } + return comparator::compare; } /** diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridColumnTest.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridColumnTest.java index cf937971ac8..32657f8df97 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridColumnTest.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/test/java/com/vaadin/flow/component/grid/GridColumnTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertNotNull; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import java.util.function.BiFunction; @@ -330,6 +331,140 @@ public void setColumnRowHeader_updatedPropertyValue() { Assert.assertTrue(rowHeaderColumn.isRowHeader()); } + @Test + public void testCustomReversedComparatorIsRespected() { + Grid grid = new Grid<>(); + Column column = grid.addColumn(ValueProvider.identity()); + + // Custom comparator that keeps favorites (even numbers) at the top + // regardless of sort direction + Comparator customComparator = new Comparator() { + private boolean isFavorite(Integer value) { + return value != null && value % 2 == 0; + } + + @Override + public int compare(Integer o1, Integer o2) { + // Favorites first, then natural order + return Comparator.comparing(this::isFavorite).reversed() + .thenComparing(Comparator.naturalOrder()) + .compare(o1, o2); + } + + @Override + public Comparator reversed() { + // Custom reversed: favorites still first, but reversed within + // groups + return (o1, o2) -> { + boolean fav1 = isFavorite(o1); + boolean fav2 = isFavorite(o2); + + if (fav1 != fav2) { + // Favorites always come first + return fav1 ? -1 : 1; + } + // Within the same group, reverse the natural order + return Comparator. reverseOrder().compare(o1, o2); + }; + } + }; + + column.setComparator(customComparator); + + // Get comparators for both directions + SerializableComparator ascComparator = column + .getComparator(SortDirection.ASCENDING); + SerializableComparator descComparator = column + .getComparator(SortDirection.DESCENDING); + + // Test ascending sort + // Favorites (even) should come first: 2, 4, then non-favorites: 1, 3 + Assert.assertTrue("Even 2 should come before odd 1 in ascending", + ascComparator.compare(2, 1) < 0); + Assert.assertTrue("Even 4 should come before odd 3 in ascending", + ascComparator.compare(4, 3) < 0); + Assert.assertTrue( + "Within favorites, 2 should come before 4 in ascending", + ascComparator.compare(2, 4) < 0); + Assert.assertTrue( + "Within non-favorites, 1 should come before 3 in ascending", + ascComparator.compare(1, 3) < 0); + + // Test descending sort with custom reversed implementation + // Favorites (even) should still come first: 4, 2, then non-favorites: + // 3, 1 + Assert.assertTrue( + "Even 2 should come before odd 1 in descending (custom reversed)", + descComparator.compare(2, 1) < 0); + Assert.assertTrue( + "Even 4 should come before odd 3 in descending (custom reversed)", + descComparator.compare(4, 3) < 0); + Assert.assertTrue( + "Within favorites, 4 should come before 2 in descending", + descComparator.compare(4, 2) < 0); + Assert.assertTrue( + "Within non-favorites, 3 should come before 1 in descending", + descComparator.compare(3, 1) < 0); + } + + @Test + public void testSerializableComparatorPreservesOriginal() { + Grid grid = new Grid<>(); + Column column = grid.addColumn(ValueProvider.identity()); + + // Create a serializable comparator with custom reversed + SerializableComparator serializableComparator = new SerializableComparator() { + @Override + public int compare(Integer o1, Integer o2) { + // Even numbers first, then natural order + boolean even1 = o1 != null && o1 % 2 == 0; + boolean even2 = o2 != null && o2 % 2 == 0; + if (even1 != even2) { + return even1 ? -1 : 1; + } + return Integer.compare(o1, o2); + } + + @Override + public Comparator reversed() { + // Custom reversed: evens still first, but reversed within + // groups + return new SerializableComparator() { + @Override + public int compare(Integer o1, Integer o2) { + boolean even1 = o1 != null && o1 % 2 == 0; + boolean even2 = o2 != null && o2 % 2 == 0; + if (even1 != even2) { + return even1 ? -1 : 1; + } + // Within groups, reverse order + return Integer.compare(o2, o1); + } + }; + } + }; + + column.setComparator(serializableComparator); + + // Get comparators for both directions + SerializableComparator ascComparator = column + .getComparator(SortDirection.ASCENDING); + SerializableComparator descComparator = column + .getComparator(SortDirection.DESCENDING); + + // Test that ascending uses original comparator + Assert.assertTrue("2 (even) should come before 1 (odd)", + ascComparator.compare(2, 1) < 0); + Assert.assertTrue("2 should come before 4 within evens", + ascComparator.compare(2, 4) < 0); + + // Test that descending uses custom reversed + Assert.assertTrue("2 (even) should still come before 1 (odd) in desc", + descComparator.compare(2, 1) < 0); + Assert.assertTrue("4 should come before 2 within evens in desc", + descComparator.compare(4, 2) < 0); + } + private void assertEqualColumnClasses(Class columnClass, Class compareTo) { assertNotNull(columnClass); Assert.assertEquals(compareTo, columnClass); From 6b946befe93e22053e483e545fcac6166ad8df8f Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Sat, 4 Oct 2025 19:14:46 +0300 Subject: [PATCH 2/3] fix: respect custom reversed() comparator in Grid columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes issue where Grid ignored custom reversed() implementations when sorting columns in descending order. Now stores both the comparator and its reversed version at assignment time, preserving custom reverse logic (e.g., for "favorites" that stay on top regardless of direction). Fixes #3926 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../com/vaadin/flow/component/grid/Grid.java | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index bcbd2acdd52..1516341806b 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -445,7 +445,8 @@ public static class Column extends AbstractColumn> { return Stream.of(new QuerySortOrder(key, direction)); }; - private Comparator comparator; + private SerializableComparator comparator; + private SerializableComparator reversedComparator; private Registration columnDataGeneratorRegistration; private Registration editorDataGeneratorRegistration; @@ -735,7 +736,22 @@ public Element getElement() { public Column setComparator(Comparator comparator) { Objects.requireNonNull(comparator, "Comparator must not be null"); setSortable(true); - this.comparator = comparator; + + // Store the main comparator + if (comparator instanceof SerializableComparator) { + this.comparator = (SerializableComparator) comparator; + } else { + this.comparator = comparator::compare; + } + + // Compute and store the reversed comparator + Comparator reversed = comparator.reversed(); + if (reversed instanceof SerializableComparator) { + this.reversedComparator = (SerializableComparator) reversed; + } else { + this.reversedComparator = reversed::compare; + } + return this; } @@ -783,18 +799,7 @@ public SerializableComparator getComparator( setSortable(true); boolean reverse = sortDirection != SortDirection.ASCENDING; - if (reverse) { - Comparator reversed = comparator.reversed(); - if (reversed instanceof SerializableComparator) { - return (SerializableComparator) reversed; - } - return reversed::compare; - } - - if (comparator instanceof SerializableComparator) { - return (SerializableComparator) comparator; - } - return comparator::compare; + return reverse ? reversedComparator : comparator; } /** From d386d3a30aa45c673a81d7e74068ed7dbdb5dd01 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Mon, 6 Oct 2025 09:41:33 +0300 Subject: [PATCH 3/3] Extract hack to method --- .../main/java/com/vaadin/flow/component/grid/Grid.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java index 1516341806b..50d504421bc 100755 --- a/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java +++ b/vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java @@ -737,6 +737,11 @@ public Column setComparator(Comparator comparator) { Objects.requireNonNull(comparator, "Comparator must not be null"); setSortable(true); + internalSetComparator(comparator); + return this; + } + + void internalSetComparator(Comparator comparator) { // Store the main comparator if (comparator instanceof SerializableComparator) { this.comparator = (SerializableComparator) comparator; @@ -751,8 +756,6 @@ public Column setComparator(Comparator comparator) { } else { this.reversedComparator = reversed::compare; } - - return this; } /** @@ -1881,7 +1884,7 @@ protected > C addColumn( item -> formatValueToSendToTheClient( applyValueProvider(valueProvider, item))), columnFactory); - ((Column) column).comparator = ((a, b) -> compareMaybeComparables( + column.internalSetComparator((a, b) -> compareMaybeComparables( applyValueProvider(valueProvider, a), applyValueProvider(valueProvider, b))); return column;