Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(240)

Issue 1895803: Fixes #8036, properly sorting of places in PlaceHistoryGeneratorContext

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by aaslin
Modified:
3 years, 11 months ago
Reviewers:
tbroyer, mdempsky, goktug
CC:
google-web-toolkit-contributors_googlegroups.com
Base URL:
http://google-web-toolkit.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Given the following places:

public class A extends Place {
  
  @Prefix("placeA")
  public static class Tokenizer implements PlaceTokenizer<A> {
    ...
  }
}

public class B extends Place {
  
  @Prefix("placeB")
  public static class Tokenizer implements PlaceTokenizer<B> {
    ...
  }
}

public class C extends A {
  
  @Prefix("placeC")
  public static class Tokenizer implements PlaceTokenizer<C> {
    ...
  }
}

... and the following PlaceHistoryMapper definition:

@WithTokenizers({B.Tokenizer.class, A.Tokenizer.class, C.Tokenizer.class})
interface AppPlaceHistoryMapper extends PlaceHistoryMapper {
};

PlaceHistoryMapper.getToken will return the following tokens for respective
place

A -> "placeA"
B -> "placeB"
C -> "placeA" <-- Should be "placeC"

The reason for this is the way the places are sorted in
PlaceHistoryGeneratorContext, later used in PlaceHistoryMapperGenerator
(line:108) for generating the implementing class. In
PlaceHistoryGeneratorContext, the usage of MostToLeastDerivedPlaceTypeComparator
in conjuction with a TreeMap creates a possibility for "related" places to end
up in different branches in the TreeMap. Therefor, the correctness of the
sorting depends on in which order the places are added to the TreeMap, which
depends on in which order the PlaceTokenizers are declared in @WithTokenizers.

To make MostToLeastDerivedPlaceTypeComparator work properly in conjuction with a
TreeMap the comparission should be done on the depth of each place's inheritance
hierarchy instead of whether they are assignable to/from each other or not.

Patch Set 1

Patch Set 2 : Fixed and added additional tests

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
user/src/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparator.java View 1 chunk +2 lines, -2 lines 0 comments Download
user/test/com/google/gwt/place/impl/PlaceHistoryMapperGeneratorTest.java View 2 chunks +11 lines, -0 lines 0 comments Download
user/test/com/google/gwt/place/rebind/MostToLeastDerivedPlaceTypeComparatorTest.java View 1 7 chunks +17 lines, -10 lines 0 comments Download

Messages

Total messages: 10
tbroyer
LGTM
4 years, 1 month ago #1
aaslin
On 2013/02/26 13:51:10, tbroyer wrote: > LGTM Is there anything more I should provide regarding ...
4 years ago #2
tbroyer
We just need someone with commit rights. Calling in Matthew (at random), who will delegate ...
4 years ago #3
goktug
On 2013/04/04 14:06:13, tbroyer wrote: > We just need someone with commit rights. Calling in ...
4 years ago #4
aaslin
On 2013/04/08 23:51:51, goktug wrote: > On 2013/04/04 14:06:13, tbroyer wrote: > > We just ...
3 years, 11 months ago #5
tbroyer
On 2013/05/28 08:52:04, larsaaslin wrote: > > Can I run only MostToLeastDerivedPlaceTypeComparatorTest with an ant ...
3 years, 11 months ago #6
aaslin
On 2013/04/08 23:51:51, goktug wrote: > On 2013/04/04 14:06:13, tbroyer wrote: > > We just ...
3 years, 11 months ago #7
goktug
On 2013/05/28 19:29:46, aaslin wrote: > On 2013/04/08 23:51:51, goktug wrote: > > On 2013/04/04 ...
3 years, 11 months ago #8
aaslin
On 2013/05/29 00:33:29, goktug wrote: > On 2013/05/28 19:29:46, aaslin wrote: > > On 2013/04/08 ...
3 years, 11 months ago #9
goktug
3 years, 11 months ago #10
On 2013/05/30 19:20:02, aaslin wrote:
> On 2013/05/29 00:33:29, goktug wrote:
> > On 2013/05/28 19:29:46, aaslin wrote:
> > > On 2013/04/08 23:51:51, goktug wrote:
> > > > On 2013/04/04 14:06:13, tbroyer wrote:
> > > > > We just need someone with commit rights. Calling in Matthew (at
random),
> > who
> > > > > will delegate if needed.
> > > > 
> > > > I'm not sure if this doesn't introduce any new problems and solves all
> > > problems
> > > > with the comparator. Can you reproduce the problem in
> > > > MostToLeastDerivedPlaceTypeComparatorTest and additional test cases?
> > > 
> > > The test I added in PlaceHistoryMapperGeneratorTest.java is an example of
> > > reproducing the problem since it will fail if executed with the original
> > version
> > > of MostToLeastDerivedPlaceTypeComparator.java. I also added a new patch
set
> > that
> > > fixes MostToLeastDerivedPlaceTypeComparatorTest.java which, from what I
can
> > see,
> > > haven't been working properly before. Please let me now if there is
anything
> > > more I can do.
> > 
> > Do you mind moving your patch to http://gwt-review.googlesource.com which is
> our newer
> > code review system?
> 
> Absolutely. Is there a way to migrate this issue or do I have to create a new
> one there?

You need to create a new one there.

> > 
> > About the test: Why did you change the order in the previous test case
instead
> > of adding new one? 
> 
> Because the test will fail with the changed comparator since "places" are not
> order from most to least derived. 
> 

Hmm, I didn't look at the test case closely before now I see what is going on.
Test is actually not included in the suite before. Can you add it to PlaceSuite
so it becomes part of test harness.

> > I think it would be best to see the new comparator working in
> > as many scenarios as possible.
> > 
> > Perhaps also we can simplify this further as we touch it.  From what I can
> see,
> > the only reason we have a TreeMap is because of getPlaceTypes call. We can
> keep
> > the places in a LinkedHashMap and change getPlaceTypes to
getSortedPlaceTypes
> > and do the sort there. With that change we will avoid keeping a sorted list
> also
> > we will no longer need to be consistent with equals in a comparator (i.e.
you
> > can just do "return o2.getFlattenedSupertypeHierarchy().size() -
> > o1.getFlattenedSupertypeHierarchy().size;").
> > 
> > Anyway this refactoring is optional, I can do it later.
> 
> I'll gladly give the refactoring a go. Is there any particular reason to use a
> LinkedHashMap over just a HashMap?

To reduce randomness of behavior throughout different compilations.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld revision f51cb906c4ad+