Prevent folders from drag and dropping onto themselves in dolphin main view

Summary:
This patch aims to improve user experience by not allowing the user to drag and drop a folder into itself.

The current behavior shows a red message at the top which can then be closed by the user, instead of relying on that, this patch disables the option of dropping onto self and uses the "Invalid drop target cursor" to highlight the behavior.

BUG: 307747

Since spectacle is unable to screenshot the cursor overlay, find attached a photo of the screen.
{F3787651}

Test Plan:
1. Drag a folder.
2. Drop it onto itself.

Reviewers: #dolphin, elvisangelaccio, ngraham, rkflx, dfaure

Reviewed By: #dolphin, elvisangelaccio, rkflx, dfaure

Subscribers: rkflx, ngraham, elvisangelaccio, dfaure, anthonyfieroni, #konqueror

Tags: #dolphin

Differential Revision: https://phabricator.kde.org/D6281

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index 5dbeb32b2e2a7e416f8f143d595fe2264081a3b1..0ca748d7ab8803363ab6e00c583289b7de83b84f 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -73,7 +73,7 @@ public:
      *         the root-parent of all items.
      * @see rootItem()
      */
      *         the root-parent of all items.
      * @see rootItem()
      */

-    QUrl directory() const;
+    QUrl directory() const Q_DECL_OVERRIDE;

 
     /**
      * Cancels the loading of a directory which has been started by either
 
     /**
      * Cancels the loading of a directory which has been started by either

diff --git a/src/kitemviews/kitemlistcontroller.cpp b/src/kitemviews/kitemlistcontroller.cpp
index 753d7915d096a75d3789193c9224f0715deb105e..008b6c4c6cdb21a3a463a1d65709b37cb7214f2a 100644 (file)
--- a/src/kitemviews/kitemlistcontroller.cpp
+++ b/src/kitemviews/kitemlistcontroller.cpp
@@ -39,6 +39,7 @@
 #include <QMimeData>
 #include <QTimer>
 #include <QAccessible>
 #include <QMimeData>
 #include <QTimer>
 #include <QAccessible>

+#include <views/draganddrophelper.h>

 
 KItemListController::KItemListController(KItemModelBase* model, KItemListView* view, QObject* parent) :
     QObject(parent),
 
 KItemListController::KItemListController(KItemModelBase* model, KItemListView* view, QObject* parent) :
     QObject(parent),


@@ -842,6 +843,7 @@ bool KItemListController::dragLeaveEvent(QGraphicsSceneDragDropEvent* event, con
     Q_UNUSED(event);
     Q_UNUSED(transform);
 
     Q_UNUSED(event);
     Q_UNUSED(transform);
 

+    m_autoActivationTimer->stop();

     m_view->setAutoScroll(false);
     m_view->hideDropIndicator();
 
     m_view->setAutoScroll(false);
     m_view->hideDropIndicator();
 


@@ -859,8 +861,8 @@ bool KItemListController::dragMoveEvent(QGraphicsSceneDragDropEvent* event, cons
         return false;
     }
 
         return false;
     }
 

-    event->acceptProposedAction();

 
 

+    QUrl hoveredDir = m_model->directory();

     KItemListWidget* oldHoveredWidget = hoveredWidget();
 
     const QPointF pos = transform.map(event->pos());
     KItemListWidget* oldHoveredWidget = hoveredWidget();
 
     const QPointF pos = transform.map(event->pos());


@@ -883,6 +885,11 @@ bool KItemListController::dragMoveEvent(QGraphicsSceneDragDropEvent* event, cons
         }
 
         const int index = newHoveredWidget->index();
         }
 
         const int index = newHoveredWidget->index();

+
+        if (m_model->isDir(index)) {
+            hoveredDir = m_model->url(index);
+        }
+

         if (!droppingBetweenItems) {
             if (m_model->supportsDropping(index)) {
                 // Something has been dragged on an item.
         if (!droppingBetweenItems) {
             if (m_model->supportsDropping(index)) {
                 // Something has been dragged on an item.


@@ -908,6 +915,8 @@ bool KItemListController::dragMoveEvent(QGraphicsSceneDragDropEvent* event, cons
         m_view->hideDropIndicator();
     }
 
         m_view->hideDropIndicator();
     }
 

+    event->setAccepted(!DragAndDropHelper::urlListMatchesUrl(event->mimeData()->urls(), hoveredDir));
+

     return false;
 }
 
     return false;
 }
 

diff --git a/src/kitemviews/kitemmodelbase.cpp b/src/kitemviews/kitemmodelbase.cpp
index ee7e81084a6b46b5bb11731e4adfb3041854502b..d73468336b6dde6071b578fe703ee78a8a5d15d8 100644 (file)
--- a/src/kitemviews/kitemmodelbase.cpp
+++ b/src/kitemviews/kitemmodelbase.cpp
@@ -164,3 +164,17 @@ void KItemModelBase::onSortOrderChanged(Qt::SortOrder current, Qt::SortOrder pre
     Q_UNUSED(previous);
 }
 
     Q_UNUSED(previous);
 }
 

+QUrl KItemModelBase::url(int index) const
+{
+    return data(index).value("url").toUrl();
+}
+
+bool KItemModelBase::isDir(int index) const
+{
+    return data(index).value("isDir").toBool();
+}
+
+QUrl KItemModelBase::directory() const
+{
+    return QUrl();
+}
\ No newline at end of file

diff --git a/src/kitemviews/kitemmodelbase.h b/src/kitemviews/kitemmodelbase.h
index 45ad1f61ab99737ca2076c6929ec26c5feff5e14..55078dc2486272b2072e6e240c813cf2779188e7 100644 (file)
--- a/src/kitemviews/kitemmodelbase.h
+++ b/src/kitemviews/kitemmodelbase.h
@@ -182,6 +182,20 @@ public:
      */
     QString blacklistItemDropEventMimeType() const;
 
      */
     QString blacklistItemDropEventMimeType() const;
 

+    /**
+     * @return URL of the item at the specified index
+     */
+    virtual QUrl url(int index) const;
+
+    /**
+     * @return True, if item at specified index is a directory
+     */
+    virtual bool isDir(int index) const;
+
+    /**
+     * @return Parent directory of the items that are shown
+     */
+    virtual QUrl directory() const;

 signals:
     /**
      * Is emitted if one or more items have been inserted. Each item-range consists
 signals:
     /**
      * Is emitted if one or more items have been inserted. Each item-range consists

diff --git a/src/panels/places/placesitemmodel.cpp b/src/panels/places/placesitemmodel.cpp
index abd6bc925f5397ebecf73d30a6d5bda6b9e480a6..680d513b5c166dcd214984eaa3d9b5ac44da58d8 100644 (file)
--- a/src/panels/places/placesitemmodel.cpp
+++ b/src/panels/places/placesitemmodel.cpp
@@ -1159,6 +1159,12 @@ QString PlacesItemModel::timelineDateString(int year, int month, int day)
     return date;
 }
 
     return date;
 }
 

+bool PlacesItemModel::isDir(int index) const
+{
+    Q_UNUSED(index);
+    return true;
+}
+

 QUrl PlacesItemModel::createSearchUrl(const QUrl& url)
 {
     QUrl searchUrl;
 QUrl PlacesItemModel::createSearchUrl(const QUrl& url)
 {
     QUrl searchUrl;

diff --git a/src/panels/places/placesitemmodel.h b/src/panels/places/placesitemmodel.h
index dcc9759e6d5192cda427fb5b941834d309ad5196..7dd49bf5a7b17f0c92bd867ea76758af9eaa6bb9 100644 (file)
--- a/src/panels/places/placesitemmodel.h
+++ b/src/panels/places/placesitemmodel.h
@@ -132,6 +132,7 @@ public:
      */
     void saveBookmarks();
 
      */
     void saveBookmarks();
 

+    bool isDir(int index) const Q_DECL_OVERRIDE;

 signals:
     void errorMessage(const QString& message);
     void storageSetupDone(int index, bool success);
 signals:
     void errorMessage(const QString& message);
     void storageSetupDone(int index, bool success);

diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt
index 1c2335cbfde2dc59a9785ab40a133351c098bb02..13bd963f9f04c0ac343bed13dcf9204490ebc794 100644 (file)
--- a/src/tests/CMakeLists.txt
+++ b/src/tests/CMakeLists.txt
@@ -57,3 +57,5 @@ LINK_LIBRARIES dolphinprivate dolphinstatic Qt5::Test)
 ecm_add_test(dolphinmainwindowtest.cpp
 TEST_NAME dolphinmainwindowtest
 LINK_LIBRARIES dolphinprivate dolphinstatic Qt5::Test)
 ecm_add_test(dolphinmainwindowtest.cpp
 TEST_NAME dolphinmainwindowtest
 LINK_LIBRARIES dolphinprivate dolphinstatic Qt5::Test)

+
+ecm_add_test(draganddrophelpertest.cpp LINK_LIBRARIES dolphinprivate Qt5::Test)
\ No newline at end of file

diff --git a/src/tests/draganddrophelpertest.cpp b/src/tests/draganddrophelpertest.cpp
new file mode 100644 (file)
index 0000000..8166b5b
--- /dev/null
+++ b/src/tests/draganddrophelpertest.cpp
@@ -0,0 +1,81 @@
+/***************************************************************************
+ *   Copyright (C) 2017 by Emirald Mateli <aldo.mateli@gmail.com>          *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA            *
+ ***************************************************************************/
+
+#include <QTest>
+#include <views/draganddrophelper.h>
+
+class DragAndDropHelperTest : public QObject
+{
+    Q_OBJECT
+
+private slots:
+    void testUrlListMatchesUrl_data();
+    void testUrlListMatchesUrl();
+};
+
+void DragAndDropHelperTest::testUrlListMatchesUrl_data()
+{
+    QTest::addColumn<QList<QUrl>>("urlList");
+    QTest::addColumn<QUrl>("url");
+    QTest::addColumn<bool>("expected");
+
+    QTest::newRow("test_equal")
+            << QList<QUrl> {QUrl::fromLocalFile("/root")}
+            << QUrl::fromLocalFile("/root")
+            << true;
+
+    QTest::newRow("test_trailing_slash")
+            << QList<QUrl> {QUrl::fromLocalFile("/root/")}
+            << QUrl::fromLocalFile("/root")
+            << true;
+
+    QTest::newRow("test_ftp_scheme")
+            << QList<QUrl> {QUrl("ftp://server:2211/dir")}
+            << QUrl("ftp://server:2211/dir")
+            << true;
+
+    QTest::newRow("test_not_matched")
+            << QList<QUrl> {QUrl::fromLocalFile("/usr/share"), QUrl::fromLocalFile("/usr/local/bin")}
+            << QUrl::fromLocalFile("/usr/bin")
+            << false;
+
+    QTest::newRow("test_empty_target")
+            << QList<QUrl> {QUrl::fromLocalFile("/usr/share"), QUrl::fromLocalFile("/usr/local/bin")}
+            << QUrl()
+            << false;
+
+    QTest::newRow("test_empty_list")
+            << QList<QUrl>()
+            << QUrl::fromLocalFile("/usr/bin")
+            << false;
+}
+
+void DragAndDropHelperTest::testUrlListMatchesUrl()
+{
+    QFETCH(QList<QUrl>, urlList);
+    QFETCH(QUrl, url);
+    QFETCH(bool, expected);
+
+    QCOMPARE(DragAndDropHelper::urlListMatchesUrl(urlList, url), expected);
+}
+
+
+QTEST_MAIN(DragAndDropHelperTest)
+
+#include "draganddrophelpertest.moc"

diff --git a/src/views/draganddrophelper.cpp b/src/views/draganddrophelper.cpp
index 01b41f8b21c23b861a39f0e7b4a3cd1e9f5e04ff..831a9d43e08be707514235df07fb87435b80cf4c 100644 (file)
--- a/src/views/draganddrophelper.cpp
+++ b/src/views/draganddrophelper.cpp
@@ -29,6 +29,14 @@
 #include <KIO/DropJob>
 #include <KJobWidgets>
 
 #include <KIO/DropJob>
 #include <KJobWidgets>
 

+
+bool DragAndDropHelper::urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl)
+{
+    return std::find_if(urls.constBegin(), urls.constEnd(), [destUrl](const QUrl& url) {
+        return url.matches(destUrl, QUrl::StripTrailingSlash);
+    }) != urls.constEnd();
+}
+

 KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window)
 {
     const QMimeData* mimeData = event->mimeData();
 KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event, QWidget* window)
 {
     const QMimeData* mimeData = event->mimeData();


@@ -42,6 +50,10 @@ KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event
         message.setArguments({destUrl.toDisplayString(QUrl::PreferLocalFile)});
         QDBusConnection::sessionBus().call(message);
     } else {
         message.setArguments({destUrl.toDisplayString(QUrl::PreferLocalFile)});
         QDBusConnection::sessionBus().call(message);
     } else {

+        if (urlListMatchesUrl(event->mimeData()->urls(), destUrl)) {
+            return nullptr;
+        }
+

         // Drop into a directory or a desktop-file
         KIO::DropJob *job = KIO::drop(event, destUrl);
         KJobWidgets::setWindow(job, window);
         // Drop into a directory or a desktop-file
         KIO::DropJob *job = KIO::drop(event, destUrl);
         KJobWidgets::setWindow(job, window);

diff --git a/src/views/draganddrophelper.h b/src/views/draganddrophelper.h
index 3153f06ef0c879faef5b03878e295a63f2baa6bd..e47f83ca83ce4aa309bd2c3200ce74d8239ab975 100644 (file)
--- a/src/views/draganddrophelper.h
+++ b/src/views/draganddrophelper.h
@@ -22,9 +22,10 @@
 #define DRAGANDDROPHELPER_H
 
 #include "dolphin_export.h"
 #define DRAGANDDROPHELPER_H
 
 #include "dolphin_export.h"

+#include <QList>
+#include <QUrl>

 
 
 
 

-class QUrl;

 class QDropEvent;
 class QWidget;
 namespace KIO { class DropJob; }
 class QDropEvent;
 class QWidget;
 namespace KIO { class DropJob; }


@@ -42,11 +43,17 @@ public:
      *                  is true.
      * @param event     Drop event.
      * @param window    Widget where the drop happened, will be used as parent of the drop menu.
      *                  is true.
      * @param event     Drop event.
      * @param window    Widget where the drop happened, will be used as parent of the drop menu.

-     * @return          KIO::DropJob pointer
+     * @return          KIO::DropJob pointer or null in case the destUrl is contained
+     *                  in the mimeData url list.

      */
     static KIO::DropJob* dropUrls(const QUrl& destUrl,
                                   QDropEvent* event,
                                   QWidget *window);
      */
     static KIO::DropJob* dropUrls(const QUrl& destUrl,
                                   QDropEvent* event,
                                   QWidget *window);

+
+    /**
+     * @return True if destUrl is contained in the urls parameter.
+     */
+    static bool urlListMatchesUrl(const QList<QUrl>& urls, const QUrl& destUrl);

 };
 
 #endif
 };
 
 #endif