Adds drag and drop functionality

[deeplow]

Fixes #409

Behaviors

1 valid file	2 valid files

1 invalid file	1 valid file + 1 invalid file

[deeplow]

I was thinking about about how to make the "drag and drop" functionality more discoverable and it hit me that we could just show both the drop area and the add button at the same time. On the draft mockups that we have, both the "drop area" and the button are visible at the same time.

mockup	option 1 (dynamic)	option 2 (static but more discoverable)

So @apyrgio if you agree with this approach, I's squash it. It ended up significantly simplifying the code as well, since it doesn't need any dynamic parts anymore.

[harrislapiroff]

That looks like a good UI solution to me 👍 It's looking good!

[deeplow]

Security note: mime handling

The following line raised security concerns for me:

dangerzone/dangerzone/gui/main_window.py

Line 685 in 7bdb8dc

for url_path in ev.mimeData().urls():

I was afraid that it could be doing some sort of Mime type guessing by going into the suspicious files. So I went down the rabbit hole of checking if this affected us.

TL;DR there is no security concern as far as I can tell. The rest of this post is just the journey.

---

I started by looking up QMimeData to see what it was all about. It seems to be pretty powerful and able to handle a variety of situations for dropping and moving elements in general (text, files, UI elements, etc.). Complexity is the opposite of what we want.

Taking a look at some related source code I see cause for concern in this case switch:

    switch (mode) {
    case QMimeDatabase::MatchDefault:
        break;
    case QMimeDatabase::MatchExtension:
        return mimeTypeForFileExtension(fileName);
    case QMimeDatabase::MatchContent: {
        QFile file(fileName);
        return mimeTypeForData(&file);
    }

In particular the QMimeDatabase::MatchContent case, which implies that it's digging into the file itself. That that's indeed what the function does.

However, what disarmed the situation was me later realizing that the MIME data is for a list of files (text/uri-list) and not any particular file. So it's just a list of URLs for local files...

I got to this conclusion by inspecting the events. Dropping the code here in case it's ever useful or in case anyone wants to confirm this:

diff --git a/dangerzone/gui/main_window.py b/dangerzone/gui/main_window.py
index d9583543..acfa708b 100644
--- a/dangerzone/gui/main_window.py
+++ b/dangerzone/gui/main_window.py
@@ -631,6 +631,25 @@ class DocSelectionWidget(QtWidgets.QWidget):
             pass
 
 
+class DropEventFilter(QtCore.QObject):
+    def __init__(self, target_widget):
+        super().__init__(target_widget)
+        self.target_widget = target_widget
+        self.target_widget.installEventFilter(self)
+
+    def eventFilter(self, target: QtWidgets.QWidget, event: QtCore.QEvent) -> bool:
+        log.debug(f"EVENT FILTERED {event}")
+        if target is self.target_widget and event.type() == QtGui.QDropEvent:
+            dropEvent = QtGui.QDropEvent(event)
+            if dropEvent.key() == Qt.Key_Tab:
+                # Special tab handling
+                return True
+            else:
+                return False
+
+        return False
+
+
 class DocSelectionDropFrame(QtWidgets.QFrame):
     """
     HACK Docs selecting widget "drag-n-drop" border widget
@@ -650,6 +669,7 @@ class DocSelectionDropFrame(QtWidgets.QFrame):
 
         # Drag and drop functionality
         self.setAcceptDrops(True)
+        self.installEventFilter(DropEventFilter(self))
 
         self.document_image_text = QtWidgets.QLabel(
             "Drag and drop\n documents here\n\n or"

The output will look something like this:

[DEBUG] EVENT FILTERED <PySide6.QtGui.QDragMoveEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,99, answerRect=QRect(430,99 1x1), formats=QList("text/uri-list", "text/plain;charset=utf-8"), LeftButton>
[DEBUG] EVENT FILTERED <PySide6.QtGui.QDragMoveEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,99, answerRect=QRect(430,99 1x1), formats=QList("text/uri-list", "text/plain;charset=utf-8"), LeftButton>
[DEBUG] EVENT FILTERED <PySide6.QtGui.QDragMoveEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,98, answerRect=QRect(430,98 1x1), formats=QList("text/uri-list", "text/plain;charset=utf-8"), LeftButton>
[DEBUG] EVENT FILTERED <PySide6.QtGui.QDragMoveEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,97, answerRect=QRect(430,97 1x1), formats=QList("text/uri-list", "text/plain;charset=utf-8"), LeftButton>
[DEBUG] EVENT FILTERED <PySide6.QtGui.QDragMoveEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,96, answerRect=QRect(430,96 1x1), formats=QList("text/uri-list", "text/plain;charset=utf-8"), LeftButton>
[DEBUG] EVENT FILTERED <PySide6.QtGui.QDropEvent(dropAction=CopyAction, proposedAction=CopyAction, possibleActions=CopyAction|MoveAction|LinkAction, posF=430,96, formats=QList("text/uri-list", "text/plain;charset=utf-8"), NoButton>
[INFO] Assigning ID '4pxYgt' to doc '/home/user/dangerzone/tests/test_docs/sample-bmp.bmp'
[DEBUG] Removing all documents
[DEBUG] EVENT FILTERED <PySide6.QtCore.QEvent(QEvent::Hide)>
[DEBUG] EVENT FILTERED <PySide6.QtCore.QEvent(QEvent::HideToParent)>

[Erioldoesdesign]

I was thinking about about how to make the "drag and drop" functionality more discoverable and it hit me that we could just show both the drop area and the add button at the same time. On the draft mockups that we have, both the "drop area" and the button are visible at the same time.

mockup option 1 (dynamic) option 2 (static but more discoverable)

So @apyrgio if you agree with this approach, I's squash it. It ended up significantly simplifying the code as well, since it doesn't need any dynamic parts anymore.

OOoooooooooo COOL!

[deeplow]

I just realized that this last iteration looks very much like the original dangerzone UX improvement suggestions. I had the feeling I had seen this somewhere. Now I know where:

[naglis]

Tested locally, seems to work great.

I was wondering about the "Change Selection" button - IIUC, once a file is selected, the selection can only be changed via the dialog initiated via the "Change Selection" button, i.e. it is not possible to change the selection via drag&drop?

[almet]

That looks very nice, thanks!

I've added a few comments here and there, but really nothing of importance. I checked with @deeplow who is okay we take it over from here.

[almet]

(Just rebased this on latest main)

[almet]

Added some mypy type checking, removed some checks that weren't accurate anymore. All tests are passing except on debian bookworkm

[almet]

I'm not sure if it's related to the CI or with my machine, but I can't reproduce locally the errors on this distribution right now. Retriggering the CI to see if the second take is better.

[almet]

To me, this is ready to be merged.

@apyrgio I'll let you have a look at it when you have some time, and then we can merge 👍

[apyrgio]

Thanks a lot Alexis, I'll let you know once I've taken a look.

[apyrgio]

You're good to go Alexis, thanks a lot. Let's add a QA scenario at some point for this feature.