[PATCH] RunnerManager

[Aaron J. Seigo]

On Monday 21 April 2008, Jordi Polo wrote:
> These are some basic changes for krunner.

RunnerManager API review...

General:

* missing API dox. this must be added before committing.
* add a second ctor: RunnerManager(const KConfigGroup& config, QObject *parent 
= 0);

+    public:
+    
+        RunnerManager(QObject *parent);

* mark as explicit
* parent = 0 default param

+        ~RunnerManager();
+        void config();

* this sounds like a getter. if this is meant to show the configuration, 
perhaps follow the pattern we're using elsewhere:

void createConfiguration(QWidget *parent);

+        void loadConfig();

+        /**
+         *　Finds and returns a loaded runner or NULL
+         * @arg name the name of the runner
+         * @return Pointer to the runner 
+         */
+        AbstractRunner* getRunner(const QString& name);

* runner(const QString &name) (no "get", proper placement of the & while we're 
at it)
* should be const

+        /**
+         * Retrieves the current context
+         * @return pointer to the current context
+         */
+        SearchContext* globalContext();

* is there a local context? if not, we can probably ditch "global" and make it 
searchContext() instead.
* should be const

+        /**
+         * set the update delay of the matchesUpdated signal, default 
100msecs
+         * @arg new delay in msecs
+         */
+        void setUpdate(int msecs);

* setUpdateInterval, matching DataEngine
* missing the symmetrical getter: int updateInterval() const;

+        /**
+         * Retrieves all available runners for a given term.
+         * @return list of runners
+         */
+        AbstractRunner::List availableRunners(const QString & term);

* missing apidox for term parameter; what is "term" supposed to be exactly?
* availableRunners -> runners
* should be const
* term should have a default parameter of QString()
* why is this even needed at all? shouldn't RunnerManager handle the runners 
compeltely internally, taking queries and returning matches?

+        /**
+         * Retrieves all available matches found so far for the previously 
launched query 
+         * @return List of matches
+         */
+        QList<SearchMatch *> matches();

* should be const
* equivalent to context()->matches()? if so, why bother having it here at all?

implementation questions:

* what is priorityList for exactly? the nested for loops could be disasterous 
for performance. shouldn't this happen as part of the sorting rather than 
which order they appear in the list?

* it seems that you return d->context->matches() anyways, so this is all an 
exercise in futility.

my recommendation: drop this method completely, rely on sorting to get 
priorities right (should be a lot faster, and allows it to be factored into 
the rest of the sorting) and have consumers of this class simply use 
context()->matches()

+        /**
+         * Execute a given match
+         * @arg pointer to the match to be executed
+         */
+         void exec(const SearchMatch *match);

* should be const

+    public Q_SLOTS:
+
+        /**
+         * Launch a query, this will create threads and return inmediately. 
+         * When the information will be available can be known using the 
+         * matchesChanged, matchesUpdated, matchesCompleted signals
+         * The information can be retrieved calling matches 
+         * @arg term the term we want to find matches for
+         * @arg runner optional, if only one specific runner is to be used 
+         * @return 0 if nothing was launched
+         */
+        int launchQuery(const QString & term, const QString & 
runner=QString());

* a return on an async method such as this probably doesn't make much sense
* split into two methods: 
	void launchQuery(const QString &term, QStringList runners = QStringList())
	bool execQuery(const QString &term, QStringList runners = QStringList())

implementation notes:

* shouldn't an empty term result in calling reset()?

+        /**
+         * Reset the current data and stops the query
+         */
+        void reset();



+    Q_SIGNALS:
+        /**
+         * Three ways to be aware of the matching is provided:
+         * matchesChanged: emited each time a new match is added to the list
+         * matchingUpdated: emited no more than once each updateDelay (100 ms 
is the default) 
+         *                  or when it everything was finished. Recommended 
signal for GUIs.
+         * matchingCompleted: emited when all the matches have been found
+         */
+        void matchesChanged();
+        void matchesUpdated();
+        void matchesCompleted();

* apidox need to be done for each method, otherwise the apidox will not be 
generated poperly
* is there really a need for matchesChanged vs matchesUpdated? an 
updateInterval of zero should be enough of a cue, and only one signal needed

+    private Q_SLOTS:
+
+        void manageMatchesChanged();
+        void updateMatching();
+        void manageFinishedJobs();

* Q_PRIVATE_SLOT

+    private:
+
+        void reorderMatches();

* what is this supposed to do?
* move to private class (well, when implemented; for now, just remove it)

+        /**
+         * Called to load and get a list available of Runners.
+         */
+        AbstractRunner::List load();
+        

* move to Private class

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080421/6045dfda/attachment.pgp