Community
Participate
Working Groups
HEAD 1) Create java project jp1 2) Create non-java project p1 3) Create folder p1/p1f1 4) Drag p1f1 to jp1 > It copies instead of moves
The problem is easy to see from the CNF DnD tracing. It is cause by the fix to bug 261060. If I drag the p1f1 through the "JRE System Library" object, the JavaDropAssistant changes the operation to a copy (as it should). However, the operation stays a copy because once it's changed it just stays that way (which is generally what you want). Note if you drag the p1f1 *around* the JRE System Library, it correctly moves it. So I think this is going to require a bit more JFace work to fix this right. We need to save originally requested operation (which is the first non-NONE operation we see). I would propose adding two new methods to ViewerDropAdapter (and corresponding public methods to CommonDropAdapter: void overrideOperation(int operation) - this causes the operation for the current element to be overridden. If the drop happens on this element, the operation will be that of the overridden operation. If the drag moves somewhere else, the operation will be reset to the original operation. I think having this will make it easy to do what you want, which is override the operation for this one element. int getOriginalOperation() - returns the operation the user requested (this would also be added as a public method to the CommonDropAdapter). The downside of this is the other adapters will have to be smart about when to restore the original operation because someone else could have changed it, so I don't like this solution much (though I think the API would be useful). Eric, Dani, what do you think about this proposal? --- Here is the CNF tracing that shows the problem: CommonDragAdapter.dragStart (begin): DragSourceEvent{DragSource {} time=936146273 data=null operation=0 type=0 doit=true} CommonDragAdapter.dragStart source: DragSource {} CommonDragAdapter.dragStart (end): doit=true CommonDropAdapter.validateDrop (begin) operation: 2 target: P/p1 CommonDropAdapter.validateDrop valid for plugin transfer CommonDropAdapter.validateDrop (returning true) CommonDropAdapter.validateDrop (begin) operation: 2 target: P/p1 CommonDropAdapter.validateDrop target: P/p1 CommonDropAdapter.validateDrop local selection: [F/p1/p1f1] CommonDropAdapter.validateDrop checking assistant: "org.eclipse.jdt.internal.ui.navigator.JavaDropAdapterAssistant@1cb9af7 CommonDropAdapter.validateDrop VALID CommonDropAdapter.validateDrop (returning 0: OK) CommonDropAdapter.validateDrop (begin) operation: 1 target: org.eclipse.jdt.internal.ui.packageview.ClassPathContainer@6ee78191 CommonDropAdapter.validateDrop target: org.eclipse.jdt.internal.ui.packageview.ClassPathContainer@6ee78191 CommonDropAdapter.validateDrop local selection: [F/p1/p1f1] CommonDropAdapter.validateDrop (returning false) CommonDropAdapter.validateDrop (begin) operation: 1 target: org.eclipse.jdt.internal.ui.packageview.ClassPathContainer@6ee78191 CommonDropAdapter.validateDrop target: org.eclipse.jdt.internal.ui.packageview.ClassPathContainer@6ee78191 CommonDropAdapter.validateDrop local selection: [F/p1/p1f1]
*** Bug 263618 has been marked as a duplicate of this bug. ***
>Eric, Dani, what do you think about this proposal? I didn't review the recent JFace changes that cause this now (bug 261060). We should look at them again when fixing this bug here to make sure we don't break existing clients. I would not want a solution that breaks existing clients (as it does now) and requires them to react on the JFace changes.
(In reply to comment #3) > >Eric, Dani, what do you think about this proposal? > I didn't review the recent JFace changes that cause this now (bug 261060). We > should look at them again when fixing this bug here to make sure we don't break > existing clients. I would not want a solution that breaks existing clients (as > it does now) and requires them to react on the JFace changes. > I asked Boris and Eric to review the JFace changes, and Eric ended up doing it. As far as I know, the JFace changes for bug 261060 did not break existing clients. That happened only because JDT hooked up to the new API to allow the operation to be modified, that's what broke things. If you did not modify the operation, it would perform as it always did. And the new API performed as expected. We just did not think things through to see the side effect of changing the operation. My question still stands, what do you think about this proposal to change the operation only as long as the drop is on the item where the operation was changed?
>As far as I know, the JFace changes for bug 261060 did not break existing >clients. That happened only because JDT hooked up to the new API Francis, I cannot speak about this bug here you filed now as I have no time to investigate. I know though for sure that bug 263618 which I filed yesterday and which you marked as duplicate of this one, happened without the adoption of the API in JDT. I suggest you take a closer look. >what do you think about this proposal Sorry, Francis this would need a bit more time to review and I'm really out of time at the moment.
Created attachment 125183 [details] Patch
Eric/Dani, Here is the patch for this. This patch does not require any JDT UI changes (except to eventually change the name of they method called in JavaDropAdapterAssistant to overrideOperation(int) from setCurrentOperation(int), as setCurrentOperation(int) is kept temporarily for compatibility with the current JDT UI code.
Eric, your question was what would happen if I just set the lastValidOperation in overrideOperation and removed the modifications to doDropValidation? That would not work because event.detail would not be set with the overriddenOperation so the user would not get the right feedback. Another approach would be to change the currentOperation but that won't work because it would set the eventDetail which would never be reset, in fact it would cause the same situation we had now that whenever you overrode the operation it would stick forever. One thing that might ease your mind is that if you look at the changes to lastValidOperation, they are all triggered by overrideOperation being sometihng other than -1, which means they are enabled only if overrideOperation is actually used. Hope this helps.
Thanks for the update...I'll bear that safety check in mind when I get back to the patch (we've just gotta be -sure- that if the 'overrideOperation' is not called then it works -exactly- as before or we get flamed...;-).
Francis, the patch looks ok (to me) except that 'overrideOperation' isn't initialized to -1...won't that be an issue on the first call to 'doDropValidation'? Added Boris for a second look.
Created attachment 125729 [details] Updated to set overrideOperation to -1
Thanks Francis, looks good to me but we'll wait for a look from Boris.
Released to HEAD N20090302
*** Bug 119014 has been marked as a duplicate of this bug. ***
Verified in I20090428-0100
Francis, I've reset the 'review' bit since this is already in...
