Bug 261606

Comment 1 Rob Stryker 2009-01-21 01:58:49 EST

Or, just removing the silly restriction that the dragged object must be an allowed child.  

I'd say since the CommonDropAdapterAssistant already has validate methods in it, we should leave it up to the CommonDropAdapterAssistant (and its subclasses) to say whether the dragged object makes sense or not.  

Comment 2 Max Rydahl Andersen 2009-01-21 07:53:22 EST

Yes, limiting possible drop candidates to only allowed children is too limiting. Things that get dropped is often things that needs conversion to become a child.

Comment 3 Francis Upton IV 2009-02-04 18:17:26 EST

I'm not sure I understand the issue here.

The CommonDragAdapterAssistant subclass is selected based on the dropAssistant extension point.  This selection is based on the possibleChildren expression of the enclosing navigatorContent.  Alternatively the dropAssistant can be used outside of a navigatorContext extension.

In either case a further granularity of selection is done though the possibleDropTargets expression associated with the dropAssistant.

Either the possibleDropTargets or possibleChildren expressions can specify a wide range of conditions which determines which drop adapter assistant is selected, including simply true (an empty condition results in true [I know this is not commonly known]), or being able to adapt to an object.

I have a feeling you might be confused by the fact that the name of the element is "possibleChildren".  This really has nothing to do with the children of the element, it's just an expression which is used to select the content extension for drop handling purposes.  The documentation needs to be clarified more on this point, and I will do that.

Please reopen this if I'm missing the point here, but I think you have everything you need to do what you want.

Comment 4 Francis Upton IV 2009-02-04 18:18:14 EST

See comment above.

Comment 5 Rob Stryker 2009-02-05 01:03:40 EST

The "possibleChildren" portion of the framework is there to choose the proper navigatorContent that an object could belong to. This is done for efficiency purposes (as I understand it).  If you have 10 extensions, and your object is an IFile, instead of checking all 10 extensions you can just check the ones that claim support for an IFile. (This is also important because content providers do not on their own provide an API to see if some object could be a child or not. This is added value and important to the CNF framework.)

If I were to evaluate to simply "true" in my "possibleChildren" section, it would remove this efficiency bonus... and so that's something I don't wish to do. I also cannot currently use the "adapt" functionality because I do not wish to register a global adapter. I kinda wanted my "drop adapter" to do the adapting ;) 

Upon reflection, I suppose what I'm really suffering from here, then, is a dislike of how much of the framework seems to be in the xml unnecessarily. I realize this might put me in the minority. I also have this vague sense that 3 levels of approval for a drop assistant is too much.

Level 1) are you a possible child in the navigatorContent?
Level 2) are you also approved in the possibleDropTargets section
Level 3) are you FURTHER approved by the validateDrop method in the CommonDropAdapterAssistant.

Wouldn't it just be easier to let the CommonDropAdapterAssistant just say whether it approves of the drop or not in the validateDrop() method, and if it doesn't, try the next one?

I completely understand the possibleChildren methodology in the view from the efficiency standpoint, and to locate which navigatorContent is the proper one for some object. The added xml configuration exists to speed up and make more efficient the framework as a whole.  

But in the case of drop adapters, I simply do not see such an efficiency boost to justify the verbosity, added complexity, and redundancy. "possibleChildren" will already be used to find the proper navigatorContent for the drop target. I believe that finding the proper navigatorContent is 90% of the work. 

Once you have the proper navigatorContent, however, the rest of the work is fairly simple. Get a list of drop adapters, find one that can handle your object, and go with it. The added levels of checking (checking if it fits in possibleChildren, checking if it fits in possibleDropTargets) do not serve any speed enhancement or provide anything of value that simply implementing "validateDrop" could not have provided. 

There's usually the fight (at least in there used to be, in JEE) over whether you should do things via code or via a descriptor file. It seems that common navigator (in the case of drop adapters) has opted to force the adopter to do both, filling in lots of xml and then also still implementing a validateDrop method. 

If drop assistants did not have a validate method, then all this extra xml checking would be necessary. Or if it did have a validate method, but adding the xml removed the need to implement it, that'd also be ok. (I wouldn't like it, but you'd be choosing configuration over code, which is a fine decision... I just happen to not agree with it ;)) 

But the API to "validate" is already inside the drop adapter assistant, and using all the xml configuration does not remove the need to implement validate()... so I see it as unnecessary and in fact non-valuable. 

I would strongly support a change in API such that the CommonDropAdapterAssistant takes over the responsibility of declaring whether they approve of the drop or not and the possibleChildren / possibleDropTargets points are deprecated. 

I'm sure you'll simply close this bug again ;) But I just re-opened it so you'd make sure to see it. 

Comment 6 Francis Upton IV 2009-02-05 01:17:22 EST

Rob, I think you are missing a very important point in your arguments.  While I also hate the XML configuration world, one of the main points of it is to avoid loading plugin code where possible.  We expect eclipse environments to run with thousands of plugins most of which are not in use at any given time.  In a component like the CNF, it's important to only load the plugins that are actually required, and that's the whole point of the configuration and the declarative testing in the XML.

If we went with your suggestion, we would be forced to load all possible plugins to evaluate a something dragged over a CNF object, which would be terribly inefficient because there could be 10s or even hundreds depending on what the platform is being used for.

This dynamic loading plugins only when required is one of the key strengths of the Eclipse platform and the reason it can perform very well even with a lot of unrelated components.

So, yes, I am going to close this again.  You don't need to reopen it for me to read your comments, I always read comments on bugs that are assigned to me.  I may not react to them right away, but I always follow them.