[Development] QPushButton: drag and drop

[Volker Hilsheimer]

> On 4 Jun 2022, at 00:08, Giuseppe D'Angelo <giuseppe.dangelo at kdab.com> wrote:
> 
> On 03/06/2022 23:05, Volker Hilsheimer wrote:
>> If we were to deliver a mouseReleaseEvent to the widget that initiated the drag via QDrag::exec after the exec returns, then we handle the mouseReleaseEvent twice.
> 
> Why twice? It receives only one?


The drag’n’drop system handles the mouse release event, and sends a drop-event to the drop target (which might be another process). Qt, in the general case, might never see a mouse release event (it doesn’t on Windows or macOS, for instance).


> Note that the infrastructure for sending a release when the drag ends is already there:
> 
>> https://github.com/qt/qtbase/blob/dev/src/gui/kernel/qsimpledrag.cpp#L125


This is a special class for a special case - intra-application drag’n’drop. We can provide a simplified API like this for that specific case, but I don’t think we should start sending different events depending on the drop target.

On macOS for instance, the performDragOperation of our Dragging interface implementation gets called (in qnsview_dragging.mm) when you drop the drag. If the drop is performed within the Qt application that started the drag, then we can construct a QDropEvent from the information provided, and from the information we record in Qt during the preceding events (key modifiers and buttons etc). But if the drop is performed in another process, then we have no such information.

> It's just that in QtWidgets it never reaches the widget, as it gets filtered out by QApplicationPrivate::pickMouseReceiver (called by QWidgetWindow::handleMouse).
> 
> Given the blame on that code shows no changes since Nokia times, something just tells me that this has never worked properly and people started 1) relying on the release to be never received, and/or 2) add workarounds for that.
> 
> <rant>
> This isn't the only case where Qt event handling is very very inconsistent. Leave/Enter events are also extremely unreliable (if a widget is shown under the cursor, you may or may not get a enter event. Same for a widget moving on its own, moving under or outside the cursor.)
> </rant>

Indeed, we need to heavily rely on the windowing system for enter/leave dispatching to work when windows open. We spent a significant amount of time last year on fixing this on e.g. macOS, where we need to work around inconsistent OS behavior etc. It’s extremely difficult to test reliably in CI as well. I think we fixed all the cases, but if you still experience specific issues here, please open bug reports.

Anyway, off-topic-ish. Drag’n’drop is a special application mode - it’s modal, both in terms of interaction model, and in terms of programming model (QDrag::exec is a blocking call), and the fact that we hand control entirely over to the windowing system limits what we can do.


>> In the case of the button, it would mean that we emit clicked() if the drag is dropped inside the button. Check the following, which simulates that Qt would do that. Drag from the button, and drop the drag inside the button. The button now emits clicked(). You most certainly don’t want slot connected to that signal to execute in that interaction.
> 
> That's kind of an orthogonal story, that is, adding DND support on top of a class that already uses mouse interactions isn't always possible. But you could say the same for any other event-driven interaction when you start messing with the received events. What if tomorrow we add DND support to QPushButton itself, with the related logic in the mouse event handlers? Then the overrides in the testcase will break (they might _re_start DND).


That is rather exactly my point, which I evidently failed to get across. Widgets that implement drag’n’drop handling today will update their states after QDrag::exec returns, usually while they are still in the mouseMoveEvent handler. Take for example QAbstractItemView and subclasses. Or they might do so before calling QDrag::exec (should the button stay pressed while you are dragging from it? only the widget author knows this). They have to do that anyway, because if the drag performed was a move-drag, then they need to remove their data.

If we now start delivering a mouse release event to the source widget when QDrag::exec returns, then we are executing mouseReleaseEvent code with the widget in a state in which it didn’t use to execute this code. Unknown things will happen. In my contrived example, we emit clicked() even though the users didn’t click the button (but instead dragged from it, then changed their mind). Should the mouseReleaseEvent handler be executed while we are still in QDrag::exec (resulting in reentrancy issues)? Or should we post an event and wait for the stack to unroll before running mouseReleaseEvent?

Widgets that implement drag’n’drop today don’t need a mouseReleaseEvent to know that the drag’n’drop operation ended: they know that it did, because QDrag::exec returns.

But if they would have to handle that the mouseReleaseEvent might be called after a QDrag::exec returns, then they need to usually skip over certain aspects of the event handling code, ie not emit clicked() in my contrived example. That means adding more state handling logic, to every widget that implements drag’n’drop correctly today, just so that we can ignore an event that IMHO shouldn’t have been delivered in the first place.


>> This is of course a special case, but it shows that we can’t just start delivering MouseButtonRelease events to widgets when a drag operation finishes, because they would suddenly execute mouseReleaseEvent code in a state in which they don’t expect it to.
> 
> Yes, I'm not claiming that this should be a fix to cherry-pick back. Being likely a 10+ yo behavior, it's dev material only.


I am fairly confident that this has been like this since the Qt 1.x days, and that it was designed to be like that. One can argue that we should have done things differently back then, but here we are. The reason why it is how it is can perhaps be explained with the following equivalent:

We don’t deliver a mouseReleaseEvent to the pressed widget when the pressing of the widget opens a modal dialog. The widget is modally blocked by the dialog, and must not receive any input events during that time. The button will stay pressed, also after you closed the dialog.


    void mousePressEvent(QMouseEvent *e)
    {
        QPushButton::mousePressEvent(e);
        QDialog dialog(this);
        dialog.exec();
    }


If you write a UI like that, it’s a bug in your UI. The fix in the above case is to call setDown(false); after the dialog returned not to deliver a mouseReleaseEvent to the button that never occurred.


QDrag::exec starts a modal interaction with the system, and it is blocking. It’s in practice identical to calling QDialog::exec() - modal, and blocking.


>> For instance, QAbstractItemView does not expect to get a mouseReleaseEvent when QAbstractItemView::startDrag returns. I didn’t test it, but it start the editor of the item that was dragged, which might crash when that index got removed by the drag!
>> Widgets that support drag’n’drop need to reset their state when QDrag::exec returns, and must be able to rely that they don’t get a mouseReleaseEvent when that mouse release already was processed by the drag’n’drop system to end the drag.
> 
> But this simply can't be done reliably in general, as you don't necessarily have access to that state so you can't reset it. Again, what if tomorrow QPushButton starts drawing itself differently depending on mouse movements, and wants to see the release to reset its drawing?

If as the widget author you don’t have access to the states you need, then you have a problem anyway. And yes, not all classes in Qt are designed for you to override them and add arbitrary interaction models on top. QAbstractButton naturally is designed to be overridden, and it gives you access to the states you need, e.g. via setDown in this case. If you want the button to be pressed while the drag is going on, call setDown(false) when QDrag::exec returns; if you want the button to be raised while the drag is going on, call setDown(false) just before you call QDrag::exec. That’s your choice as the widget author, not Qt’s.

With other widgets, it will be harder to add new interaction models without conflicting with the existing ones. Then you will have to decide on the tradeoffs you want, or push patches that make more functionality available through protected members etc.


Volker