Commit a1d553a1 authored by Eduardo Cruz's avatar Eduardo Cruz Committed by Alexander Lohnau
Browse files

Fix flickering in Application Launcher for every character typed

This fixes the flickering that happened on the Application Launcher for every character typed.

Before:
![before](/uploads/cfd6dff218256a4de398deb9982e40df/before.gif)
After:
![after](/uploads/34a6aff7cc79dcb51f102c07573353ae/after.gif)

`RunnerManagerPrivate::scheduleMatchesChanged()` method forwards its events only once every 250ms. The issue was that it "wasted" the very first refresh cycle by forwarding the initially empty matches result set. It only forwarded meaningful results after 250ms, that's why the Application Launcher showed an empty list for 250ms for every character typed.

We are now stalling for the duration of the timeout (250ms) before starting to display results for a new query, so we kind of "ignore" the empty/near-empty result set (unless it actually hits the timeout with no more results coming in the meantime). This avoids refreshing the client with the initially empty (or near-empty) result set, so users won't see the empty list if more meaningful results come in the next 250ms (and they usually do come!).

I also saw that when the search job was done, the engine still waited for the running timeout until it displayed the last results it found. I changed that as well, so now we intercept the running timer, cancel it, and refresh immediately as soon as the job is done. So if the total query runs in less than 250ms, it won't even have to wait that: it will refresh only once (as intended), with the full result set, and as fast as possible.

As a result of all this, the application launcher feels much more responsive now, I believe it is a meaningful improvement in user experience.

Also benefits the standalone krunner since it will now respond faster than 250ms for simple quick queries.

BUG: 423161
parent ae4be604
Pipeline #126782 passed with stages
in 1 minute and 7 seconds
......@@ -35,6 +35,11 @@ ecm_add_tests(
LINK_LIBRARIES Qt${QT_MAJOR_VERSION}::Test KF5Runner Qt${QT_MAJOR_VERSION}::Widgets Qt${QT_MAJOR_VERSION}::DBus
)
ecm_add_tests(
runnermanagertest.cpp
LINK_LIBRARIES Qt${QT_MAJOR_VERSION}::Test KF5Runner Qt${QT_MAJOR_VERSION}::Widgets Qt${QT_MAJOR_VERSION}::DBus
)
add_executable(testremoterunner)
qt_add_dbus_adaptor(demoapp_dbus_adaptor_SRCS "../src/data/org.kde.krunner1.xml" testremoterunner.h TestRemoteRunner)
target_sources(testremoterunner PRIVATE testremoterunner.cpp ${demoapp_dbus_adaptor_SRCS})
......@@ -47,3 +52,4 @@ target_link_libraries(testremoterunner
include(../KF5KRunnerMacros.cmake)
configure_krunner_test(dbusrunnertest testremoterunner DESKTOP_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dbusrunnertest.desktop")
configure_krunner_test(runnermanagersinglerunnermodetest testremoterunner DESKTOP_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dbusrunnertest.desktop")
configure_krunner_test(runnermanagertest testremoterunner DESKTOP_FILE "${CMAKE_CURRENT_SOURCE_DIR}/dbusrunnertest.desktop")
/*
SPDX-FileCopyrightText: 2022 Eduardo Cruz <eduardo.cruz@kdemail.net>
SPDX-License-Identifier: LGPL-2.1-or-later
*/
#include "runnermanager.h"
#include "fakerunner.h"
#include <KSharedConfig>
#include <QAction>
#include <QCoreApplication>
#include <QObject>
#include <QProcess>
#include <QStandardPaths>
#include <QTest>
#include "abstractrunnertest.h"
Q_DECLARE_METATYPE(Plasma::QueryMatch)
Q_DECLARE_METATYPE(QList<Plasma::QueryMatch>)
using namespace Plasma;
class RunnerManagerTest : public AbstractRunnerTest
{
Q_OBJECT
private Q_SLOTS:
void loadRunner()
{
startDBusRunnerProcess({QStringLiteral("net.krunnertests.dave")});
qputenv("XDG_DATA_DIRS", QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation).toLocal8Bit());
QCoreApplication::setLibraryPaths(QStringList());
initProperties();
auto md = KPluginMetaData::fromDesktopFile(QFINDTESTDATA("dbusrunnertestmulti.desktop"), {QStringLiteral("plasma-runner.desktop")});
QVERIFY(md.isValid());
manager->loadRunner(md);
}
void cleanup()
{
killRunningDBusProcesses();
}
void init()
{
qRegisterMetaType<QList<Plasma::QueryMatch>>();
}
/**
* This will test the mechanismm that stalls for 250ms before emiting any result in RunnerManager::scheduleMatchesChanged()
* and the mechanism that anticipates the last results emission in RunnerManager::jobDone().
*/
void testScheduleMatchesChanged()
{
loadRunner();
QSignalSpy spyQueryFinished(manager.get(), &Plasma::RunnerManager::queryFinished);
QSignalSpy spyMatchesChanged(manager.get(), &Plasma::RunnerManager::matchesChanged);
QVERIFY(spyQueryFinished.isValid());
QVERIFY(spyMatchesChanged.isValid());
QCOMPARE(spyQueryFinished.count(), 0);
// This will track the total execution time
QElapsedTimer timer;
timer.start();
// This special string will simulate a 300ms delay
manager->launchQuery("fooDelay300");
// We will have one queryFinished emission immediately signaled by RunnerManager::reset()
QCOMPARE(spyQueryFinished.count(), 1);
// However not yet a matcheschanged, it should be stalled for 250ms
QCOMPARE(spyMatchesChanged.count(), 0);
// After 250ms it will emit with empty matches, we wait for that
QVERIFY(spyMatchesChanged.wait(265)); // 265ms as a margin of safety for 250ms
// This should have taken no less than 250ms. It waits for 250s before "giving up" and emitting an empty matches list.
QVERIFY(timer.elapsed() >= 250);
QCOMPARE(spyMatchesChanged.count(), 1);
QCOMPARE(manager->matches().count(), 0); // This is the empty matches "reset" emission, result is not ready yet
QCOMPARE(spyQueryFinished.count(), 1); // Still the same, query is not done
// We programmed it to emit the result after 300ms, so we need to wait 50ms more for the next emission
QVERIFY(spyQueryFinished.wait(65)); // 65ms as a margin of safety for 50ms
// This should have taken at least 300ms total, as we requested via the special query string
QVERIFY(timer.elapsed() >= 300);
// RunnerManager::jobDone() should have anticipated the final emission, so it should not have waited the full 250+250 ms.
QVERIFY(timer.elapsed() <= 330); // This total should be just a tad bigger than 300ms, we put a 10% margin of safety
QCOMPARE(spyMatchesChanged.count(), 2); // We had the second matchesChanged emission, now with the query result
QCOMPARE(manager->matches().count(), 1); // The result is here
QCOMPARE(spyQueryFinished.count(), 2); // Will have emited queryFinished, job is done
// Now we will make sure that RunnerManager::scheduleMatchesChanged() emits matchesChanged instantly
// if we start a query with an empty string. It will never produce results, stalling is meaninless
manager->launchQuery("");
QCOMPARE(spyMatchesChanged.count(), 3); // One more, instantly, without stall
QCOMPARE(manager->matches().count(), 0); // Empty results for empty query string
}
};
QTEST_MAIN(RunnerManagerTest)
#include "runnermanagertest.moc"
......@@ -56,6 +56,19 @@ RemoteMatches TestRemoteRunner::Match(const QString &searchTerm)
icon.fill(Qt::blue);
m.properties[QStringLiteral("icon-data")] = QVariant::fromValue(serializeImage(icon));
ms << m;
} else if (searchTerm.startsWith(QLatin1String("fooDelay"))) {
// This special query string "fooDelayNNNN" allows us to introduce a desired delay
// to simulate a slow query
const int requestedDelayMs = searchTerm.mid(8).toInt();
RemoteMatch m;
m.id = QStringLiteral("id3");
m.text = QStringLiteral("Match 1");
m.iconName = QStringLiteral("icon1");
m.type = Plasma::QueryMatch::ExactMatch;
m.relevance = 0.8;
m.properties[QStringLiteral("actions")] = QStringList(QStringLiteral("action1"));
QThread::msleep(requestedDelayMs);
ms << m;
} else if (searchTerm.contains(QLatin1String("foo"))) {
RemoteMatch m;
......
......@@ -237,12 +237,10 @@ void RunnerContext::reset()
// we still have to remove all the matches, since if the
// ref count was 1 (e.g. only the RunnerContext is using
// the dptr) then we won't get a copy made
if (!d->matches.isEmpty()) {
d->matches.clear();
Q_EMIT matchesChanged();
}
d->matches.clear();
d->term.clear();
Q_EMIT matchesChanged();
d->mimeType.clear();
d->uniqueIds.clear();
d->type = UnknownType;
......@@ -252,7 +250,9 @@ void RunnerContext::reset()
void RunnerContext::setQuery(const QString &term)
{
reset();
if (!this->query().isEmpty()) {
reset();
}
if (term.isEmpty()) {
return;
......
......@@ -67,6 +67,7 @@ public:
: q(parent)
{
matchChangeTimer.setSingleShot(true);
matchChangeTimer.setTimerType(Qt::TimerType::PreciseTimer); // Without this, autotest will fail due to imprecision of this timer
delayTimer.setSingleShot(true);
QObject::connect(&matchChangeTimer, &QTimer::timeout, q, [this]() {
......@@ -88,11 +89,28 @@ public:
void scheduleMatchesChanged()
{
if (lastMatchChangeSignalled.hasExpired(250)) {
// We avoid over-refreshing the client. We only refresh every this much milliseconds
constexpr int refreshPeriod = 250;
// This will tell us if we are reseting the matches to start a new search. RunnerContext::reset() clears its query string for its emission
if (context.query().isEmpty()) {
matchChangeTimer.stop();
// This actually contains the query string for the new search that we're launching (if any):
if (!this->untrimmedTerm.trimmed().isEmpty()) {
// We are starting a new search, we shall stall for some time before deciding to show an empty matches list.
// This stall should be enough for the engine to provide more meaningful result, so we avoid refreshing with
// an empty results list if possible.
matchChangeTimer.start(refreshPeriod);
// We "pretend" that we have refreshed it so the next call will be forced to wait the timeout:
lastMatchChangeSignalled.restart();
} else {
// We have an empty input string, so it's not a real query. We don't expect any results to come, so no need to stall
Q_EMIT q->matchesChanged(context.matches());
}
} else if (lastMatchChangeSignalled.hasExpired(refreshPeriod)) {
matchChangeTimer.stop();
Q_EMIT q->matchesChanged(context.matches());
} else {
matchChangeTimer.start(250 - lastMatchChangeSignalled.elapsed());
matchChangeTimer.start(refreshPeriod - lastMatchChangeSignalled.elapsed());
}
}
......@@ -319,13 +337,17 @@ public:
searchJobs.remove(runJob);
oldSearchJobs.remove(runJob);
if (searchJobs.isEmpty() && context.matches().isEmpty()) {
// we finished our run, and there are no valid matches, and so no
// signal will have been sent out. so we need to emit the signal
// ourselves here
Q_EMIT q->matchesChanged(context.matches());
}
if (searchJobs.isEmpty()) {
// If there are any new matches scheduled to be notified, we should anticipate it and just refresh right now
if (matchChangeTimer.isActive()) {
matchChangeTimer.stop();
Q_EMIT q->matchesChanged(context.matches());
} else if (context.matches().isEmpty()) {
// we finished our run, and there are no valid matches, and so no
// signal will have been sent out. so we need to emit the signal
// ourselves here
Q_EMIT q->matchesChanged(context.matches());
}
Q_EMIT q->queryFinished();
}
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment