Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 317532

Summary:	[CommonNavigator] CommonViewerSorter uses wrong sorter
Product:	[Eclipse Project] Platform	Reporter:	Bruno Fischel <bfische>
Component:	UI	Assignee:	Paul Webster <pwebster>
Status:	VERIFIED FIXED	QA Contact:
Severity:	normal
Priority:	P2	CC:	francisu, kdevolder
Version:	3.4.2
Target Milestone:	3.7.2
Hardware:	PC
OS:	Windows XP
Whiteboard:

Description Bruno Fischel

2010-06-21 18:41:12 EDT

Build Identifier: 3424I20100312

When comparing two objects contributed by different Navigator Extensions, the CommonViewerSorter uses the sorter that has the lowest priority (incidentally, that's the one with the highest priority number).



Reproducible: Always

Steps to Reproduce:
1. Define two Navigator Extensions (navigatorContent) with different priorities and a common sorter for each.
2. Associate the CNF with a view and display content in it.
3. Put breakpoint at line 129 of org.eclipse.ui.navigator.CommonViewerSorter (branch 3.4.2)
4. sorter with 'lowest' priority (priority == 6) is used instead of sorter with 'highest' priority (priority == 0)

Comment 1 Bruno Fischel

2010-06-21 19:36:53 EDT

There's a similar issue with:
org.eclipse.ui.internal.navigator.NavigatorPipelineService

In the for loop of the method registerContribution(), the navigator descriptor with the lowest priority (i.e. highest priority number) is associated with the new contribution (it contradict the comment on L105

Comment 2 Francis Upton IV

2010-06-21 20:43:52 EDT

This has changed substantially in 3.6, can you see if you can reproduce your issue there?

Comment 3 Kris De Volder

2011-09-27 14:02:21 EDT

(In reply to comment #2)
> This has changed substantially in 3.6, can you see if you can reproduce your
> issue there?

I'm still hitting this bug in Eclipse 3.7.1 and it is mighty annoying!
https://issuetracker.springsource.com/browse/STS-1108

This bug makes it impossible to control sorting order of content providers that add elements mixed in with existing providers, such as those from JDT.

Comment 4 Kris De Volder

2011-09-27 14:04:47 EDT

Note: this should be easy to fix in this method:

org.eclipse.ui.navigator.CommonViewerSorter.compare(Viewer, TreePath, Object, Object)

This case brach

case BOTH_UNDERSTAND:
  sorter = sourceOfLvalue.getSequenceNumber() > sourceOfRvalue.getSequenceNumber() 
? sorterService.findSorter(sourceOfLvalue, parent, e1, e2)
						: sorterService.findSorter(sourceOfRvalue, parent, e1, e2);
				break;

Comment 5 Kris De Volder

2011-09-27 14:06:46 EDT

Note: this should be easy to fix in this method:

org.eclipse.ui.navigator.CommonViewerSorter.compare(Viewer, TreePath, Object, Object)

This case branch

case BOTH_UNDERSTAND:
  sorter = sourceOfLvalue.getSequenceNumber() > sourceOfRvalue.getSequenceNumber() 
        ? sorterService.findSorter(sourceOfLvalue, parent, e1, e2)
	: sorterService.findSorter(sourceOfRvalue, parent, e1, e2);
 break;

The ">" needs to be replaced with a "<".

Comment 6 Francis Upton IV

2012-01-05 14:29:05 EST

(In reply to comment #5)
> The ">" needs to be replaced with a "<".
Duh, god how stupid of me. Yes of course this is the correct fix. Thanks for finding it.

Paul: thanks for putting it in, unfortunately I don't even have a workspace that works at atm and I have to get set up with all of the scary git stuff, so I can't really run anything. 

When you make this change, it's likely that some navigator tests will break, and hopefully the fix will be pretty obvious (I just looked through the test code and I could not see any specific sorter tests, but I think they are in there). If you have trouble with this ping me and I will try to help. Unfortunately I can't spend the time now to get all set up so that I can run these things myself.

Comment 7 Paul Webster

2012-01-06 09:01:37 EST

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9760254007e7c8a93aef708bf0ebf96c23e21c14

Released into 3.8 and 4.2 as well.

PW

Comment 8 Paul Webster

2012-01-31 13:45:10 EST

In M20120127-0800.

Kris/Bruno, you should verify with a current M build as well.

PW

Comment 9 Kris De Volder

2012-01-31 18:04:38 EST

I don't really have a way to verify this anymore. When we were hitting that bug I ended up restructuring our viewer contents so what we don't end up hitting this case anymore.