From 6019dc7b271c71eeb1280dc970390035db98e602 Mon Sep 17 00:00:00 2001 From: "Aaron J. Seigo" Date: Fri, 9 May 2008 03:49:11 +0000 Subject: [PATCH] huge patch, but it's needed to avoid crashes and i can't wait on other people's pending work forever. so... QueryMatch by value! shared dptr == no copy of data either! so (nearly) all the speed love of pointers with none of the crash hate. woo! RunnerContext also by value! die pointers die! also get rid of tons of locking that just doesn't make sense anymore; get rid of data priority enum as well since it's completely meaningless. speed, glory, stability! ftw! svn path=/trunk/KDE/kdebase/workspace/libs/plasma/; revision=805661 --- abstractrunner.cpp | 12 ++-- abstractrunner.h | 6 +- querymatch.cpp | 37 ++++++++++-- querymatch.h | 21 ++++++- runnercontext.cpp | 118 +++++++++++-------------------------- runnercontext.h | 25 ++------ runnermanager.cpp | 9 +-- runnermanager.h | 6 +- scripting/runnerscript.cpp | 4 +- scripting/runnerscript.h | 4 +- 10 files changed, 109 insertions(+), 133 deletions(-) diff --git a/abstractrunner.cpp b/abstractrunner.cpp index 2ecefa3ad..ad61abff9 100644 --- a/abstractrunner.cpp +++ b/abstractrunner.cpp @@ -62,7 +62,7 @@ public: script = Plasma::loadScriptEngine(api, runner); if (!script) { - kDebug() << "Could not create a" << api << "ScriptEngine for the" + kDebug() << "Could not create a(n)" << api << "ScriptEngine for the" << runnerDescription.name() << "Runner."; delete package; package = 0; @@ -118,7 +118,7 @@ KConfigGroup AbstractRunner::config() const return KConfigGroup(&runners, group); } -void AbstractRunner::performMatch( Plasma::RunnerContext &globalContext ) +void AbstractRunner::performMatch(Plasma::RunnerContext &globalContext) { static const int reasonableRunTime = 1500; static const int fastEnoughTime = 250; @@ -126,7 +126,7 @@ void AbstractRunner::performMatch( Plasma::RunnerContext &globalContext ) d->runtime.restart(); //TODO :this is a copy ctor RunnerContext localContext(globalContext,0); - match(&localContext); + match(localContext); // automatically rate limit runners that become slooow const int runtime = d->runtime.elapsed(); bool slowed = speed() == SlowSpeed; @@ -212,8 +212,6 @@ void AbstractRunner::setIgnoredTypes(RunnerContext::Types types) d->blackListed = types; } - - KService::List AbstractRunner::serviceQuery(const QString &serviceType, const QString &constraint) const { QMutexLocker lock(&Private::bigLock); @@ -225,14 +223,14 @@ QMutex* AbstractRunner::bigLock() const return &Private::bigLock; } -void AbstractRunner::run(const Plasma::RunnerContext *search, const Plasma::QueryMatch *action) +void AbstractRunner::run(const Plasma::RunnerContext &search, const Plasma::QueryMatch &action) { if (d->script) { return d->script->run(search, action); } } -void AbstractRunner::match(Plasma::RunnerContext *search) +void AbstractRunner::match(Plasma::RunnerContext &search) { if (d->script) { return d->script->match(search); diff --git a/abstractrunner.h b/abstractrunner.h index b2d43a2d2..1f41b10cc 100644 --- a/abstractrunner.h +++ b/abstractrunner.h @@ -89,14 +89,14 @@ class PLASMA_EXPORT AbstractRunner : public QObject * to return from this method right away, nor to create all matches * here. */ - virtual void match(Plasma::RunnerContext *search); + virtual void match(Plasma::RunnerContext &context); /** * Triggers a call to match. * * @arg globalContext the search context used in executing this match. */ - void performMatch(Plasma::RunnerContext &globalContext); + void performMatch(Plasma::RunnerContext &context); /** * If the runner has options that the user can interact with to modify @@ -132,7 +132,7 @@ class PLASMA_EXPORT AbstractRunner : public QObject * Called whenever an exact or possible match associated with this * runner is triggered. */ - virtual void run(const Plasma::RunnerContext *context, const Plasma::QueryMatch *action); + virtual void run(const Plasma::RunnerContext &context, const Plasma::QueryMatch &action); /** * The nominal speed of the runner. diff --git a/querymatch.cpp b/querymatch.cpp index 810dfbc52..f9a3c921f 100644 --- a/querymatch.cpp +++ b/querymatch.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -31,17 +32,32 @@ namespace Plasma { -class QueryMatch::Private +class QueryMatch::Private : public QSharedData { public: Private(AbstractRunner *r) - : runner(r), + : QSharedData(), + runner(r), type(QueryMatch::ExactMatch), enabled(true), relevance(.7) { } + Private(const Private &other) + { + kDebug() << "copy"; + runner = other.runner; + type = other.type; + id = other.id; + text = other.text; + subtext = other.subtext; + icon = other.icon; + data = other.data; + enabled = other.enabled; + relevance = other.relevance; + } + QPointer runner; QueryMatch::Type type; QString id; @@ -61,9 +77,13 @@ QueryMatch::QueryMatch(AbstractRunner *runner) // kDebug() << "new match created"; } +QueryMatch::QueryMatch(const QueryMatch &other) + : d(other.d) +{ +} + QueryMatch::~QueryMatch() { - delete d; } QString QueryMatch::id() const @@ -161,14 +181,19 @@ bool QueryMatch::operator<(const QueryMatch& other) const return d->relevance < other.d->relevance; } -void QueryMatch::run(const RunnerContext *context) const +QueryMatch& QueryMatch::operator=(const QueryMatch &other) { - Q_ASSERT(context); + kDebug(); + d = other.d; + return *this; +} +void QueryMatch::run(const RunnerContext &context) const +{ //kDebug() << "we run the term" << context->query() << "whose type is" << context->mimetype(); if (d->runner) { //TODO: this could be dangerous if the runner is deleted behind our backs. - d->runner->run(context, this); + d->runner->run(context, *this); } } diff --git a/querymatch.h b/querymatch.h index c00ac0e9b..fcc82e3a4 100644 --- a/querymatch.h +++ b/querymatch.h @@ -20,6 +20,8 @@ #ifndef QUERYMATCH_H #define QUERYMATCH_H +#include + #include class QIcon; @@ -43,7 +45,8 @@ class PLASMA_EXPORT QueryMatch /** * The type of match. Value is important here as it is used for sorting */ - enum Type { CompletionMatch = 10 /**< Possible completion for the data of the query */, + enum Type { NoMatch = 0 /**< Null match */, + CompletionMatch = 10 /**< Possible completion for the data of the query */, PossibleMatch = 30 /**< Something that may match the query */, InformationalMatch = 50 /**< A purely informational, non-actionable match, such as the answer to a question or calculation*/, @@ -66,6 +69,12 @@ class PLASMA_EXPORT QueryMatch * @arg runner the runner this match belongs to */ explicit QueryMatch(AbstractRunner *runner); + + /** + * Copy constructor + */ + QueryMatch(const QueryMatch &other); + ~QueryMatch(); /** @@ -116,8 +125,14 @@ class PLASMA_EXPORT QueryMatch bool isEnabled() const; bool operator<(const QueryMatch& other) const; + QueryMatch& operator=(const QueryMatch &other); - void run(const RunnerContext *context) const; + /** + * Requests this match to activae using the given context + * + * @param context the context to use in conjunction with this run + */ + void run(const RunnerContext &context) const; /** * Sets data to be used internally by the associated @@ -148,7 +163,7 @@ class PLASMA_EXPORT QueryMatch private: class Private; - Private * const d; + QSharedDataPointer d; }; } diff --git a/runnercontext.cpp b/runnercontext.cpp index 22c304493..1236851b4 100644 --- a/runnercontext.cpp +++ b/runnercontext.cpp @@ -34,49 +34,37 @@ #include "querymatch.h" -#define LOCK_FOR_READ(context) if (context->d->policy == Shared) { context->d->lock.lockForRead(); } -#define LOCK_FOR_WRITE(context) if (context->d->policy == Shared) { context->d->lock.lockForWrite(); } -#define UNLOCK(context) if (context->d->policy == Shared) { context->d->lock.unlock(); } -/* +//#define LOCK_FOR_READ(context) if (context->d->policy == Shared) { context->d->lock.lockForRead(); } +//#define LOCK_FOR_WRITE(context) if (context->d->policy == Shared) { context->d->lock.lockForWrite(); } +//#define UNLOCK(context) if (context->d->policy == Shared) { context->d->lock.unlock(); } + #define LOCK_FOR_READ(context) context->d->lock.lockForRead(); #define LOCK_FOR_WRITE(context) context->d->lock.lockForWrite(); #define UNLOCK(context) context->d->lock.unlock(); -*/ + namespace Plasma { class RunnerContext::Private : public QSharedData { public: - Private(RunnerContext *context, RunnerContext::DataPolicy p) + Private(RunnerContext *context) : QSharedData(), type(RunnerContext::UnknownType), - q(context), - policy(p) + q(context) { } Private(const RunnerContext::Private& p) : QSharedData(), - term(p.term), - mimeType(p.mimeType), - type(p.type), - q(p.q), - policy(p.policy) + type(RunnerContext::None), + q(p.q) { - //kDebug() << "¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿boo yeah"; + //kDebug() << "¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿¿boo yeah" << type; } ~Private() { - if (policy == Shared) { - lock.lockForWrite(); - } - qDeleteAll(matches); - matches.clear(); - if (policy == Shared) { - lock.unlock(); - } } /** @@ -84,10 +72,10 @@ class RunnerContext::Private : public QSharedData */ void determineType() { - if (policy == Shared) { - lock.lockForWrite(); - } - + // NOTE! this method must NEVER be called from + // code that may be running in multiple threads + // with the same data. + type = UnknownType; QString path = KShell::tildeExpand(term); int space = term.indexOf(' '); @@ -113,30 +101,22 @@ class RunnerContext::Private : public QSharedData mimeType = mimeTypePtr->name(); } } - } else if (term.contains('.')) { - // default to a network location so we can can do things like www.kde.org - type = NetworkLocation; } } - - if (policy == Shared) { - lock.unlock(); - } } QReadWriteLock lock; - QList matches; + QList matches; QString term; QString mimeType; RunnerContext::Type type; RunnerContext * q; - const RunnerContext::DataPolicy policy; }; -RunnerContext::RunnerContext(QObject *parent, DataPolicy policy) +RunnerContext::RunnerContext(QObject *parent) : QObject(parent), - d(new Private(this, policy)) + d(new Private(this)) { } @@ -155,25 +135,22 @@ RunnerContext::~RunnerContext() void RunnerContext::reset() { - // Locks are needed as other contexts can be copied of this one - // We will detach if we are a copy of someone. But we will reset // if we are the 'main' context others copied from. Resetting // one RunnerContext makes all the copies oneobsolete. d.detach(); - LOCK_FOR_WRITE(this) - //kDebug() << "reset searchContext"; - d->type = RunnerContext::UnknownType; - d->term.clear(); - d->mimeType.clear(); - UNLOCK(this); - // 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 - removeAllMatches(); + if (!d->matches.isEmpty()) { + d->matches.clear(); + emit d->q->matchesChanged(); + } + d->term.clear(); + d->mimeType.clear(); + d->type = UnknownType; //kDebug() << "match count" << d->matches.count(); } @@ -185,19 +162,17 @@ void RunnerContext::setQuery(const QString &term) return; } - LOCK_FOR_WRITE(this) d->term = term; - UNLOCK(this); d->determineType(); } QString RunnerContext::query() const { - LOCK_FOR_READ(this) - QString term = d->term; - UNLOCK(this); - return term; + // the query term should never be set after + // a search starts. in fact, reset() ensures this + // and setQuery(QString) calls reset() + return d->term; } RunnerContext::Type RunnerContext::type() const @@ -210,9 +185,11 @@ QString RunnerContext::mimeType() const return d->mimeType; } -bool RunnerContext::addMatches(const QString& term, const QList &matches) +bool RunnerContext::addMatches(const QString& term, const QList &matches) { - if (query() != term || matches.isEmpty()) { + Q_UNUSED(term) + + if (matches.isEmpty()) { return false; } @@ -227,11 +204,9 @@ bool RunnerContext::addMatches(const QString& term, const QList &ma return true; } -bool RunnerContext::addMatch(const QString &term, QueryMatch *match) +bool RunnerContext::addMatch(const QString &term, const QueryMatch &match) { - if (query() != term) { - return false; - } + Q_UNUSED(term) LOCK_FOR_WRITE(this) d->matches << match; @@ -242,35 +217,14 @@ bool RunnerContext::addMatch(const QString &term, QueryMatch *match) return true; } - -QList RunnerContext::matches() const +QList RunnerContext::matches() const { LOCK_FOR_READ(this) - QList matches = d->matches; + QList matches = d->matches; UNLOCK(this); return matches; } -void RunnerContext::removeAllMatches() -{ - LOCK_FOR_WRITE(this) - if (!d->matches.isEmpty()) { - QList matches = d->matches; - d->matches.clear(); - UNLOCK(this); - - // in case someone is still holding on to the Matches - // when we emit the matchesChanged() signal, we don't - // delete the matches until after the signal is handled. - // a bit safer. - emit d->q->matchesChanged(); - - qDeleteAll(matches); - } else { - UNLOCK(this); - } -} - } #include "runnercontext.moc" diff --git a/runnercontext.h b/runnercontext.h index dd3c12140..f9558e797 100644 --- a/runnercontext.h +++ b/runnercontext.h @@ -57,17 +57,10 @@ class PLASMA_EXPORT RunnerContext : public QObject Q_DECLARE_FLAGS(Types, Type) - enum DataPolicy { Shared = 0, - SingleConsumer - }; - - explicit RunnerContext(QObject *parent = 0, DataPolicy policy = Shared); + explicit RunnerContext(QObject *parent = 0); /** - * Constructs a RunnerContext with a DataPolicy of SingleConsumer that - * contains the search metadata (though none of the currently registered - * matches) from the passed in RunnerContext. Primarily useful for creating - * a thread-local copy of a Shared RunnerContext. + * Copy constructor */ explicit RunnerContext(RunnerContext& other, QObject *parent = 0); @@ -114,21 +107,20 @@ class PLASMA_EXPORT RunnerContext : public QObject * * @return true if matches were added, false if matches were e.g. outdated */ - bool addMatches(const QString& term, const QList &matches); + bool addMatches(const QString& term, const QList &matches); /** * Appends a match to the existing list of matches. * The RunnerContext takes over ownership of the match on successful addition. * - * If you are going to be adding multiple matches, it is better to use - * addMatches instead. + * If you are going to be adding multiple matches, use addMatches instead. * * @arg term the search term that this match was generated for * @arg match the match to add * * @return true if the match was added, false otherwise. */ - bool addMatch(const QString &term, QueryMatch *match); + bool addMatch(const QString &term, const QueryMatch &match); /** * Takes the matches from this RunnerContext and copies to them another. @@ -143,12 +135,7 @@ class PLASMA_EXPORT RunnerContext : public QObject /** * Retrieves all available matches for the current search term. */ - QList matches() const; - - /** - * Removes all matches from this RunnerContext. - */ - void removeAllMatches(); + QList matches() const; Q_SIGNALS: void matchesChanged(); diff --git a/runnermanager.cpp b/runnermanager.cpp index 277065e0b..6ce9b2255 100644 --- a/runnermanager.cpp +++ b/runnermanager.cpp @@ -310,13 +310,10 @@ RunnerManager::RunnerManager(KConfigGroup& config, QObject *parent) //ThreadWeaver::setDebugLevel(true, 4); } - RunnerManager::~RunnerManager() { - d->context.removeAllMatches(); } - AbstractRunner* RunnerManager::runner(const QString &name) const { //TODO: using a list of runners, if this gets slow, switch to hash @@ -336,15 +333,15 @@ RunnerContext* RunnerManager::searchContext() const } //Reordering is here so data is not reordered till strictly needed -QList RunnerManager::matches() const +QList RunnerManager::matches() const { return d->context.matches(); } -void RunnerManager::run(const QueryMatch *match) +void RunnerManager::run(const QueryMatch &match) { //TODO: this function is not const as it may be used for learning - match->run(&d->context); + match.run(d->context); } void RunnerManager::launchQuery (const QString & term, const QString & runnerName) diff --git a/runnermanager.h b/runnermanager.h index 6290d3350..e30391072 100644 --- a/runnermanager.h +++ b/runnermanager.h @@ -65,13 +65,13 @@ class PLASMA_EXPORT RunnerManager : public QObject * Retrieves all available matches found so far for the previously launched query * @return List of matches */ - QList matches() const; + QList matches() const; /** * Runs a given match * @arg pointer to the match to be executed */ - void run(const QueryMatch *match); + void run(const QueryMatch &match); public Q_SLOTS: /** @@ -104,7 +104,7 @@ class PLASMA_EXPORT RunnerManager : public QObject /** * Emited each time a new match is added to the list */ - void matchesChanged(const QList &matches); + void matchesChanged(const QList &matches); private: Q_PRIVATE_SLOT(d, void scheduleMatchesChanged()) diff --git a/scripting/runnerscript.cpp b/scripting/runnerscript.cpp index 30cdd3438..ba47ba809 100644 --- a/scripting/runnerscript.cpp +++ b/scripting/runnerscript.cpp @@ -52,12 +52,12 @@ AbstractRunner* RunnerScript::runner() const return d->runner; } -void RunnerScript::match(Plasma::RunnerContext *search) +void RunnerScript::match(Plasma::RunnerContext &search) { Q_UNUSED(search) } -void RunnerScript::run(const Plasma::RunnerContext *search, const Plasma::QueryMatch *action) +void RunnerScript::run(const Plasma::RunnerContext &search, const Plasma::QueryMatch &action) { Q_UNUSED(search) Q_UNUSED(action) diff --git a/scripting/runnerscript.h b/scripting/runnerscript.h index 2cf537865..0949d9de3 100644 --- a/scripting/runnerscript.h +++ b/scripting/runnerscript.h @@ -62,13 +62,13 @@ public: * RunnerContext::addInformationalMatch, RunnerContext::addExactMatch, and * RunnerContext::addPossibleMatch. */ - virtual void match(Plasma::RunnerContext *search); + virtual void match(Plasma::RunnerContext &search); /** * Called whenever an exact or possible match associated with this * runner is triggered. */ - virtual void run(const Plasma::RunnerContext *search, const Plasma::QueryMatch *action); + virtual void run(const Plasma::RunnerContext &search, const Plasma::QueryMatch &action); protected: /**