<br>Ok, I'll hopefully address these issues and send a new patch tomorrow, what a pity we are so timezone misaligned. <br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
having to read through a 50k patch with no prior explanations of what it is<br>
doing isn't very nice either ..<br>
</blockquote><div><br>Sorry, knowing the code well made me think other people had super analizing-and-understanding-unknown-code powers<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
so.. let's get this cleaned up and ready to go in. also, remember that you can<br>
always start a branches in branches/ anytime you feel it's worthwhile.<br>
<div class="Ih2E3d"></div></blockquote><div><br>I plan to do it post (beta freezing + bug hunting) <br> <br>
</div><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"><br>
> - Sometimes I hit some dbus out of memory errors -> investigate. I may<br>
> resurrect the queueMatch idea.<br>
<br>
</div>dbus out of memory? hm. that's.. odd. where is dbus being used at all? ah, in<br>
one of the runners you have installed, perhaps? in any case, when you<br>
say "resurrect the queueMatch idea" can you explain what you replaced it<br>
with, etc? i shouldn't have to reverse engineer your work when we're all here<br>
on the same list ;)<br>
</blockquote><div><br>In fact a backtrace that showed kbookmark runner as guilty. <br>But my X session died with the unsaved data. I'll hit it again for sure. <br> <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
other general comments:<br>
<br>
* the copyrights in the file are wrong. you can't copy other people's code<br>
from existing files and not carry the copyright over. so there are a few<br>
copyrights missing there, probably Ryan's and mine for starters.<br>
</blockquote><div><br>Sorry, very unpolite, just used to open file and ... <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
* i see that the call to processEvents is commented out. that's good because<br>
you should NEVER call that unless there are very, very good reasons to.<br>
processEvents is evil, evil, evil and reserved only as an Escape Route From<br>
Some Other Level Of Hell ;)</blockquote><div><br>Yes, I tried it and didn't work exactly well :P<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
* in the copy ctor for Context, the completer object is no longer copied over.<br>
at the very least the completions registered should be copied over.<br>
<br>
* using launchQuery as both async and sync should be avoided IMHO. make two<br>
different methods: one that spawns threads and one that doesn't. call the<br>
latter one execQuery.<br>
<br>
* returning an integer from a method as seen in launchQuery is nasty. return a<br>
boolean or use a proper enumeration if an error code is needed. right now i<br>
only see 0 and 1, so a bool would be appropriate. this is C++ after all.<br>
</blockquote><div><br>Ok to all, will do.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
* RunnerManager::getRunner is O(N). if it gets called often and N grows large<br>
(which we probably hope it does ;), this could be an issue. it's probably<br>
premature to optimize this (by using a name->Runner mapping/hash instead of a<br>
simple list) but it's probably worthwhile to at least put a comment there<br>
noting this problem. at some point we should go count how often this gets<br>
called to find out if isuch an optimization is worthwhile in the Real<br>
World(tm).<br>
</blockquote><div><br>So far only is used to get the session runner. But will do. <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
* in the RunnerManager ctor there is this:<br>
<br>
+ d->runners += load();<br>
<br>
load() should simply clear d->runners and set d->runners itself. this will<br>
open the door to changes in configuration down the road as well (just call<br>
load() again ..)<br>
<br>
* please be careful with the coding style. there are a lot of instances where<br>
whitespace is missing, e.g.:<br>
<br>
+ bool emptyMatches=matches.isEmpty();<br>
<br>
which should be<br>
<br>
+ bool emptyMatches = matches.isEmpty();<br>
<font color="#888888"></font></blockquote><div><br>OK, understood, as stated check your inbox tomorrow.<br><br>BTW, in the days I started with plasma development (3 weeks ago?) someone told me a review board was used to be around ... It is still nonfunctional?<br>
<br>About the configuration, if a KConfigGroup is passed it means that each app that wants to use runners capabilites will have a group in its configuration file for it. Create a GUI to configure it can mean creating a widget that each app should use ... <br>
<br><br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><font color="#888888"><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>
</font><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>