D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R308:a07027cd4f22: Dont delay emission of matchesChanged 
indefinitely (authored by fvogt).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59252=60049

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik, ngraham
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D21607: Don't delay emission of matchesChanged indefinitely

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  I tried it and it works fine for me. Note that the AppStream runner's 
behavior has improved as of 0.12.7 since it now correctly returns no results 
instead of all results when it doesn't get a match.

REPOSITORY
  R308 KRunner

BRANCH
  master

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

To: fvogt, #frameworks, broulik, ngraham
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  Before I land this, I'd like if someone other than me tries krunner with this 
patch applied and judges the result with several runners. The difference is 
very noticable with the appstream runner as it does not batch results.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D21607: Don't delay emission of matchesChanged indefinitely

[Nathaniel Graham]

ngraham added a comment.


  I think it's quite fine to have the entries jump around a little bit. To me 
it makes KRunner feel faster to have it start displaying results immediately 
and then refining them milliseconds later. +1

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt updated this revision to Diff 59252.
fvogt added a comment.


  New algorithm with no delay if not necessary.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59203=59252

BRANCH
  master

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474772 , @fvogt wrote:
  
  > So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.
  
  
  And I just now realize that I'm stupid and mixed this up with milou. krunner 
is a framework so there is no stable branch...
  
  I'll update the diff to the [0,250] case.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474771 , @fvogt wrote:
  
  > I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  >
  >   if(lastMatchChangeSignalled.hasExpired(250)) {
  >   matchChangeTimer.stop();
  >   emit q->matchesChanged(context.matches());
  >   } else {
  >   matchChangeTimer.start(250 - lastMatchChangeSignalled.elapsed());
  >   }
  >
  
  
  Just tried this and it's not too bad, but a noticable change in behaviour. As 
results are shown basically immediately once they're available, it's now 
visible how the entries are filled.
  
  So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474763 , @bruns wrote:
  
  > This would emit the signal more often, but wouldn't
  >
  >   if (!matchChangeTimer.isActive())
  > matchChangeTimer.start(100)
  >
  >
  > achieve essentially the same?
  
  
  That would do it even more often.
  
  I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  
if(lastMatchChangeSignalled.hasExpired(100)) {
matchChangeTimer.stop();
emit q->matchesChanged(context.matches());
} else {
matchChangeTimer.start(100 - lastMatchChangeSignalled.expired());
}
  
  What do you think?

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Stefan Brüns]

bruns added a comment.


  This would emit the signal more often, but wouldn't
  
if (!matchChangeTimer.isActive())
  matchChangeTimer.start(100)
  
  achieve essentially the same?

REPOSITORY
  R308 KRunner

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

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Frameworks, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Currently the signal is only emitted if there was no change to the matches in
  the last 100ms. Especially during krunner startup and early result collection,
  this is unlikely to happen though, so make sure that the signal is emitted
  at least once every ~500ms.

TEST PLAN
  Sometimes results only appeared after a noticable delay, now this
  delay is much shorter.

REPOSITORY
  R308 KRunner

BRANCH
  master

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

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns