Allow to drop one file or one folder on KDirOperator
ClosedPublic

Authored by meven on Apr 26 2019, 11:19 AM.

Details

Summary

BUG: 45154
FIXED-IN: 5.59

Test Plan
  • Manual testing using kfilewidgettestgui.cpp and kfilewidgettest_saving_gui.cpp
  • added automated test

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D20838
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11572
Build 11590: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven marked an inline comment as done.Apr 26 2019, 12:14 PM
meven added inline comments.Apr 26 2019, 12:59 PM
src/filewidgets/kdiroperator.cpp
420

Not sure I have done this correctly.

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.

src/filewidgets/kdiroperator.cpp
1412

Comment seems unnecessary; the code is clear enough

meven updated this revision to Diff 57056.Apr 26 2019, 4:48 PM

Remove unnecessary comment

meven marked an inline comment as done.Apr 26 2019, 4:49 PM
meven added a comment.Apr 26 2019, 5:01 PM

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.

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?

meven updated this revision to Diff 57098.Apr 27 2019, 1:23 PM

Use mostLocalUrls when resolving the dropped url, bettor organize code

meven updated this revision to Diff 57099.Apr 27 2019, 1:26 PM

Fix dropping directory

meven updated this revision to Diff 57101.Apr 27 2019, 1:46 PM

Have to keep a reference to the connection in the object, or it gets deleted before the closure is run

meven updated this revision to Diff 57124.Apr 28 2019, 10:20 AM

Allow KDirModel to accept more actions when dropping on the model, simplify implementation, translates kde url to mostlocalurls when dropping

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 ?

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

meven updated this revision to Diff 57441.May 3 2019, 8:23 AM

Fix an issue where the closuser would be called for each finished event once set

meven updated this revision to Diff 57442.May 3 2019, 8:30 AM

Grab the focus so that the currentItem becomes the kfilewidget selected item, otherwise the user needs to give the kdiroperator the focus first

meven updated this revision to Diff 57589.May 5 2019, 12:45 PM

Clean up and fix test

meven updated this revision to Diff 57597.May 5 2019, 3:33 PM

Clean up test code

ngraham requested changes to this revision.May 5 2019, 8:10 PM

Still works great, and I have just a few code change requests. Then let's aim to land this early in the 5.59 cycle so it gets lots of testing.

src/filewidgets/kdiroperator.cpp
420

Yep, I'd say so now.

1404

this-> not needed

1418

this-> not needed

1423

this-> not needed

1558

this-> not needed

tests/kfilewidgettest_gui.cpp
36

Unnecessary whitespace change.

This revision now requires changes to proceed.May 5 2019, 8:10 PM
meven updated this revision to Diff 57625.May 6 2019, 1:35 AM

review feedback

meven marked 5 inline comments as done.May 6 2019, 1:36 AM
meven added inline comments.
src/filewidgets/kdiroperator.cpp
420

Any idea where I should add this information ?
An example would suffice.

meven edited the summary of this revision. (Show Details)May 6 2019, 9:14 AM
meven edited the test plan for this revision. (Show Details)
meven planned changes to this revision.May 6 2019, 9:38 AM

An eventFilter must return true when the event has been processed and prevent further event handling.
This prevent the drag filtering to not work (multiple files for instance).
The code does not do that currently, I am on it.

meven updated this revision to Diff 57636.May 6 2019, 10:32 AM
meven edited the test plan for this revision. (Show Details)

Fix drag^Ciltering, add mime filtering to the drag filtering

ngraham accepted this revision.May 6 2019, 3:59 PM

LGTM!

src/filewidgets/kdiroperator.cpp
420

You already did, in the inline function documentation in src/filewidgets/kdiroperator.h

This revision is now accepted and ready to land.May 6 2019, 3:59 PM
ngraham edited the summary of this revision. (Show Details)May 6 2019, 3:59 PM
meven updated this revision to Diff 57707.May 7 2019, 10:23 AM

update since comment kio version

meven added a comment.May 8 2019, 1:41 PM

@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.

elvisangelaccio added inline comments.
autotests/kfilewidgettest.cpp
38

Unused?

407–413

Please use descriptive variable names instead of f and fw

415

Missing camelCase

src/filewidgets/kdiroperator.cpp
1404–1405

Please use descriptive variable names also here.

1407

QRegExp should be avoid in new code, can you try to use QRegularExpression instead?

meven updated this revision to Diff 58000.May 13 2019, 12:23 PM

Review, removed a dead variable in test, use QRegularExpression

meven marked 8 inline comments as done.May 13 2019, 12:24 PM
ngraham accepted this revision.May 17 2019, 1:56 PM

I think this can land.

This revision was automatically updated to reflect the committed changes.
meven marked an inline comment as done.
dfaure added a subscriber: dfaure.EditedJul 7 2019, 9:11 PM

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)]
meven added a comment.Jul 9 2019, 11:34 AM

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.

meven added a comment.Jul 17 2019, 9:35 AM

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.