A Little Bit Confused About Drag and Drop

General JUCE discussion
1

Drag and drop is working in my Juce application, but when an item is dropped the DragAndDropTarget has this method called

itemDropped (const String& sourceDescription,
                             Component* sourceComponent,
                             int x, int y);

I’m a little bit confused about what I am supposed to do. My “drag data” is a pointer to an object. How do I retrieve the drag data? I would have to dynamic_cast<subclass*>(sourceComponent) and then call some method.

But what if there are several different subclasses of Component that can provide this drag data? The obvious solution is to make each subclass use multiple inheritance:

struct DragDataSource
{
  virtual DragData* GetTheDragData()=0;
};

struct DragDataProvidingComponent1 : Component, DragDataSource
{
  DragData* GetTheDragData();
};

struct DragDataProvidingComponent2 : Component, DragDataSource
{
  DragData* GetTheDragData();
};

This is fine, I suppose, but now I need an additional class for each type of data that I want to drag and drop. Clearly not a scalable solution, nor is it convenient.

Am I overengineering this or am I missing something obvious?

2

Yes, that’s pretty much the way I generally do it.

I wrote the drag and drop stuff a long, long time ago, and I’ve also thought in retrospect that it’d have been better for the drag initiator to create an object which gets delivered to the target when it finishes. (Although I’ve certainly had situations where it’s better to have a reference back to the originating component - e.g. when the originator might want to provide different data depending on the target). I may decide to make that change one day, though it’d require end-users to update their code.

3

Maybe you should have a look to double dispatch algorithms, and the visitor pattern.
In short, whatever the method, it’s always awful in C++.

4

I think it would be possible to engineer a solution that allows for delivering an object of class type, without breaking existing code. It would be an additional set of functions in DragAndDropTarget. Basically a duplicate of isInterestedInDragSource, itemDragEnter, itemDragMove, itemDragExit, and itemDropped. But instead of String and Component*, it would have as parameters DragData* and Component*.

A DragData would be a generic base class from which all drag data would need to be derived. When a drop target wants to see if there is something it is interested in, instead of doing string comparisons it could do a series of dynamic_cast<> for each type it is interested in.

For example, lets say that you were dragging an image and it could be delivered as a Bmp, Gif, or Jpeg. The DragData could be declared like this:

struct BmpDragData : DragData
{
  Bmp* Render();
};

// similar declarations for Gif and Jpeg

struct MyImageDragData : BmpDragData, GifDragData, JpegDragData
{
  ...
};

// returns True if there is a Bmp in the drag data
bool isInterestedInDragSource( DragData* dragData, Component* sourceComponent )
{
  return dynamic_cast<BmpDragData*>(dragData) != 0;
}

// example of handling item dropped

void itemDropped( DragData* dragData, Component* sourceComponent, int x, int y )
{
  BmpDragData* bmp;
  JpegDragData* jpeg;
  // we prefer Bmp since it is lossless
  if( bmp = dynamic_cast<BmpDragData*>(dragData) )
  {
    // handle bmp
  }
  // but we will take Jpeg otherwise
  else if( jpeg = dynamic_cast<JpegDragData*>(dragData) )
  {
    // handle jpeg
  }
}

In order to not break existing code, these new functions would be in addition to the old ones, instead of replacing them. To determine whether we should be calling the new, or the old functions, we would need to add one more function to DragAndDropContainer():

void DragAndDropContainer::StartDragging( DragData* dragData, Component* sourceComponent, ... );

This way if we are dragging the new style of object we would call the new functions in DragAndDropTarget. Existing code would use the old paths.

Also note that in my example, the drag data “promises” the ability to deliver Bmp, Jpeg, or Gif, but the data doesn’t have to be pre-generated. Instead, once the data is delivered, the recipient uses the appropriate dynamic_cast and then calls Render() on the subclass in order to produce the data in the desired format. This has nothing to do with Juce of course, but I’m just pointing out that using this technique it is possible to promise data in various formats while deferring the actual generation until it is needed.

5

Yes, that’s exactly the kind of thing that I meant.

It’d be terribly confusing to try to leave both sets of methods available though. When it’s necessary to make this kind of change I think it’s better to just go for it, making sure that existing code will fail to compile in a meaningful way, rather than causing silent errors. Probably the best option in this case would be to make the data object an optional extra parameter, that people could ignore. That way you could update old code by simply adding a parameter to your definitions, which is pretty easy to do.

6

To be honest, I really hate the dynamic_cast chain, for multiple reason:

  1. It’s very heavy (look at the asm code that stuff generated, compared to a switch(overloadedObj->getType())
  2. It’s a poor design and a pain to maintain (each time you add a new subclass, you must go to every code that pass such chain and modify it. Worst, if you forget to do so, the code still compile, but it won’t work when the such chain is called, or worst it’ll appear to work)
  3. It’s crashy. If you have a class Child : public Base, then if you test dynamic_cast before Child, then you’ll go into the base class code, while expecting the child code to run. This usually leads to crash / impossible to debug case, that pops up way later.

dynamic_cast is a kind of a C++ hack to help implementing double dispatch, but it’s only a hack. It’s useful when you only expect 1 or 2 types differencing (like the current Juce code does), but not more.
There are other way to create a better implementation of such double dispatch algorithm, without any of the issue I’ve talked above.

Please have a look here:

The advantages of such pattern is that if you either had to add a new class, the code will not compile until you’ve added the visitor for it.
It’s clear to read, as the method signature says it all, instead of having to read full pages of dynamic_cast chain
It can be refactored easily without breaking all the code.
It’s lightweight since it’s boils down to only 2 vtable lookup.

7

Has nobody ever told you the rules of optimisation?
http://c2.com/cgi/wiki?RulesOfOptimization

Personally, I don’t really care how fast a dynamic_cast is, and have never bothered looking at the asm involved. They’re just like any other language feature - a tool you can use where appropriate. Avoiding them in performance-critical tight loops is wise, but avoiding them in principle is just silly.

And I disagree with your FUD about them anyway - they’re certainly not dangerous. I don’t understand your point about the Base/Child classes, but can’t think of any way you could cast an object into a state that would crash…?

8

You usually have this:

struct Base
{
    void preventManufacturerAboutCustomerOrder();
};

struct Fruit : public Base
{
    double weight;
    Colour color;
    
    virtual float computeConsumerAttractionRate();
};

struct Apple : public Fruit
{
    RebateFurnisher * rebate;
    virtual float computeConsumerAttractionRate();


    Apple() : rebate(0) {}
};

Let’s say you application is a shopping basket, so the drag and drop is easy to follow.

Then in your D&D item dropped code, you’ll probably write something like this:

void itemDropped(Base * base)
{
    if (dynamic_cast<Fruit* >(base))
    {
        Fruit * theFruit = dynamic_cast<Fruit*>(base);
        AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping", String("Attract rate: ") << theFruit->computeConsumerAttractionRate());
    }
    else if (dynamic_cast<Apple*>(base))
    {
         Apple * apple = dynamic_cast<Apple*>(base);
         float rate = apple->computeConsumerAttractionRate();
         if (apple->rebate) rate *= 1/apple->rebate->getDiscountRate();
         AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping", String("Attract rate: ") << rate);
    }     
}

If you look at this code, it’ll look ok, but the second part (else if) will never we called. Why? Because Apple is a Fruit, but not the opposite, so the first branch will always succeed.
Ok, then, let’s fix the code so that the most derived class are in front:

void itemDropped(Base * base)
{
    if (dynamic_cast<Apple*>(base))
    {
         Apple * apple = dynamic_cast<Apple*>(base);
         float rate = apple->computeConsumerAttractionRate();
         if (apple->rebate) rate *= 1/apple->rebate->getDiscountRate();
         AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping", String("Attract rate: ") << rate);
    }     
    else if (dynamic_cast<Fruit* >(base))
    {
        Fruit * theFruit = dynamic_cast<Fruit*>(base);
        AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping", String("Attract rate: ") << theFruit->computeConsumerAttractionRate());
    }
}

Ok, now it’s working.

Let’s say later on your shop get bigger, and you decide to diversify:

struct GrannySmith : public Apple
{
    virtual float computeConsumerAttractionRate() 
    { 
         // Let's be more interesting than the other Apples
         if (Apple::computeConsumerAttractionRate() < 0.9 && !rebate) 
             rebate = new SpecialDiscountRebate(); 
         return Apple::computeConsumerAttractionRate(); 
    }
    void preventManufacturerAboutOrder()
    {   
        sendEmail("manufacturer@bob.com", String("Great discount worked, please place an order with this rebate:")<< rebate->getDiscountRate());
    }
};

First possible error:
You forgot to change the itemDropped code. The code compiles. However, you’ll never have any sale on the granny smith, since it doesn’t even appear.
Second possible error:
Someone else than you got hired to add the granny smith case, and did a search over the code. Great. He fix the itemDropped code, but, as 99% of human will do, he’ll add his code after the first else if, like this:

    if (dynamic_cast<Apple*>(base))
    {
         Apple * apple = dynamic_cast<Apple*>(base);
         float rate = apple->computeConsumerAttractionRate();
         if (apple->rebate) rate *= 1/apple->rebate->getDiscountRate();
         AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping these delicious apples", String("Attract rate: ") << rate);
    }     
    else if (dynamic_cast<Fruit* >(base))
    {
        Fruit * theFruit = dynamic_cast<Fruit*>(base);
        AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping these fruits", String("Attract rate: ") << theFruit->computeConsumerAttractionRate());
    }
    else if (dynamic_cast<GrannySmith* >(base))
    {
         GrannySmith * smith = dynamic_cast<GrannySmith*>(base);
         float rate = smith->computeConsumerAttractionRate();
         if (smith->rebate) rate *= 1/apple->smith->getDiscountRate();
         AlertWindow::showMessageBox(AlertWindow::InfoIcon, "Thanks for shopping these delicious granny smith", String("Attract rate: ") << rate);
    }
    base->preventManufacturerAboutConsumerOrder(); 
}

Same error. The added code will not work.
Worst, it’ll crash too, since the preventManufacturer… method will call the GrannySmith’s version, which expected the previous code to have allocated the Rebate.

9

I agree about the general “don’t fix what is not broken” rule.
The difference is significant, but probably not in the order that’s worth optimizing it.

However, the dynamic_cast chain was one of the bugs that I had to fix when I’ve been hired in my second programmer job. It was MFC, with all classes deriving from CObject, there was dcast everywhere, not smartly placed like in Juce, but used as a general cast operator that “does some magic” of reporting NULL if not the right type. The code crashed everywhere, as the dcast where used to mix different base types too (think about this, if the hierarchy start to contains multiple inheritance, where do you place your dynamic_cast in the if/elseif chain ?)

As a result, when I was lucky, the dynamic cast crashed at the cast place, but sometime, it actually converted the object in the base type, doing the base’s operation, while the intend was the child’s version, obviously, since it was virtual.
I’ve spent weeks fixing the code, removing the dcast, and moving to Visitor pattern. (At that time, I’ve invented a template based Visitor pattern that saved the first dispatch at compile time.)

Still, in the visitor pattern, changing the hierarchy cause compile time error, since the method is strictly matching the child class, not the base class.
This is good for the programmer and for the project.

10

Sounds like you’ve had some bad experiences with it, but I think it’s silly to blame the tool rather than the workman.

Personally, if I ever had a situation where I found myself casting to different classes that may inherit from each other, I’d immediately stop and redesign it all - that’s just basic common sense. But I strongly feel that the visitor pattern would be overkill for most drag-and-drop operations. You may want to use it if your app is complex and has many different type interactions, but it shouldn’t be compulsory.

11

Well, I kind of disagree with you.
I don’t say the Visitor pattern should be used in all D&D code. For 99% of the code, only a single or two dynamic cast are required so it’s quite safe & easy to maintain.

It’s just that the initial question was about this exact issue of polymorphism & dynamic_cast, and this is where the Visitor pattern should kick in, instead of the dynamic cast chain.

The dynamic cast tool has defects, and it’s important to be aware of them. If the workman had known the defect, it would have saved me a lot of time.
So that’s why I’m explaining the issue, in case TheVinn will be the next workman. And hopefully, when we all know the issue, we won’t overdesign a dynamic_cast chain anymore.

12

It sounds a lot like you are volunteering to submit a patch making the drag and drop interface more robust, using an actual object (versus String) and the visitor pattern.

I don’t care whether it uses the visitor pattern, dynamic_cast, or any other equivalent, as long as it is possible to drop an actual object (and with the relevant object lifetime/deletion/ownership issues addressed).

13

I really need to have the ability to drag and drop an object of class type, and I don’t mind writing it myself if it can be added to Juce. This is what I am going to do. If any of it offends you, or looks lame/retarded, please speak up I will not mind at all, in fact I prefer it so I can get it right.

  • new base class DragAndDropData, derived from ReferenceCountedObject

  • Constructed with an optional description

  • String DragAndDropData::getDescription() function will preserve the old behavior of passing a string

  • Jules and the community can decide how to extend DragAndDropData to support multiple formats and data representations, it’s a separate task and doesn’t affect the architecture. People like me can use the dynamic_cast technique, which doesn’t require any special support from the architecture.

  • Existing functions to support drag and drop will be preserved, like ListBox::getDragSourceDescription() and Component::isInterestedInDragSource()

  • A new set of functions, parallel to each routine that takes a const String& sourceDescription, will be created, taking a ReferenceCountedObjectPtr instead ), whose default implementation is empty

  • Everywhere Juce calls the old functions with the sourceDescription, we will first call the new routine with the DragAndDropData if it exists.

  • Classes that originate drag and drop, like TableListBoxModel with getDragSourceDescription(), will have an additional function added to return a ReferenceCountedObjectPtr, whose default implementation is to return a null pointer.

  • Controls and base classes that originate drag and drop will provide an interface for performing the drag using the new DragAndDropData object first, and if that comes back as null then it will use the existing code paths for compatibility.

14

Oh yeah and obviously DragAndDropContainer will have startDragging() changed to support the DragAndDropData, with the old function signature implemented by just creating an object with the same sourceDescription and calling the new routine.

15

Well I tried to attach a patch with my changes but the board won’t let me…

16

TBH this is the kind of change that I’d rather do myself. In complicated cases, submitted code can be really helpful, but for simple things like this it’d be more hassle for me to download a patch, apply it, look at the changes, check them, tweak them, etc… than it would just to change it myself!

17

Can we agree on the concept then? So I can continue to use my implementation, and then it should be not too difficult to switch.

I’m talking about adding a DragAndDropData class which will be a ReferenceCountedObject, and changing DragAndDropContainer::startDragging() to take a ReferenceCountedObjectPtr.

Routines that take the String will be duplicated (i.e. isInterestedInDragSource, itemDragEnter, etc) and changed to take the DragAndDropData. Classes like ListBoxModel and TableListBoxModel will have functions added. For example:

class TableListBoxModel
{
    //...
    virtual ReferenceCountedObjectPtr<DragAndDropData> getDragAndDropData (const SparseSet<int>& currentlySelectedRows);
    //...
};

This is what I was thinking for DragAndDropData

//==============================================================================
/**
    Used to pass data for drag and drop operations.

    @see DragAndDropContainer
*/
class JUCE_API  DragAndDropData : public ReferenceCountedObject
{
public:
    //==============================================================================
    /** Convenience type
    */
    typedef ReferenceCountedObjectPtr<DragAndDropData> Ptr;

    /** Creates a DragAndDropData.

        The string is passed to legacy functions that don't understand this object.
    */
    DragAndDropData (const String& sourceDescription_=String::empty);

    //==============================================================================
    /** Destructor. */
    virtual ~DragAndDropData();

    //==============================================================================
    /** Returns the legacy source description for this data.
    */
    const String getSourceDescription ();

    //==============================================================================
    /** Adds a copy of the specified object to this drag and drop data.
        ObjectType must be copy-constructible.
    */
    template<class ObjectType>
    void addItem (const ObjectType& what )
    {
      ItemBase* item = new Item<ObjectType> (what);
      items.add (item);
    }

    //==============================================================================
    /** Uses dynamic_cast<> to find an item descriptor matching the
        template type. If no item is found, a null pointer is returned.
        If more than one item of the same type exists, only the first one
        is returned.
    */
    template<class ObjectType>
    const ObjectType* findItem( ObjectType* pResult=0 )
    {
      for( int i=0; i<items.size(); ++i )
      {
        Item<ObjectType>* item=dynamic_cast<Item<ObjectType>*> (items[i]);
        if( item!=0 )
        {
          if (pResult)
            *pResult=item->object;
          return &item->object;
        }
      }
      return 0;
    }

    juce_UseDebuggingNewOperator

private:
    //==============================================================================
    /** Wraps up an object so we can store it in a generic array
        ObjectType must be copy-constructible.
    */
    struct ItemBase
    {
      virtual ~ItemBase() {}
    };
    template<class ObjectType>
    struct Item : ItemBase
    {
      Item( const ObjectType& what ) : object(what) {}
      ObjectType object;
    };

    String sourceDescription;
    Array<ItemBase*> items;
};

Can I assume that when this gets changed, it will resemble this? Or is it going to go in a completely different direction?

18

Example usage for my proposed object:

DragAndDropData::Ptr MyListBoxModel::getDragAndDropData (const SparseSet<int>& currentlySelectedRows)
{
  DragAndDropData::Ptr dragData (new DragAndDropData);

  // arbitrary classes and basic types can be stored in this container
  Image image;
  dragData->addItem (image); // throw an image in there

  MyUserData data;
  dragData->addItem (data); // put some app specific stuff in the drop data

  return dragData;
}

Checking the contents of the data is similarly easy:

bool MyDragDropTarget::isInterestedInDragSource (DragAndDropData::Ptr dragData,
                               Component* sourceComponent)
{
  bool bInterested=false;

  Image image;
  MyUserData userData;
  
  // we prefer to receive an image
  if (dragData->findItem (&image))
    bInterested=true;

  // but we will accept a MyUserData also
  else if (dragData->findItem (&userData))
    bInterested=true;

  return bInterested;
}
19

Woah, slow down there! There’s a bunch of things there that I couldn’t go with. Adding any more methods to DragAndDropTarget would make the class terribly confusing, and if they were similar-but-different versions of the existing ones, that’s just not a clean solution. And your over-engineered object for carrying the values is an interesting idea, and could be a useful container in its own right, but it doesn’t belong in the drag-and-drop classes!

I need to have a think about it all, but my inclination would be to either just make a small change to the signature of itemDropped to add an object, e.g.

itemDropped (const String& sourceDescription, Component* sourceComponent, int x, int y, ReferenceCountedObject* userData)

That approach would only require a very simple change to people’s code to get it to compile.

Or, I could change the signatures of all the DragAndDropTarget methods to pass just a single DragAndDropInfo struct which would contain all the usual x, y, component, etc, plus an optional user-specified object and whatever else. That’d be more hassle for people to update their code, but it’d be future-proof as I could update the struct if I wanted to add features in the future. Don’t know what’s best, I’ll have to ponder…

20

Okay thanks for looking at it. Responding to what you wrote:

  • I see what you mean about making the class confusing. I went for compatibility with existing code.

  • The drag and drop data object could be considered over-engineered I guess, and it could easily be its own separate class.

  • I thought there was utility in being able to easily add different objects to the drag bundle. This way if a Component exports an object of type Image, anyone can easily pick it up with a single function call, and there can be multiple class representations of the drag and drop data. The recipient doesn’t have to know anything other than which object types they can handle.

  • besides itemDropped you still need itemDragEnter, itemDragMove, etc… or else you can’t react appropriately.

  • existing classes like ListBoxModel, TableListBoxModel still need to be changed or else you can’t use the new drag and drop object.

I understand you have other stuff to worry about and that this is low priority thats why I whipped up my own implementation so I can get some code ported in my project. I hope that when you do get around to the drag and drop, you will consider something similar to what I have done.

Thanks!