Issues with setupMatchSession/matchSessionComplete in RunnerManager

[Ryan P. Bitanga]

On Wed, Jul 29, 2009 at 7:17 PM, Martin Gräßlin<kde at martin-graesslin.com> wrote:
> Am Mittwoch 29 Juli 2009 12:53:56 schrieb Ryan P. Bitanga:
>> Another issue is that teardown is currently only emitted when the
>> dialog is hidden. I think the assumption was that there would be a one
>> to one correspondence between emissions of prepare and teardown.
>> However, with the current implementation, prepare will most likely be
>> emitted several more times than teardown. This poses problems for the
>> new window runner which only clears the data after the teardown signal
>> is emitted. It ends up with too much obsolete data.
> As I commented on Aaron's review request: currently teardown signal is never
> fired. And with the prepped bool it should be ensurable that prepare is only
> fired when there has been a teardown before (which is also currently not
> working ;-)). If those two points are fixed there is no problem.
>>
Hmm... The signal is supposed to be fired after the dialog is closed.
If you left the dialog open while testing the signal won't be fired.

And yep, you're right, prepare is only fired once and won't be fired
again until after someone calls matchSessionComplete. I was coming
from the assumption prepare should be launched everytime a query
changes so i missed that initially... Oops. :) So I think the question
now is should there really be a one to one correspondence between the
two? I think it'd be better if there were multiple prepare signals
instead since the slots are supposed to be thread safe and mutex locks
take time, plus things would be slightly faster by avoiding a call to
a slot.

>> We could leave it up to the runner author to make sure that methods
>> connected to the prepare signal clear obsolete data when the query
>> changes.
>>
>> e.g. assuming setupMatch() is connected to prepare and matchComplete()
>> is connected to teardown
>> void Foo::setupMatch()
>> {
>>     QMutexLocker l(&m_mutex);
>>     // If the query changed before teardown was emitted get rid of the old
>> stuff if (data.count()) {
>>         matchComplete();
>>     }
>>     ...
>> }
>>
>> or we could emit teardown prior to launching a new query. Say in
>> RunnerManager::setupMatchSession() add
>> if (d->prepped) {
>>     matchSessionComplete();
>> }
> I think that is not a good idea. For example the windows runner: it caches all
> icons and window information. That cache should be cleared as soon as the
> query session ended.

The question here is _when_ is the query session over? For some
instances (since we're running in multiple threads), the match()
method won't even be executed because a newer query arrived. Ideally,
we should fire teardown once all the jobs for that particular query
are done. But given we can launch a new query anytime, do we fire the
teardown signal immediately before we launch a new query (the second
option) or do we expect runners to realize they can get multiple
prepare signals and work around it (the first option)?

And oh, since my net conked out before I could post my reply in
reviewboard and I was too lazy to type everything all over again, I
know you are a kwin dev and thanks for coverswitch XD

Cheers,
Ryan