Community
Participate
Working Groups
The DND API needs to be cleaned up and take into account the code review comments from bug 116460.
If you need a higher-bandwidth discussion about best practices for DnD, Eric has offerred to help out.
Some minor changes to the DND API which were scoped for 3.2 M5 are now ready to be released. These changes are going to be required by downstream clients that wish to define DND handlers. There are no known existing clients that should be broken as a result of these changes.
Created attachment 35125 [details] The patch for org.eclipse.ui.navigator for anyone that wishes to review it.
Created attachment 35126 [details] The patch for org.eclipse.ui.navigator.resources for anyone that wishes to review it. A reference implementation for the Resource extension is also provided.
I've asked Eric if he can review the API.
Review comments from Eric Moffatt: Michael, here's my first cut at some more in-depth questions and a few notes about the what I -think- the implementation doing (just to see if I understand how the parts fit together...;-). Overall this looks -much- better than the first cut I saw...nice! Aside from the issues I mention below I don't see any glaring architectural issues here and it appears to cover First a few 'nits', then a few 'notes'... NITS: plugin.xml: This appears to have a stale reference to an extension point "dropHandler"; there's no "schema/dropHandler.exsd"...?? navigatorContent.exsd: Why is the extension point called 'commonDropAdapter' here but the one in 'viewer.exsd' is called 'dragAssistant'? Since it states that it requries a CommonDropAdapterAssistant perhaps 'dropAssistant' would be better. In general we've found that using 'priority' to sort out clashes turns out to be a scalability issue but I don't have an alternative...does anyone else? viewer.exsd: The API doc for 'dragAssistant' says that the class should provide an instance of 'IDragAssistant'...I'm betting that this should be 'CommonDragAdapterAssistant'. The schema for the 'class' attribute doesn't specify 'CommonDragAdapterAssistant' as the class supertype (i.e. clicking 'class:' in the PDE editor does not fill in the superclass...). CommonDragAdapter: The method 'getSupportDragTransfers' should be 'getSupportedDragTransfers'. CommonDropAdapter: I'm still a bit concerned that this 'publicizes' the ViewerDropAdapter's protected methods. I know -why- it's necessary (multiple discreet assistants, each needing the info) but I'm not sure that this is the right approach (Nick/McQ ?). CommonDragAdapterAssistant: The JDoc for 'getSupportedTransferTypes' says: "When a match is found for a particular * {@link DragSourceEvent}, * {@link #setDragData(DragSourceEvent, IStructuredSelection)} will be * called directly after." You might want to update this to be more specific that -only- when the 'drop' event occurs will the source be asked to supply the data (as appears to be the case, a good thing (tm)...;-). Notes: Can clients use the 'old style' PluginDropAdapter extension with these viewers? If so, does this cause any confusion? The API is intended to be fully re-usable for 'custom' viewer implementations. Do these other viewers have to be derivatives (subclasses) of the Common Navigator ? CommonViewer's 'initDragAndDrop' instantiates both 'CommonDragAdapter' and 'CommonDropAdapter'. Clients providing custom support would override this method if they want to provide subclassed implementations of these two classes (do they have to be subclasses?). Michael, you may want to have the implementation of 'CommonDragAdapter:dragSetData' return after the first valid 'assistant' has added the data; the current impl can result in multiple (perhaps costly) assistant's 'setDragData' being called.
I will produce a patch to this defect to respond to these comments. Re NITS) This is a stale reference; will remove it. Agree with the name change for commonDropAdapter; will make the change. The priority discussions have come up before, and I agree there isn't a known pattern that gets around this for the general case; the priority values employed by the CN are really for clients that are coordinating their extensions; it's more like the "relevance" setting for JDT quick fix listings than a true "priority". The API doc for viewer.exsd will be updated accordingly. Will rename to getSupportedDragTransfers. The ViewerDropAdapter is subclassable API; and it's allowed for clients to have dependencies to the methods which are "publicized". In this case, the overridden methods are just pass through to the super methods. I don't see this as a big risk, but if there is other feedback about why it might be bad (with an alternative proposal), I'm open to it. I will update the doc for getSupportedTransferTypes() accordingly. Re Notes) Clients that are dropping onto the viewer could use the PluginTransfer type; it's is a supported drop transfer type. Clients cannot really use PluginTransfer directly in the outbound direction; the Common Navigator already does this, and delegates to interested clients (see CommonDropAdapterAssistant#handle|validatePluginTransferDrop()) For clients that wish to take advantage of the DND support in custom viewers (non-CommonViewer Viewers), they can re-use CommonDragAdapter and CommonDropAdapter. Each of these adapters are final and cannot be extended. Clients can either subclass CommonViewer and override initDragAndDrop with their own level of support, or they can have a completely different viewer that basically copies the code in CommonViewer.initDragAndDrop() (which is all API based since it only relies on CommonDragAdapter and CommonDropAdapter). Clients could even override this method and just change one of the adapters (use CommonDragAdapter with their own custom drop support, for instance). There are no requirements that CommonDragAdapter and CommonDropAdapter be used as a pair. I can have it return. I may suggest changing the signature to return a boolean value if the assistant set the data. After the first assistant to return true completes, CommonDragAdapter.dragSetData() will return.
Created attachment 35313 [details] org.eclipse.ui.navigator
Created attachment 35314 [details] org.eclipse.ui.navigator.resources
Patches to address these concerns have been released. I'm prepared to release them when approved.
By "have been released", I meant added to this defect. Do I have the necessary approval to release these changes to the repo?
Michael, everything looks good to me except that the 'dropAssistant' extension in org.eclipse.ui.navigator.navigatorContent still has a few nits: 1) It sets the superclass to CommonDragAdapterAssistant rather than CommonDropAdapterAssistant. 2) The schema description doesn't specify any help for the dropAssistant's 'id' and 'class' fields (so no tooltips appear)... 3) You might want to review the description itself; it appears (to me) that you're using 'commonDropAdapter' once where it should be 'CommonDropAdapterAssistant' (first use) and once where it should be 'dropAssistant' (last use).
I've made those changes locally now so should I go ahead and release all of it?
Assuming Eric's concerns have been addressed, you should release it. I'm hoping we are almost done cleaning up these "API that got delayed" issues. +1 for the API changes.
Released.
