Re-implement Meta-E global launch shortcut using KGlobalAccel
ClosedPublic

Authored by ngraham on Apr 3 2019, 2:57 PM.

Details

Summary

FEATURE: 405302
FIXED-IN: 19.08.0

Test Plan
  1. Apply patch
  2. Reboot (didn't work until I rebooted or logged out and back in again)
  3. Hit Meta+E
  4. Dolphin Launches

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Apr 3 2019, 2:57 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptApr 3 2019, 2:57 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham requested review of this revision.Apr 3 2019, 2:57 PM

No technical complaints from me.

ngraham updated this revision to Diff 55350.Apr 3 2019, 3:09 PM

Better match existing CMake style

elvisangelaccio accepted this revision.Apr 6 2019, 5:33 PM

Too late for 19.04, please push to master (after replacing BUG: with FEATURE: in the commit message :D)

This revision is now accepted and ready to land.Apr 6 2019, 5:33 PM
ngraham edited the summary of this revision. (Show Details)Apr 6 2019, 5:40 PM
ngraham edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
asturmlechner added inline comments.
src/CMakeLists.txt
391

Is there a way to do this that does not break a sandboxed (as in: packaging) build? It will also break Windows builds if anyone is doing those.

 * --------------------------- ACCESS VIOLATION SUMMARY ---------------------------
 * LOG FILE: "/var/log/sandbox/sandbox-7242.log"
 * 
VERSION 1.0
FORMAT: F - Function called
FORMAT: S - Access Status
FORMAT: P - Path as passed to function
FORMAT: A - Absolute Path (not canonical)
FORMAT: R - Canonical Path
FORMAT: C - Command Line

F: symlink
S: deny
P: /usr/share/kglobalaccel/org.kde.dolphin.desktop
A: /usr/share/kglobalaccel/org.kde.dolphin.desktop
R: /usr/share/kglobalaccel/org.kde.dolphin.desktop
C: /usr/bin/cmake -E create_symlink /usr/share/applications/org.kde.dolphin.desktop /usr/share/kglobalaccel/org.kde.dolphin.desktop 
 * --------------------------------------------------------------------------------

Hmm, can you help me understand why this change breaks in a sandboxed environment?

asturmlechner added a comment.EditedMay 2 2019, 8:36 AM

Hmm, can you help me understand why this change breaks in a sandboxed environment?

The problem is that cmake symlink command tries to execute on the root filesystem.

So during first build, ${IMAGEDIR}/usr/share/kglobalaccel is created, the symlink command simply fails non-fatally, the empty directory is then installed to the root file system.

-- Installing: /var/tmp/portage/kde-apps/dolphin-9999/image/usr/share/kglobalaccel
failed to create symbolic link '/usr/share/kglobalaccel/org.kde.dolphin.desktop': No such file or directory

Then on second build, the directory already exists on the root file system so that the symlink command fails with the aforementioned sandbox error.

This fix just got sent my way:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 63de0932..e8a7c807 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -387,7 +387,7 @@ install( PROGRAMS org.kde.dolphin.desktop DESTINATION ${KDE_INSTALL_APPDIR} )
 install( DIRECTORY DESTINATION "${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel" )
 
 install(
-    CODE "execute_process(COMMAND \"${CMAKE_COMMAND}\" -E create_symlink ${KDE_INSTALL_FULL_APPDIR}/org.kde.dolphin.desktop ${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel/org.kde.dolphin.desktop)"
+    CODE "execute_process(COMMAND \"${CMAKE_COMMAND}\" -E create_symlink \"${KDE_INSTALL_FULL_APPDIR}/org.kde.dolphin.desktop\" \"\$ENV{DESTDIR}${KDE_INSTALL_FULL_DATAROOTDIR}/kglobalaccel/org.kde.dolphin.desktop\")"
 )
 
 install( FILES settings/dolphin_directoryviewpropertysettings.kcfg

Makes perfect sense. Wanna submit a patch?

@asturmlechner Is Spectacle also affected then? This cmake code was copied from there.

@asturmlechner Is Spectacle also affected then? This cmake code was copied from there.

I can't find such a code snippet there?

@asturmlechner Is Spectacle also affected then? This cmake code was copied from there.

I can't find such a code snippet there?

Ah right, it's not landed yet: D19310

Thanks for that!