Bug 126459 - [Common Navigator] CommonViewer should use TreePathViewerSorter
Summary: [Common Navigator] CommonViewer should use TreePathViewerSorter
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.2 M6   Edit
Assignee: Michael D. Elder CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on: 126506
Blocks: 126659
  Show dependency tree
 
Reported: 2006-02-04 14:53 EST by Michael Valenta CLA
Modified: 2006-02-24 14:30 EST (History)
3 users (show)

See Also:

Attachments
Patch to add TreePathViewerSorter to JFace (30.62 KB, patch)
2006-02-05 19:46 EST, Michael Valenta CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Valenta CLA 2006-02-04 14:53:14 EST 
The spec of CommonViewerSorter states that it must be used with a CommonViewer. However, there is nothing in the class that has a depencency on CommonViewer. The only dependency is on INavigatorContentService. Unless there is some reason to do so, I think that CommonViewer should be a convenience class and there should be no reference to it from the INavigatorContentService, INavigatorActionService or CommonViewerSorter.

Here is the reason for my request. One of the things we have in Team is a global synchronization wizard. From this, the user can browse the workspace and select what resources they want to synchronize. I have extended this to use the Common Navigator framework. The tricky part is that the tree is a ContainerCheckedTreeViewer. I was able to use an INavigatorContentService with this viewer with no problems. I want to use the CommonViewerSorter as well and there does not appear to be anything in the implementation that would prevent it's use.
Comment 1 Michael Valenta CLA 2006-02-04 15:34:45 EST 
The biggest connection between the CommonViewerSorter and CommonViewer seems to be that the viewer needs to provide the sorter with the parent when sorting. Given the recent work on support for TreePaths in the JFace tree viewer, I think it would be worthwhile to create a TreePath based sorter and have it live in JFace. An added benefit of this would be that the CommonViewer would not need to override as many methods.

Nick and Michael, does this seem reasonable? If it does, I can help with the implementation if needed. I've been looking at support for TreePath based content providers already (see bug 122756).
Comment 2 Michael Valenta CLA 2006-02-04 15:48:51 EST 
As for the use of the INavigatorContentService in a CheckboxTreeViewer (i.e. not a subclass of CommonViewer), can you think of any gotchas I should be aware of? I haven't seen any problems yet.
Comment 3 Michael Valenta CLA 2006-02-05 19:46:24 EST 
Created attachment 34166 [details]
Patch to add TreePathViewerSorter to JFace

Here's the patch that adds the TreePathViewerSorter to JFace and converts the CommonViewerSorter to use that sorter (and removes the copied code from CommonViewer).
Comment 4 Michael Valenta CLA 2006-02-05 19:52:06 EST 
I have logged bug 126506 to add the TreePathViewerSorter to JFace
Comment 5 Michael D. Elder CLA 2006-02-23 11:11:23 EST 
This was also scoped for M5. The change is required to make better use of JFace API that was introduced in M5.
Comment 6 Mike Wilson CLA 2006-02-23 11:43:50 EST 
+1 to applying the patch.
Comment 7 Michael D. Elder CLA 2006-02-23 13:21:16 EST 
Released the patch. Added a work around due to bug 129193.
Comment 8 Michael D. Elder CLA 2006-02-23 13:59:33 EST 
Fixed.