From 99e80c1c7e6e77aa26ccbca4fbb0430b35974544 Mon Sep 17 00:00:00 2001 From: Emirald Mateli Date: Sat, 11 Nov 2017 15:05:15 +0100 Subject: [PATCH] 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 --- src/kitemviews/kfileitemmodel.h | 2 +- src/kitemviews/kitemlistcontroller.cpp | 11 +++- src/kitemviews/kitemmodelbase.cpp | 14 +++++ src/kitemviews/kitemmodelbase.h | 14 +++++ src/panels/places/placesitemmodel.cpp | 6 ++ src/panels/places/placesitemmodel.h | 1 + src/tests/CMakeLists.txt | 2 + src/tests/draganddrophelpertest.cpp | 81 ++++++++++++++++++++++++++ src/views/draganddrophelper.cpp | 12 ++++ src/views/draganddrophelper.h | 11 +++- 10 files changed, 150 insertions(+), 4 deletions(-) create mode 100644 src/tests/draganddrophelpertest.cpp diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h index 5dbeb32b2e..0ca748d7ab 100644 --- a/src/kitemviews/kfileitemmodel.h +++ b/src/kitemviews/kfileitemmodel.h @@ -73,7 +73,7 @@ public: * 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 diff --git a/src/kitemviews/kitemlistcontroller.cpp b/src/kitemviews/kitemlistcontroller.cpp index 753d7915d0..008b6c4c6c 100644 --- a/src/kitemviews/kitemlistcontroller.cpp +++ b/src/kitemviews/kitemlistcontroller.cpp @@ -39,6 +39,7 @@ #include #include #include +#include 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); + m_autoActivationTimer->stop(); m_view->setAutoScroll(false); m_view->hideDropIndicator(); @@ -859,8 +861,8 @@ bool KItemListController::dragMoveEvent(QGraphicsSceneDragDropEvent* event, cons return false; } - event->acceptProposedAction(); + QUrl hoveredDir = m_model->directory(); 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(); + + if (m_model->isDir(index)) { + hoveredDir = m_model->url(index); + } + 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(); } + event->setAccepted(!DragAndDropHelper::urlListMatchesUrl(event->mimeData()->urls(), hoveredDir)); + return false; } diff --git a/src/kitemviews/kitemmodelbase.cpp b/src/kitemviews/kitemmodelbase.cpp index ee7e81084a..d73468336b 100644 --- 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); } +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 45ad1f61ab..55078dc248 100644 --- a/src/kitemviews/kitemmodelbase.h +++ b/src/kitemviews/kitemmodelbase.h @@ -182,6 +182,20 @@ public: */ 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 diff --git a/src/panels/places/placesitemmodel.cpp b/src/panels/places/placesitemmodel.cpp index abd6bc925f..680d513b5c 100644 --- 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; } +bool PlacesItemModel::isDir(int index) const +{ + Q_UNUSED(index); + return true; +} + QUrl PlacesItemModel::createSearchUrl(const QUrl& url) { QUrl searchUrl; diff --git a/src/panels/places/placesitemmodel.h b/src/panels/places/placesitemmodel.h index dcc9759e6d..7dd49bf5a7 100644 --- a/src/panels/places/placesitemmodel.h +++ b/src/panels/places/placesitemmodel.h @@ -132,6 +132,7 @@ public: */ void saveBookmarks(); + bool isDir(int index) const Q_DECL_OVERRIDE; 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 1c2335cbfd..13bd963f9f 100644 --- 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(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 index 0000000000..8166b5bf28 --- /dev/null +++ b/src/tests/draganddrophelpertest.cpp @@ -0,0 +1,81 @@ +/*************************************************************************** + * Copyright (C) 2017 by Emirald Mateli * + * * + * 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 +#include + +class DragAndDropHelperTest : public QObject +{ + Q_OBJECT + +private slots: + void testUrlListMatchesUrl_data(); + void testUrlListMatchesUrl(); +}; + +void DragAndDropHelperTest::testUrlListMatchesUrl_data() +{ + QTest::addColumn>("urlList"); + QTest::addColumn("url"); + QTest::addColumn("expected"); + + QTest::newRow("test_equal") + << QList {QUrl::fromLocalFile("/root")} + << QUrl::fromLocalFile("/root") + << true; + + QTest::newRow("test_trailing_slash") + << QList {QUrl::fromLocalFile("/root/")} + << QUrl::fromLocalFile("/root") + << true; + + QTest::newRow("test_ftp_scheme") + << QList {QUrl("ftp://server:2211/dir")} + << QUrl("ftp://server:2211/dir") + << true; + + QTest::newRow("test_not_matched") + << QList {QUrl::fromLocalFile("/usr/share"), QUrl::fromLocalFile("/usr/local/bin")} + << QUrl::fromLocalFile("/usr/bin") + << false; + + QTest::newRow("test_empty_target") + << QList {QUrl::fromLocalFile("/usr/share"), QUrl::fromLocalFile("/usr/local/bin")} + << QUrl() + << false; + + QTest::newRow("test_empty_list") + << QList() + << QUrl::fromLocalFile("/usr/bin") + << false; +} + +void DragAndDropHelperTest::testUrlListMatchesUrl() +{ + QFETCH(QList, 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 01b41f8b21..831a9d43e0 100644 --- a/src/views/draganddrophelper.cpp +++ b/src/views/draganddrophelper.cpp @@ -29,6 +29,14 @@ #include #include + +bool DragAndDropHelper::urlListMatchesUrl(const QList& 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(); @@ -42,6 +50,10 @@ KIO::DropJob* DragAndDropHelper::dropUrls(const QUrl& destUrl, QDropEvent* event 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); diff --git a/src/views/draganddrophelper.h b/src/views/draganddrophelper.h index 3153f06ef0..e47f83ca83 100644 --- a/src/views/draganddrophelper.h +++ b/src/views/draganddrophelper.h @@ -22,9 +22,10 @@ #define DRAGANDDROPHELPER_H #include "dolphin_export.h" +#include +#include -class QUrl; 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. - * @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); + + /** + * @return True if destUrl is contained in the urls parameter. + */ + static bool urlListMatchesUrl(const QList& urls, const QUrl& destUrl); }; #endif