Import remote ioslave from plasma-workspace
ClosedPublic

Authored by elvisangelaccio on Feb 20 2017, 5:03 PM.

Details

Summary

As discussed on plasma-devel, the remote:/ ioslave is used also by Dolphin,
so this move will allow to drop plasma-workspace as runtime dependency for Dolphin.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 20 2017, 5:03 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
elvisangelaccio added a comment.EditedFeb 20 2017, 5:09 PM

About the co-installability problem: kio 5.32 will install the ioslave plugin as $QT_PLUGIN_PATH/kf5/kio/remote.so, while plasma-workspace 5.9 installs $QT_PLUGIN_PATH/kio_remote.so. So far so good.
The only conflict is on the kded plugin (remotedirnotify.so), so this would prevent co-installability of kio 5.32 and p-w 5.9. I think the simplest solution is just renaming the kded plugin on the kio side.

Drop Messages.sh

dfaure accepted this revision.Feb 21 2017, 4:10 PM

I know this isn't your code and you're just moving it to solve a dependency issue. No problem, go ahead.

But while at it I took at it, I look at the code and found a few issues, feel free to fix them ;)

Unittests would be good, too .... OK ok I'm pushing it :)

src/ioslaves/remote/remoteimpl.cpp
43

This should probably be QDir().mkpath(path) instead of the whole if exists + cdUp + mkdir dance, which would fail if the parent doesn't exist either (ok, unlikely, but anyway, mkpath is shorter and safer)

97

Wow this is convoluted, a simple QFile::exists(*dirpath + '/' + filename) should be enough rather than a full directory listing.

156

wrong port to QUrl, should be url = QUrl::fromLocalFile(...)

This revision is now accepted and ready to land.Feb 21 2017, 4:10 PM
elvisangelaccio marked 2 inline comments as done.
  • renamed kded module to allow co-installability with plasma-workspace <= 5.9
  • fixed issues spotted by David
In D4690#88306, @dfaure wrote:

I know this isn't your code and you're just moving it to solve a dependency issue. No problem, go ahead.

But while at it I took at it, I look at the code and found a few issues, feel free to fix them ;)

Unittests would be good, too .... OK ok I'm pushing it :)

No problem, thanks for spotting the issues :)

Is it an import with the git history as well?

Is it an import with the git history as well?

Initially I wanted to do that, but then I decided against it because it's not worth the effort imho. There are very few changes and a lot of merge commits (seeing "Merge branch Plasma/5.x" in the kio git log would be weird).
You can see the instructions I'm planning to push in the first commit message (from the "Commits" tab of this review).

Looks good to me, just found two more minor things.

src/ioslaves/remote/remoteimpl.cpp
32

probably unused at this point

88

The QDir object and this check are now redundant. It's all covered by the QFileInfo::exists check below.

  • Removed redundant dir check
  • Removed unused include
This revision was automatically updated to reflect the committed changes.