Bug 316356

Comment 1 Deepak Azad 2010-06-09 14:45:16 EDT

(In reply to comment #0)
> Right now control-drag into a call hierarchy does nothing.
Ctrl-Drag (or even plain drag) opens the call hierarchy on the dragged item.

Maybe Ctrl Drag can 'add' the new item to the view and not remove existing items while simply dragging an item opens the call hierarchy on only the new item.

Comment 2 Dani Megert 2010-06-10 08:11:44 EDT

Good idea. Bug 304135 might also help you.

Comment 3 Raksha Vasisht 2011-01-11 06:54:33 EST

Created attachment 186476 [details]
Patch

This patch now enables you to use Ctrl-drag to add to the existing hierarchy or open a new hierarchy if the current view is empty. 

As discussed, when the new input is dragged/ctrl-dragged on a pinned view, it opens a new hierarchy or adds to the hierarchy in the same view since the drag drop operation is destination specific and drop target is not changed for these operations unlike the open call hierarchy action.

Markus, could you pls review?

Comment 4 Markus Keller 2011-01-17 12:50:32 EST

- The project has not yet been switched to 1.5, so you have to enclose type arguments with /*...*/, or your patch causes compile errors.

- When you drag an item from the CH view, dropping it onto any element shown in the Call Hierarchy should stay a no-op (otherwise it's too easy to become confused when you accidentally drag an item when you meant to just select it). Dropping to the white area should still work and Ctrl+dropping should add the element to the roots. Dropping elements from other views can work anywhere (but it's also OK to only allow dropping to white space, as it was before).

- Shift+dropping an element from another view shows the move cursor but then adds the elements. The move cursor should never be shown.

- CallHierarchyTransferDropAdapter should only call setSelectionFeedbackEnabled(..) and setExpandEnabled(..) with argument 'false'. I know the old implementation expands in some cases but that doesn't make sense.

- When I add another input element, the view description is updated, but the history list only shows the new element (and it really contains only that when I select it).

- When I drop to the location viewer, the cursor changes on Ctrl, but adding elements doesn't work.

- The constructor of CallHierarchyTransferDropAdapter takes a StructuredViewer and stores it to fViewer. That's completely unnecessary, see getViewer(). If you really need the CallHierarchyViewer, then make the constructor type safe and don't just cast below. But I think the DropAdapter should not have to know the internals of the CallHierarchyViewer at all. Can't you put all that code into CallHierarchyViewPart? That's the central entity that manages the view, so it should should also manage changes to the input elements. You should not break the separation of concerns / information hiding.


- TreeRoot#addRoots(MethodWrapper[]): The implementation is correct, but it's an odd mix between using collections and manual pointer ops. To stay with a single technique, either use
		newRoots.addAll(Arrays.asList(fRoots));
etc., or use the "arraymerge" code template.

- CallHierarchyViewPart#addInputElements(IMember[]): Same 'for' loops as above.

Comment 5 Raksha Vasisht 2011-01-18 06:39:24 EST

Created attachment 186991 [details]
Patch_v2

Patch with the above mentioned changes.

Comment 6 William R. Burdick Jr. 2011-01-20 12:41:19 EST

(In reply to comment #4)
> - When you drag an item from the CH view, dropping it onto any element shown in
> the Call Hierarchy should stay a no-op (otherwise it's too easy to become
> confused when you accidentally drag an item when you meant to just select it).
> Dropping to the white area should still work and Ctrl+dropping should add the
> element to the roots. Dropping elements from other views can work anywhere (but
> it's also OK to only allow dropping to white space, as it was before).

Since dropping into the CH view always just adds things to the top level, why not just say that dropping an item that is already there is a noop?  It doesn't seem like it should matter where you drop something.

Comment 7 Raksha Vasisht 2011-01-21 02:03:10 EST

(In reply to comment #6)
> (In reply to comment #4)
> > - When you drag an item from the CH view, dropping it onto any element shown in
> > the Call Hierarchy should stay a no-op (otherwise it's too easy to become
> > confused when you accidentally drag an item when you meant to just select it).
> > Dropping to the white area should still work and Ctrl+dropping should add the
> > element to the roots. Dropping elements from other views can work anywhere (but
> > it's also OK to only allow dropping to white space, as it was before).
> 
> Since dropping into the CH view always just adds things to the top level, why
> not just say that dropping an item that is already there is a noop?  It doesn't
> seem like it should matter where you drop something.

When you expand one of the top level elements drill down multiple levels and want to make any element down the stack into a top level element , you can drag and drop it inside the view itself. This is actually desirable since its the easiest way to do it(without having to open the element) but might get confusing when you accidentally drag an item while only trying to just select it. So to enable the drag and drop option but to prevent any unintentional actions, we allow dropping into the white area but not on an existing element.

Comment 8 Markus Keller 2011-01-24 10:06:52 EST

(In reply to comment #5)
The patch still has compile errors. Please revert .settings to latest from HEAD.

Now, I can't drop items to the hierarchy at all any more (to the white space on the right of existing items). Dropping to the location viewer seems to work, but that's not enough.

The added element(s) should be selected and revealed on Ctrl+drop. Otherwise, it's hard to find them, since the roots are sorted.

CallHierarchyViewPart#updateViewWithAddedElements(IMember[]):
Why do we need "hierarchyViewer.refresh(roots[i]);" there?

Comment 9 Raksha Vasisht 2011-01-24 12:51:24 EST

Created attachment 187452 [details]
Patch_v3

(In reply to comment #8)
> (In reply to comment #5)
> The patch still has compile errors. Please revert .settings to latest from
> HEAD.
> 
Sorry about that.. I set the JRE to 1.5 and dint see the @override annotations in the patch.  
> Now, I can't drop items to the hierarchy at all any more (to the white space on
> the right of existing items). Dropping to the location viewer seems to work,
> but that's not enough.
> 
Changed determineTarget(..) which allows drop on white space next to the elements as well as below the elements.

> The added element(s) should be selected and revealed on Ctrl+drop. Otherwise,
> it's hard to find them, since the roots are sorted.
Done. Selects and reveals all the new elements added as roots.
> 
> CallHierarchyViewPart#updateViewWithAddedElements(IMember[]):
> Why do we need "hierarchyViewer.refresh(roots[i]);" there?

Yep not needed, hierarchyViewer.setExpandedState(roots[i], true) should do.

Comment 10 Markus Keller 2011-01-24 13:23:36 EST

(In reply to comment #9)
> Created attachment 187452 [details] [diff]
> Patch_v3

Looks good. Please inline CallHierarchyTransferDropAdapter#LINK and release.

Comment 11 Raksha Vasisht 2011-01-24 13:35:10 EST

(In reply to comment #10)
> (In reply to comment #9)
> > Created attachment 187452 [details] [diff] [details] [diff]
> > Patch_v3
> 
> Looks good. Please inline CallHierarchyTransferDropAdapter#LINK and release.

Commited to HEAD with the change.

Comment 12 Dani Megert 2011-01-28 02:58:28 EST

Verified in I20110127-2034.
Found one minor issue (see bug 335657).