Fix sorting of place types in generated getTokenAndPrefix
The use of a TreeMap with a Comparator that falls back to type name for
unrelated types can lead to related types ending up in different branches,
and the generated getToken returning the wrong token (for a supertype,
when a more specific types exists).
Sorting by "flattened supertype hierarchy size" works here because:
* Place is a class (rather than an interface),
* the compared types are guaranteed to extend the Place type, and
* the input to the getTokenAndPrefix is a Place too.
Original idea by larsaaslin@gmail.com.
Bug: issue 8086
Change-Id: I9295c9546839aa4cf87041c3f508acd575ffccfb
diff --git a/user/src/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparator.java b/user/src/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparator.java
index 2a993cd..f51165e 100644
--- a/user/src/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparator.java
+++ b/user/src/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparator.java
@@ -1,12 +1,12 @@
/*
* Copyright 2010 Google Inc.
- *
+ *
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
- *
+ *
* http://www.apache.org/licenses/LICENSE-2.0
- *
+ *
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
@@ -16,6 +16,7 @@
package com.google.gwt.place.rebind;
import com.google.gwt.core.ext.typeinfo.JClassType;
+import com.google.gwt.thirdparty.guava.common.collect.ComparisonChain;
import java.util.Comparator;
@@ -23,18 +24,15 @@
* Sorts types from most derived to least derived, falling back to alphabetical
* sorting.
*/
-public class MostToLeastDerivedPlaceTypeComparator implements
- Comparator<JClassType> {
+class MostToLeastDerivedPlaceTypeComparator implements Comparator<JClassType> {
public int compare(JClassType o1, JClassType o2) {
if (o1.equals(o2)) {
return 0;
}
- if (o1.isAssignableFrom(o2)) {
- return 1;
- }
- if (o1.isAssignableTo(o2)) {
- return -1;
- }
- return o1.getQualifiedSourceName().compareTo(o2.getQualifiedSourceName());
+ return ComparisonChain.start()
+ // We want the longest first, so we put o2 before o1 in the comparison chain.
+ .compare(o2.getFlattenedSupertypeHierarchy().size(), o1.getFlattenedSupertypeHierarchy().size())
+ .compare(o1.getQualifiedSourceName(), o2.getQualifiedSourceName())
+ .result();
}
-}
\ No newline at end of file
+}
diff --git a/user/test/com/google/gwt/place/impl/PlaceHistoryMapperGeneratorTest.java b/user/test/com/google/gwt/place/impl/PlaceHistoryMapperGeneratorTest.java
index 245ab8a..eb1b557 100644
--- a/user/test/com/google/gwt/place/impl/PlaceHistoryMapperGeneratorTest.java
+++ b/user/test/com/google/gwt/place/impl/PlaceHistoryMapperGeneratorTest.java
@@ -49,6 +49,11 @@
PlaceHistoryMapperWithFactory<TokenizerFactory> {
};
+ // Note: tokenizer order here is important for the test!
+ @WithTokenizers({Tokenizer2.class, Place1.Tokenizer.class, Tokenizer3.class})
+ interface SortOrder extends PlaceHistoryMapper {
+ }
+
/**
* The goal is only to test that the generator doesn't fail (but doesn't
* generate anything either).
@@ -116,6 +121,16 @@
assertNull(subject.getPlace(null));
}
+ /**
+ * See https://code.google.com/p/google-web-toolkit/issues/detail?id=8036
+ */
+ public void testSortOrder() {
+ PlaceHistoryMapper subject = GWT.create(SortOrder.class);
+ assertEquals(Place1.Tokenizer.PREFIX + ":" + place1.content, subject.getToken(place1));
+ assertEquals("Place2:" + place2.content, subject.getToken(place2));
+ assertEquals("Place3:" + place3.content, subject.getToken(place3));
+ }
+
// CHECKSTYLE_OFF
private void doTest(AbstractPlaceHistoryMapper<?> subject,
TokenizerFactory factory) {
diff --git a/user/test/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparatorTest.java b/user/test/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparatorTest.java
index 6de702d..11a2e8c 100644
--- a/user/test/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparatorTest.java
+++ b/user/test/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparatorTest.java
@@ -37,8 +37,10 @@
import java.io.PrintWriter;
import java.util.Arrays;
+import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
+import java.util.List;
import java.util.Set;
/**
@@ -138,14 +140,37 @@
}
public void testFallbackToClassName() {
- // Array sorted from least derived to most derived. In each pair of adjacent
- // values, neither place extends the other.
- JClassType[] places = {place1, place2, place3, place4, place5};
- for (int i = 0; i < places.length - 1; i++) {
- assertEquals(-1, (int) Math.signum(comparator.compare(places[i],
- places[i + 1])));
- assertEquals(1, (int) Math.signum(comparator.compare(places[i + 1],
- places[i])));
+ JClassType[][] places = {
+ {place3, place4}, // place3 and place4 both extend directly from place1
+ {place1, place2}, // place1 and place2 both extend directly from place
+ };
+ for (JClassType[] pair : places) {
+ assertEquals(-1, (int) Math.signum(comparator.compare(pair[0], pair[1])));
+ assertEquals(1, (int) Math.signum(comparator.compare(pair[1], pair[0])));
}
}
+
+ public void testCollectionSort() {
+ List<JClassType> actual = Arrays.asList(place, place1, place3, place5);
+ Collections.sort(actual, comparator);
+ assertEquals(Arrays.asList(place5, place3, place1, place), actual);
+
+ actual = Arrays.asList(place5, place3, place1, place);
+ Collections.sort(actual, comparator);
+ assertEquals(Arrays.asList(place5, place3, place1, place), actual);
+
+ actual = Arrays.asList(place, place1, place2, place3, place4, place5);
+ Collections.sort(actual, comparator);
+ assertEquals(Arrays.asList(place5, place3, place4, place1, place2, place), actual);
+
+ actual = Arrays.asList(place5, place4, place3, place2, place1, place);
+ Collections.sort(actual, comparator);
+ assertEquals(Arrays.asList(place5, place3, place4, place1, place2, place), actual);
+
+ // This is equivalent to the test-case from issue 8036
+ // https://code.google.com/p/google-web-toolkit/issues/detail?id=8036
+ actual = Arrays.asList(place2, place1, place3);
+ Collections.sort(actual, comparator);
+ assertEquals(Arrays.asList(place3, place1, place2), actual);
+ }
}
