[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: [PATCH] RunnerManager
From: "Jordi Polo" <mumismo () gmail ! com>
Date: 2008-05-01 8:56:02
Message-ID: a4162420805010156n1b9eef69q63b02386104fa03a () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Done, I've tried to solve the last issues of this mail and already commit
it.
It will be much easier work with it in SVN.
Yes, detach on reset, now I hope it is OK (If I believed in deities will
pray or something)
I also used setIgnoredTypes in some runners, didn't finish with all of them
as I have to go right now. I was very conservative (didn't add it to the
services runner even when I think some IgnoreTypes will apply), I hope not
break things.
2008/5/1 Aaron J. Seigo <aseigo@kde.org>:
> On Tuesday 29 April 2008, Jordi Polo wrote:
> > > ignoredType is wrong. it should be:
> > >
> > > SearchContext::Types ignoredTypes() const
> > >
> > > using the flags (e.g. ignoredTypes & type) would done on the
> consumer's
> > > end
> > > (e.g. in RunnerManager)
> >
> > Done
> >
> > > the Private copy ctor shouldn't copy the matches (it used only on
> detach,
> > > right? so we don't want the matches). this makes the call to
> > > removeAllMatches
> > > unecessary in reset(). that's the whole point of this approach: avoid
> > > copying, deleting, etc.
> >
> > Ok, the ctor should initialize all to empty objects as it is only called
> in
> > reset. I'll change that. See my next commentary.
> >
> > > + if (this !=d->q) {
> > > + //FIXME: this line must be uncommented.
> > > + d.detach();
> > > + }
> > >
> > > that looks completely backwards to me. it also looks completely
> > > unecessary.
> > > whenever reset is called it should detach. in practice, reset will
> only
> > > be called on the real context object, and that one should detach...
> > > again, this
> > > all about preventing copies and deletions.
> >
> > Detaching always is ok.
>
> not sure exactly what you mean by "always"; what i'm suggesting is to
> always
> detach when reset is called because reset is only called from the query
> source (e.g. the krunner UI). never detach otherwise. voila.
>
> > Detaching will create a new empty object, so we
> > don't need the rest of the method when we have indeed detached (we still
> > need to clean our state if we don't detach).
>
> the semantics of "detach()" are actually to create a copy of the
> underlying
> object. detach is COW, where the 'c' stands for copy. it should not create
> an
> empty object. in our case, we only want to create the following items new:
>
> * the mutex for locking (can't be copied)
> * the match list (since in this case, writing applies to the state of the
> SearchContext, making the matches obsolete)
>
> everything else should be copied in detach().
>
> > But detach() will only effectively detach if other object references the
> > data, as detach() returns void, we can not know if in fact detached. I
> am
>
> that's because detach always detaches. there is no "couldn't detach".
>
> > > the rest looks ok ... work needed in places still, but close enough
> ...
> > > i'm
> > > tempted to say "commit it, and then i'll polish it up"
> >
> > Puff, at last we got close, after the detach issue is solved I'll
> comment
> > kDebug() and commit.
>
> cool.
>
> --
> 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
>
> _______________________________________________
> Panel-devel mailing list
> Panel-devel@kde.org
> https://mail.kde.org/mailman/listinfo/panel-devel
>
>
--
Jordi Polo Carres
NLP laboratory - NAIST
http://www.bahasara.org
[Attachment #5 (text/html)]
<br>Done, I've tried to solve the last issues of this mail and already commit it. \
<br>It will be much easier work with it in SVN.<br><br>Yes, detach on reset, now I \
hope it is OK (If I believed in deities will pray or something)<br> I also used \
setIgnoredTypes in some runners, didn't finish with all of them as I have to go \
right now. I was very conservative (didn't add it to the services runner even \
when I think some IgnoreTypes will apply), I hope not break things. <br> \
<br><br><br><br><div class="gmail_quote">2008/5/1 Aaron J. Seigo <<a \
href="mailto:aseigo@kde.org">aseigo@kde.org</a>>:<br><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d">On Tuesday 29 April 2008, Jordi \
Polo wrote:<br> </div><div class="Ih2E3d">> > ignoredType is wrong. it should \
be:<br> > ><br>
> > SearchContext::Types ignoredTypes() const<br>
> ><br>
> > using the flags (e.g. ignoredTypes & type) would done on the \
consumer's<br> > > end<br>
> > (e.g. in RunnerManager)<br>
><br>
> Done<br>
><br>
> > the Private copy ctor shouldn't copy the matches (it used only on \
detach,<br> > > right? so we don't want the matches). this makes the call \
to<br> > > removeAllMatches<br>
> > unecessary in reset(). that's the whole point of this approach: \
avoid<br> > > copying, deleting, etc.<br>
><br>
> Ok, the ctor should initialize all to empty objects as it is only called in<br>
> reset. I'll change that. See my next commentary.<br>
><br>
> > + if (this !=d->q) {<br>
> > + //FIXME: this line must be uncommented.<br>
> > + d.detach();<br>
> > + }<br>
> ><br>
> > that looks completely backwards to me. it also looks completely<br>
> > unecessary.<br>
> > whenever reset is called it should detach. in practice, reset will only<br>
> > be called on the real context object, and that one should detach...<br>
> > again, this<br>
> > all about preventing copies and deletions.<br>
><br>
> Detaching always is ok.<br>
<br>
</div>not sure exactly what you mean by "always"; what i'm suggesting \
is to always<br> detach when reset is called because reset is only called from the \
query<br> source (e.g. the krunner UI). never detach otherwise. voila.<br>
<div class="Ih2E3d"><br>
> Detaching will create a new empty object, so we<br>
> don't need the rest of the method when we have indeed detached (we still<br>
> need to clean our state if we don't detach).<br>
<br>
</div>the semantics of "detach()" are actually to create a copy of the \
underlying<br> object. detach is COW, where the 'c' stands for copy. it \
should not create an<br> empty object. in our case, we only want to create the \
following items new:<br> <br>
* the mutex for locking (can't be copied)<br>
* the match list (since in this case, writing applies to the state of the<br>
SearchContext, making the matches obsolete)<br>
<br>
everything else should be copied in detach().<br>
<div class="Ih2E3d"><br>
> But detach() will only effectively detach if other object references the<br>
> data, as detach() returns void, we can not know if in fact detached. \
I am<br> <br>
</div>that's because detach always detaches. there is no "couldn't \
detach".<br> <div class="Ih2E3d"><br>
> > the rest looks ok ... work needed in places still, but close enough ...<br>
> > i'm<br>
> > tempted to say "commit it, and then i'll polish it up"<br>
><br>
> Puff, at last we got close, after the detach issue is solved I'll \
comment<br> > kDebug() and commit.<br>
<br>
</div>cool.<br>
<div><div></div><div class="Wj3C7c"><br>
--<br>
Aaron J. Seigo<br>
humru othro a kohnu se<br>
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43<br>
<br>
KDE core developer sponsored by Trolltech<br>
</div></div><br>_______________________________________________<br>
Panel-devel mailing list<br>
<a href="mailto:Panel-devel@kde.org">Panel-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/panel-devel" \
target="_blank">https://mail.kde.org/mailman/listinfo/panel-devel</a><br> \
<br></blockquote></div><br><br clear="all"><br>-- <br>Jordi Polo Carres<br>NLP \
laboratory - NAIST<br><a \
href="http://www.bahasara.org">http://www.bahasara.org</a><br>
_______________________________________________
Panel-devel mailing list
Panel-devel@kde.org
https://mail.kde.org/mailman/listinfo/panel-devel
[prev in list] [next in list] [prev in thread] [next in thread]