aggregate text for KRunner in DesktopView
ClosedPublic

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

Details

Summary

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

CCBUG: 416145
partial solution: resolved for DesktopView, not resolved for running KRunner from shortcut

Test Plan

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.

pdabrowski updated this revision to Diff 76798.EditedMar 2 2020, 8:12 PM
pdabrowski retitled this revision from autostart KRunner with Plasma, aggregate text for KRunner in DesktopView to aggregate text for KRunner in DesktopView.
pdabrowski edited the summary of this revision. (Show Details)

Dropped autostart feature leaving only text aggregation in DesktopView (modified to allow spaces and national characters).

davidedmundson accepted this revision.Mar 2 2020, 8:13 PM
This revision is now accepted and ready to land.Mar 2 2020, 8:13 PM
pdabrowski marked an inline comment as done.Mar 2 2020, 8:13 PM
pdabrowski added inline comments.Mar 2 2020, 8:15 PM
shell/desktopview.cpp
246

TODO: code style: {

pdabrowski updated this revision to Diff 76799.Mar 2 2020, 8:17 PM
pdabrowski marked an inline comment as done.

Can confirm the fix when running from the desktop, but not when using the shortcut. So I wouldn't call 416145 fully fixed yet. Are you planning to work on that other part in this patch, or in a separate one?

Are you planning to work on that other part in this patch, or in a separate one?

To do what exactly?

Are you planning to work on that other part in this patch, or in a separate one?

To do what exactly?

Find another way to fix the problem of keystrokes getting eaten when activating KRunner using a global shortcut.

Are you planning to work on that other part in this patch, or in a separate one?

I do not plan to change this patch.
As for solving the issue with KRunner invoked from shortcut, I still do not have a good solution for it.

ngraham edited the summary of this revision. (Show Details)Mar 6 2020, 7:53 PM

All right then. I guess it is a tricky problem.

pzro added a subscriber: pzro.EditedMar 27 2020, 5:52 PM

Hey guys, this is what I think

  1. Having the shell capture input and forward it to KRunner when it comes up - because KRunner is too slow to come up and capture text on demand
  2. Having KRunner in the background from the beginning - because KRunner is too slow on demand

I say solution 1 is hackier than 2 and 1 doesn't even solve all the problems (like when invoking with alt+space). I think 2 is better (at least until things change and lazy loading KRunner is no problem). Yes, then there will more memory usage from the start (I see about 36M just checking in KSysGuard) but a worthwhile trade off for a better experience. KRunner is one of the best things in KDE.

pdabrowski edited the test plan for this revision. (Show Details)Jul 10 2020, 7:34 AM