autostart KRunner with Plasma, aggregate text for KRunner in DesktopView
Changes PlannedPublic

Authored by pdabrowski on Jan 16 2020, 8:52 PM.

Details

Summary

autostart KRunner with Plasma as daemon to minimize query latency

aggregate text for KRunner in DesktopView to prevent eating fast-typed characters

BUG: 416145

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.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
pdabrowski created this revision.Jan 16 2020, 8:52 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 16 2020, 8:52 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pdabrowski requested review of this revision.Jan 16 2020, 8:52 PM

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

pdabrowski edited the summary of this revision. (Show Details)Jan 16 2020, 8:59 PM
pdabrowski added a reviewer: broulik.

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.

shell/main.cpp
231 ↗(On Diff #73737)

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

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?

pdabrowski added inline comments.Jan 16 2020, 9:27 PM
shell/main.cpp
231 ↗(On Diff #73737)

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.

pdabrowski marked 2 inline comments as done.
davidedmundson requested changes to this revision.Jan 17 2020, 12:04 AM

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.

This revision now requires changes to proceed.Jan 17 2020, 12:04 AM
pdabrowski planned changes to this revision.Jan 19 2020, 8:37 PM
pdabrowski added inline comments.
shell/desktopview.cpp
262

TODO: allow spaces as non-first character

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.

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.