D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[David Edmundson]

davidedmundson added a comment.


  > So you oppose preloading of KRunner service, but accept it continuing to 
run forever awaiting its next use. I don't find this consistent.
  
  It's a common lazy-load pattern. 
  Though that extension of closing down would also be a direction I would 
happily support.
  
  > This can't work fast.
  
  Any profiling to support that?  I spent a few seconds and can see that we're 
still using PC2, we're loading not two results views which could happen later. 
One of the plasma framesvgs is emitted it's changed on startup, there's a 
blocking gap where nothing is happening...
  
  If this approach was suggested after extensive hours of profiling and 
optimisations, I'd have a different view, but it shouldn't be our first step.

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski added a comment.


  In D26718#595756 , @davidedmundson 
wrote:
  
  > I don't want us to make krunner autostart. It's a hack, not a fix.
  
  
  I'm gonna play devil's advocate here :)
  If having an already running KRunner service is a hack, then why not 
terminate it each time after its job is done - and restart on request each time 
it is needed again?
  Bug #416145 says exactly this: "When krunner is invoked again during the same 
session it will work correctly."
  So you oppose preloading of KRunner service, but accept it continuing to run 
forever awaiting its next use. I don't find this consistent.
  
  IMHO (correct me if I'm wrong!):
  Something that is supposed to act immediately should be preloaded (sure: it 
may be a lighter version).
  Currently to show the KRunner window for the first time:
  
  - a call is made to D-Bus
  - which detects there is no KRunner service running
  - so it executes the KRunner binary
  - which then builds its UI
  - shows itself
  - and only then accepts user input
  
  This can't work fast. Meanwhile all user's keystrokes go to previously active 
window.

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski planned changes to this revision.
pdabrowski added inline comments.

INLINE COMMENTS

> desktopview.cpp:262
>  if (!e->modifiers() || e->modifiers() == Qt::ShiftModifier) {
>  const QString text = e->text().trimmed();
>  if (!text.isEmpty() && text[0].isPrint()) {

TODO: allow spaces as non-first character

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  The continual forwarding of keys makes sense. I'm happy to accept that.
  
  I don't want us to make krunner autostart. It's a hack, not a fix.

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski updated this revision to Diff 73740.

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26718?vs=73737=73740

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

AFFECTED FILES
  krunner/CMakeLists.txt
  krunner/dbus/org.kde.krunner.service.in
  krunner/krunner-daemon.desktop.cmake
  shell/desktopview.cpp
  shell/desktopview.h

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski added inline comments.

INLINE COMMENTS

> broulik wrote in main.cpp:231
> This is not how we want to autostart something :) Instead, you want to place 
> the desktop file in autostart folder again

Right :)
I thought of this so that everytime Plasma starts it ensures KRunner is running 
too.
But I guess this is not really needed - both KRunner and Plasma would have to 
crash for this to be useful.

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski added a comment.


  In D26718#595688 , @broulik wrote:
  
  > Maybe if we autostarted KRunner based on whether it was used previously
  
  
  "Used previously" as in opened at least once?
  I guess this is always true for every user, because KRunner probably was 
opened even accidentally (DesktopView typing, Alt+Space) by everyone.
  
  Used to actually run something at least once?
  That could work. Still if someone cares for KRunner *not* starting up with 
Plasma, they may want better control over it.
  
  So maybe a simple checkbox somewhere in System Settings?

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Kai Uwe Broulik]

broulik added a comment.


  Hmm, I'd appreciate some startup time optimizations for KRunner first.
  Maybe if we autostarted KRunner based on whether it was used previously would 
be a nice trade-off between resources for systems that don't use it and power 
users that always use it and get annoyed by the lag.

INLINE COMMENTS

> main.cpp:231
> +if (krunnerPeer->isValid()) {
> +krunnerPeer->asyncCall(QStringLiteral("Ping"));
> +}

This is not how we want to autostart something :) Instead, you want to place 
the desktop file in autostart folder again

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Nathaniel Graham]

ngraham added a comment.


  IIRC we used to do this, but changed it to run-on-demand to save some memory 
on startup.
  
  Might be worth revisiting that though, since I can confirm the latency that 
leads to the first few characters getting typed into the open app instead of 
KRunner, which is really annoying. We have a recent bug report about this: 
https://bugs.kde.org/show_bug.cgi?id=416145

REPOSITORY
  R120 Plasma Workspace

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

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26718: autostart KRunner with Plasma, aggregate text for KRunner in DesktopView

[Piotr Dabrowski]

pdabrowski created this revision.
pdabrowski added reviewers: Plasma, Plasma: Workspaces, davidedmundson, ngraham.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
pdabrowski requested review of this revision.

REVISION SUMMARY
  autostart KRunner with Plasma as daemon to minimize query latency
  
  aggregate text for KRunner in DesktopView to prevent eating fast-typed 
characters

TEST PLAN
  KRunner should be now running when Plasma starts.
  (when testing from your own KDE install location, watch out for conflicts with
  "/usr/share/dbus-1/services/org.kde.krunner.service" KRunner DBUS service)
  
  When DesktopView is focused, typed characters show KRunner query.
  All fast-typed characters should now appear correctly in KRunner window,
  instead of only first typed character.

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  krunner/dbus/org.kde.krunner.service.in
  shell/desktopview.cpp
  shell/desktopview.h
  shell/main.cpp

To: pdabrowski, #plasma, #plasma_workspaces, davidedmundson, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart