D9037: use desktop file actions instead of global shortcuts

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:cac7af89b7eb: use desktop file actions instead of global 
shortcuts (authored by broulik).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9037?vs=58829=59246

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/update/CMakeLists.txt
  krunner/update/krunnerglobalshortcuts.cpp
  krunner/update/krunnerglobalshortcuts.upd
  krunner/update/krunnerplugins.cpp
  krunner/update/main.cpp
  krunner/view.cpp

To: broulik, #plasma, davidedmundson, mart
Cc: broulik, apol, davidedmundson, plasma-devel, LeGast00n, ericadams, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Kai Uwe Broulik]

broulik updated this revision to Diff 58829.
broulik added a comment.


  - Remove accidental file rename

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9037?vs=58828=58829

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/update/CMakeLists.txt
  krunner/update/krunnerglobalshortcuts.cpp
  krunner/update/krunnerglobalshortcuts.upd
  krunner/update/krunnerplugins.cpp
  krunner/update/main.cpp
  krunner/view.cpp

To: broulik, #plasma, davidedmundson, mart
Cc: broulik, apol, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Kai Uwe Broulik]

broulik updated this revision to Diff 58828.
broulik edited the test plan for this revision.
broulik added a comment.


  - Write migration script that uses kglobalaccel at runtime
  - Adjust desktop context menu to read the new shortcut
  - Remove activation timer as here the window gets focus mostly fine

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9037?vs=32982=58828

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  containmentactions/contextmenu/menu.cpp
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/update/CMakeLists.txt
  krunner/update/krunnerglobalshortcuts.cpp
  krunner/update/krunnerglobalshortcuts.upd
  krunner/view.cpp

To: broulik, #plasma, davidedmundson, mart
Cc: broulik, apol, davidedmundson, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, mart

D9037: use desktop file actions instead of global shortcuts

[David Edmundson]

davidedmundson added a comment.


  I have a plan for a kconf_update v2 which would resolve the problem above 
nicely
  Probably means this would be 5.14.
  
  Otherwise you can copy my 4ae36aee91a23dcb0736418295269da14152 
 in 
powerdevil

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma, davidedmundson
Cc: broulik, apol, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> krunnerglobalshortcuts.cpp:34
>  
> -migrateEnabledPlugins();
> +KSharedConfig::Ptr config = 
> KSharedConfig::openConfig("kglobalshortcutsrc");
> +KConfigGroup runnergroup(config, "krunner");

1. you don't need an exe to do that, this whole thing could have been

File=kglobalshortcutsrc
RemoveGroup=krunner

in the .upd. The syntax allows for quite a lot of manipulation.

2. I did something similar for a powerdevil shortcut migration, it has a huge 
problem.

kconf_update runs, the config updates, kglobalacceld is still running so later 
syncs undoing your script.

Sorry :/

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma, davidedmundson
Cc: broulik, apol, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Marco Martin]

mart updated this revision to Diff 32982.
mart added a comment.


  - Merge branch 'master' into arcpatch-D9037
  - add a config update

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9037?vs=23123=32982

BRANCH
  arcpatch-D9037

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/update/CMakeLists.txt
  krunner/update/krunnerglobalshortcuts.cpp
  krunner/update/krunnerglobalshortcuts.upd
  krunner/update/krunnerplugins.cpp
  krunner/update/main.cpp
  krunner/view.cpp

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Kai Uwe Broulik]

broulik added a comment.


  Did you find the cause for the input lag?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Marco Martin]

mart marked 5 inline comments as done.
mart added a comment.


  delaying the first show seems to fix the focus issue.
  a fixed timer of 100ms is of course not a solution but may give some insight 
in what the problem actually is (a 0 delay timer doesn't work either)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Marco Martin]

mart updated this revision to Diff 23123.
mart added a comment.


  - delay first show

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9037?vs=23095=23123

BRANCH
  phab/krunnerondemand

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/view.cpp

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D9037#173098, @broulik wrote:
  
  > Overall fine with me.
  >
  > What hapens to places like Plasma's context menu that show "Run command 
(Alt+F2)"? What shortcut will they show and do they still work? Same for typing 
on the desktop.
  
  
  they still work, tough typing on desktop may lose some letters.. i wonder if 
would make sense to catch them plasmashell-side then launch krunner with the 
query as parameter...

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Kai Uwe Broulik]

broulik added a comment.


  Overall fine with me.
  
  What hapens to places like Plasma's context menu that show "Run command 
(Alt+F2)"? What shortcut will they show and do they still work? Same for typing 
on the desktop.

INLINE COMMENTS

> main.cpp:60
>  
> +QCommandLineOption clipboardOption(QStringList() << QStringLiteral("c") 
> << QStringLiteral("clipboard"),
> +i18n("Use the clipboard contents as 
> query for KRunner"));

Use initializer list `{QStringLiteral("c"), QStringLiteral("clipboard")}`

> main.cpp:66
> +parser.addOption(daemonOption);
> +parser.addPositionalArgument("query", 
> QCoreApplication::translate("query", "The query to run, only used if -c is 
> not provided"));
>  parser.addHelpOption();

Why no `i18n()` here?

Also, `QStringLiteral("query")`

> main.cpp:110
> +
> +if (parser.isSet(clipboardOption)) {
> +view.displayWithClipboardContents();

This stuffin the lambda is somewhat duplicated, isn't it?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: broulik, apol, davidedmundson, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[Aleix Pol Gonzalez]

apol added a comment.


  Looks cool!

INLINE COMMENTS

> main.cpp:94
>  View view;
> -view.setVisible(false);
> +const QString query = parser.positionalArguments().isEmpty() ? QString() 
> : parser.positionalArguments().first();
> +if (parser.isSet(clipboardOption)) {

You can do `const QString query = parser.positionalArguments().value(0);`

> main.cpp:108
> +parser.parse(arguments);
> +const QString query = parser.positionalArguments().isEmpty() ? 
> QString() : parser.positionalArguments().first();
> +

^ `value(0)`.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: apol, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D9037: use desktop file actions instead of global shortcuts

[David Edmundson]

davidedmundson added a comment.


  Needs the things you pointed out in the comment, but otherwise looks good.
  
  Can you confirm you can set shortcuts in systemsettings correctly?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D9037

To: mart, #plasma
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9037: use desktop file actions instead of global shortcuts

[Marco Martin]

mart created this revision.
mart added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  - Don't autostart KRunner, use actions in the desktop file to
  
  define global shortcuts and actions
  
  - install its desktop file in share/kglobalaccel to make shortcuts default
  - expand commandline options to permit more control via commandline

TEST PLAN
  works, as expected except
  
  - the first time the krunner window doesn't get keyboard focus, probably kwin 
blocking it
  - kconfigupdate stuff still missing

REPOSITORY
  R120 Plasma Workspace

BRANCH
  phab/krunnerondemand

REVISION DETAIL
  https://phabricator.kde.org/D9037

AFFECTED FILES
  krunner/CMakeLists.txt
  krunner/krunner.desktop
  krunner/main.cpp
  krunner/view.cpp

To: mart, #plasma
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart