<div class="gmail_quote">2009/2/3 Kevin Ottens <span dir="ltr"><<a href="mailto:ervin@kde.org">ervin@kde.org</a>></span><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 3 February 2009 16:57:19 Alessandro Diaferia wrote:<br>
> I've just moved my DeviceNotifier refactoring to kdereview. I feel it is<br>
> much nicer but since i removed tons of code i'd like you to review it and<br>
> tell me if something is wrong.<br>
<br>
</div>Warning: I took only a very quick glimpse at it (didn't evne attempt to build<br>
it, it's really reading code), so it's only shallow reviewing at this point.<br>
:-)<br>
<br>
In my opinion on the code side it looks OK. A couple of warnings though:<br>
1) I've found a few tabs/space mix. You probably want to remove that and use<br>
spaces everywhere.<br>
2) Internally the terminology used is very mount/umount/eject centric. And<br>
actually that confused me at first, you've to keep in mind that this applet is<br>
doomed to show devices which aren't storage devices. So for future proofing<br>
the reading I think you should stick to a more neutral naming, which would<br>
give for instance for DeviceWidget signals something like:<br>
void teardownRequested(const QString &udi);<br>
void activated(const QString &udi);<br>
<br>
Hoping that'll help.<br>
<br>
Regards.<br>
<font color="#888888">--<br>
Kévin 'ervin' Ottens, <a href="http://ervin.ipsquad.net" target="_blank">http://ervin.ipsquad.net</a><br>
"Ni le maître sans disciple, Ni le disciple sans maître,<br>
Ne font reculer l'ignorance."<br>
</font><br>_______________________________________________<br>
Plasma-devel mailing list<br>
<a href="mailto:Plasma-devel@kde.org">Plasma-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/plasma-devel" target="_blank">https://mail.kde.org/mailman/listinfo/plasma-devel</a><br>
<br></blockquote></div><br>Thank you very much Kevin, your suggestion are really useful to me. I'll commit changes asap :)<br>
<br>
P.S. hope you like its new graphical aspect :P<br>
<br>
cheers<br clear="all"><br>-- <br>Alessandro Diaferia<br>