Wow, it works and it's sooooooooo nice. What an improvement!

Drop options are not needed here; this isn't a move/copy/symlink UI; it's simply a quick way to choose a file that you happen to have visible in Dolphin or on the desktop. On that subject, I notice that drops from the desktop don't work. Not sure if that needs work here or in Folder View.

In D20838#456820, @ngraham wrote:

Wow, it works and it's sooooooooo nice. What an improvement!

:)

Drop options are not needed here; this isn't a move/copy/symlink UI; it's simply a quick way to choose a file that you happen to have visible in Dolphin or on the desktop.
On that subject, I notice that drops from the desktop don't work. Not sure if that needs work here or in Folder View.

I have tested on my side, I don't understand why it does not work.
From dolphin desktop:/ you can drag'n drop fine but not from the folder view.
I receive events and desktop:/ urls but the drag is not accepted whatever I do, like always calling event->accept() in the "case QEvent::DragEnter".
Could it be a because of the folder view filtering where it accepts to get dragged for instance ?
I am a bit stuck here. I am missing something.

In D20838#456831, @meven wrote:

I have tested on my side, I don't understand why it does not work.
From dolphin desktop:/ you can drag'n drop fine but not from the folder view.
I receive events and desktop:/ urls but the drag is not accepted whatever I do, like always calling event->accept() in the "case QEvent::DragEnter".
Could it be a because of the folder view filtering where it accepts to get dragged for instance ?

Sounds like it. If it works from desktop:/ in Dolphin, but not from Folder view, I bet the drag isn't being sent the right way.

However, testing with a file in desktop:/, the drag and drop happens successfully, but the path listed in the filename field is invalid (e.g. desktop:IMG_0713.JPG and the file can't actually be opened:

Looks like the protocol is missing a trailing slash before the file path part. Interestingly, I notice that if I drag the same file from desktop:/<file> the filename field, the path is listed as file:///home/dev/Desktop/IMG_0713.JPG which is different, but also correct. Perhaps the path just needs to be sanitized in the same way when dragged to the file view?

In D20838#456847, @ngraham wrote:

In D20838#456831, @meven wrote:

I have tested on my side, I don't understand why it does not work.
From dolphin desktop:/ you can drag'n drop fine but not from the folder view.
I receive events and desktop:/ urls but the drag is not accepted whatever I do, like always calling event->accept() in the "case QEvent::DragEnter".
Could it be a because of the folder view filtering where it accepts to get dragged for instance ?

Sounds like it. If it works from desktop:/ in Dolphin, but not from Folder view, I bet the drag isn't being sent the right way.

However, testing with a file in desktop:/, the drag and drop happens successfully, but the path listed in the filename field is invalid (e.g. desktop:IMG_0713.JPG and the file can't actually be opened:

Looks like the protocol is missing a trailing slash before the file path part. Interestingly, I notice that if I drag the same file from desktop:/<file> the filename field, the path is listed as file:///home/dev/Desktop/IMG_0713.JPG which is different, but also correct. Perhaps the path just needs to be sanitized in the same way when dragged to the file view?

I have fixed those two issues :

• the path are now translated from kde url to mostlocal urls (dropping from desktop:/ works fine) (not from trash:/ though, but it currently does not work currently in the filename field either)
• the drop action moveAction needed to be allowed in the KDirModel for the drop from the folder view to work.

Todo :

• add an automated test
• check the KDirModel change is sane and does not introduce weird behavior
• To test: what if the filewidget has a mime filter ?
• To test: what if the filewidget is in folder mode ?

Thoughts ?

In D20838#457395, @meven wrote:

I have fixed those two issues :

• the path are now translated from kde url to mostlocal urls (dropping from desktop:/ works fine) (not from trash:/ though, but it currently does not work currently in the filename field either)
• the drop action moveAction needed to be allowed in the KDirModel for the drop from the folder view to work.

Works perfectly!

• To test: what if the filewidget has a mime filter ?

In this case, if the dropped file has a mimetype not permitted by the dialog's filter, it should be marked as an invalid drop.

• To test: what if the filewidget is in folder mode ?

I would say that dropping a file should enter its parent path (and remove the filename part of the path). Dropping a folder should add the folder's path as normal

@ngraham Should I wait for some more review/testing, or LGTM means let's get this merge ASAP ?

Given that nobody else from Frameworks has offered any review comments, I say let's get it in early in this Frameworks cycle (i.e. now-ish) so we have almost a month of testing before release.

The test fails in CI, please check.

https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kio/job/kf5-qt5%20SUSEQt5.12/139/testReport/junit/projectroot/autotests/kiofilewidgets_kfilewidgettest/
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.13/job/kio/job/kf5-qt5%20SUSEQt5.13/7/testReport/projectroot/autotests/kiofilewidgets_kfilewidgettest/

PASS   : KFileWidgetTest::testDropFile(some.txt)
FAIL!  : KFileWidgetTest::testDropFile(subdir/some.txt) Compared values are not the same
  Actual   (fileWidget.locationEdit()->currentText()): ""
  Expected (expectedCurrentText)                     : "some.txt"
  Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.12/autotests/kfilewidgettest.cpp(496)]

In D20838#491992, @dfaure wrote:

The test fails in CI, please check.

https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.12/job/kio/job/kf5-qt5%20SUSEQt5.12/139/testReport/junit/projectroot/autotests/kiofilewidgets_kfilewidgettest/
https://build.kde.org/job/Frameworks/view/Platform%20-%20SUSEQt5.13/job/kio/job/kf5-qt5%20SUSEQt5.13/7/testReport/projectroot/autotests/kiofilewidgets_kfilewidgettest/

PASS   : KFileWidgetTest::testDropFile(some.txt)
FAIL!  : KFileWidgetTest::testDropFile(subdir/some.txt) Compared values are not the same
  Actual   (fileWidget.locationEdit()->currentText()): ""
  Expected (expectedCurrentText)                     : "some.txt"
  Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.12/autotests/kfilewidgettest.cpp(496)]

I can't reproduce this test failure locally.

$ ctest -R kfilewidget
Test project /home/meven/kde/build/kio
    Start 48: kiofilewidgets-kfilewidgettest
1/1 Test #48: kiofilewidgets-kfilewidgettest ...   Passed    9.06 sec

100% tests passed, 0 tests failed out of 1

It may depend on the specific versions used, or environment.

My system uses :
KDE Plasma Version: 5.16.80
KDE Frameworks Version: 5.60.0
Qt Version: 5.12.2
Kernel Version: 5.0.0-20-generic

In any case this needs some investigation, that I will dig into later, I don't have much time right now.
But without reproducing it, it might be challenging.

Strange, I'm pretty sure I was able to reproduce it locally before I wrote my comment here. But now I can't anymore. OK, to be further investigated when either of us have time.

In D20838#492751, @dfaure wrote:

Strange, I'm pretty sure I was able to reproduce it locally before I wrote my comment here. But now I can't anymore. OK, to be further investigated when either of us have time.

I have an hypothesis concerning the issue, and why we have a hard time reproducing it ourselves.

I had to add a wait in the test to make it predictable, which I am not proud of :

line 490 in kfilewidgettest.cpp

// once we drop a file the dirlister scans the dir
// wait a bit to the dirlister time to finish
QTest::qWait(100);

So this may work well on fast systems like my owns with SSD but on CI with less resources this might not be long enough.
Either we can extend a bit this wait and hope for the best or maybe if you see a way to actually get an IO signal when the dirlister is done.

ping @dfaure