<br><br><div class="gmail_quote">2009/2/3 Aaron J. Seigo <span dir="ltr"><<a href="mailto:aseigo@kde.org">aseigo@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 03 February 2009, 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>some comments/thoughts on the code (besides that Kevin already noted):<br>
<br>
* there's an m_rootItem in NotifierDialog that isn't used; looks like a hold-<br>
over from the current DeviceNotifier. ditto with the SpecificRoles enum.</blockquote><div><br> yop, i forgot to remove them :P <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>
* NotifierDialog isn't actually a dialog anymore ;) perhaps a better name<br>
would make it clearer as to what it does. DeviceWidgetManager? looking at it a<br>
bit more, i wonder if NotifierDialog is even needed. all the real<br>
functionality is in the Applet class and DeviceWidget; the "in between" class<br>
could probably be removed as a middle-man and Applet could just use<br>
DeviceWidget directly?</blockquote><div><br> i was thinking about the same.. i feel i'll remove the NotifierDialog :)<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>
* if setEjectEmblem is ever called with set == false and m_unmountActions<br>
doesn't contain an entry for that udi, a crash will occur. a Q_ASSERT should<br>
go in there at a minimum, if not a full if (m_unmountActions.contains). right<br>
now that should never be a problem, but it's one of those things that's really<br>
easy to introduce as a problem later on :)</blockquote><div><br> I'm not sure i can imagine a case when set == false and m_unmountActions does not contain the entry.<br> but i think i'll put the Q_ASSERT as you suggested :P<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>
looking at the screenshots i have two little comments:<br>
<br>
* the action seems to be on the "wrong" side of the dialog. reading left-to-<br>
right it should be "noun, verb" as in "the object i want to operate on, the<br>
thing i want to do with it" since before deciding to eject i first need to<br>
find what to eject ...</blockquote><div><br> do you mean i should put the icon action on the right? will do :) <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>
looking at the code it seems that this is using Plasma::IconWidget to paint<br>
it; that icon should actually be in the corner of the icon itself, not the<br>
corner of the widget. so, that's a small bug in Plasma::IconWidget.</blockquote><div><br> i will try to look at in and fix :P <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>
* the 128 px minimum size is probably a bit much. for preferred size it makes<br>
sense, but it should be possible to make them smaller than that.</blockquote><div><br> right, just was waiting for suggestions =)<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>
otherwise, it's looking nice.. :)</blockquote><div><br> thanks =) <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>
<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 Qt Software<br>
<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><br clear="all">Cheers<br>-- <br>Alessandro Diaferia<br>