Add support for portals in KRun
ClosedPublic

Authored by GeeXT on Feb 5 2019, 5:01 PM.

Details

Summary

Currently KRun in Flatpak/Snap doesn't support portals so it can't launch an application
for a given url (because it can't break through sandbox). This patch allows KRun to use
portals to pass the url through sandbox. QDesktopServices::openUrl handles that case and sends
appropriate DBus calls to portals, so URIs will be proccessed accordingly by host system.

FEATURE: 399380

Test Plan

I couldn't build KIO from master in current flatpak KDE runtime (even without the patch), so here are commands to test the patch on top
of KIO 5.54.1:

  1. Install org.kde.Sdk 5.12 from flathub
  2. Install org.kde.konversation from KDE Nightly flatpak repo (https://community.kde.org/Guidelines_and_HOWTOs/Flatpak)
  3. Apply the patch to KIO 5.54.1
  4. run 'flatpak run --filesystem=host --command=sh --devel org.kde.Sdk//5.12' to enter SDK
  5. build KIO
  6. exit SDK (exit or Ctrl+D)
  7. run flatpak shell with Konversation 'flatpak run --filesystem=host --command=sh org.kde.konversation'
  8. run LD_PRELOAD="path/to/libKF5KIOCore.so path/top/libKF5KIOWidgets.so path/to/libKF5KIOFileWidgets.so" /app/bin/konversation

to run it with just compiled KIO (the compiled libraries are located in bin directory inside your build directory)
Now link clicks should pass sandbox and do the appropriate actions (open browser for http links, email application for mailto links,
file manager for file://...)

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.
GeeXT created this revision.Feb 5 2019, 5:01 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 5 2019, 5:01 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
GeeXT requested review of this revision.Feb 5 2019, 5:01 PM
GeeXT updated this revision to Diff 50972.Feb 5 2019, 5:16 PM

Restored the accidentally deleted empty line...

Looks good from the portal point of view, but I would rather wait for someone who knows KRun internally to approve this review.

GeeXT updated this revision to Diff 51017.Feb 6 2019, 6:54 AM

The result should be negated

dfaure added a comment.Feb 6 2019, 8:06 AM

This should only be done if KRun::setEnableExternalBrowser(false) wasn't called, otherwise it will break KParts' BrowserRun subclass and Konqueror's KonqRun (who use KRun to actually find out the mimetype of the file, not to launch another browser).
T
his means adding a bool d->m_externalBrowserEnabled in KRun, since right now we can't know that it was called if the settings don't actually define an external browser.

GeeXT updated this revision to Diff 51047.Feb 6 2019, 6:50 PM

Add check for external browser enablement status

Ping?
Btw, I couldn't find any usages of setEnableExternalBrowser in BrowserRun or KonqRun but there is an usage after instantiating an object of KonqRun (in KonqMainWindow::openUrl). It's strange that a caller must call the function itself or it will crash...

apol accepted this revision.Feb 20 2019, 5:08 PM

Overall, it looks good to me. Let's get this in.

Regarding your process, I think you could have passed --devel on the step 7 and compiled kio there together with konversation. Maybe that makes it a bit easier next time.

src/widgets/krun.cpp
956

Maybe you can swap the condition so the function is only called when m_externalBrowserEnabled is true.

This revision is now accepted and ready to land.Feb 20 2019, 5:08 PM
GeeXT updated this revision to Diff 52163.Feb 20 2019, 5:27 PM

Swap the conditions

GeeXT marked an inline comment as done.Feb 20 2019, 5:28 PM
apol added a comment.Feb 21 2019, 7:58 PM

Thanks!
Can you land the patch or should we do it for you?

GeeXT added a comment.Feb 21 2019, 8:12 PM

@apol I didn't applied for KDE Developer Account yet (I'm new here) so I can't, sorry.

No problem, I'll take care of this for you!

This revision was automatically updated to reflect the committed changes.