[KDEREVIEW] DeviceNotifier refactor

[Aaron J. Seigo]

On Tuesday 03 February 2009, Alessandro Diaferia wrote:
> I've just moved my DeviceNotifier refactoring to kdereview. I feel it is
> much nicer but since i removed tons of code i'd like you to review it and
> tell me if something is wrong.

some comments/thoughts on the code (besides that Kevin already noted):

* there's an m_rootItem in NotifierDialog that isn't used; looks like a hold-
over from the current DeviceNotifier. ditto with the SpecificRoles enum.

* NotifierDialog isn't actually a dialog anymore ;) perhaps a better name 
would make it clearer as to what it does. DeviceWidgetManager? looking at it a 
bit more, i wonder if NotifierDialog is even needed. all the real 
functionality is in the Applet class and DeviceWidget; the "in between" class 
could probably be removed as a middle-man and Applet could just use 
DeviceWidget directly?

* if setEjectEmblem is ever called with set == false and m_unmountActions 
doesn't contain an entry for that udi, a crash will occur. a Q_ASSERT should 
go in there at a minimum, if not a full if (m_unmountActions.contains). right 
now that should never be a problem, but it's one of those things that's really 
easy to introduce as a problem later on :)

looking at the screenshots i have two little comments:

* the action seems to be on the "wrong" side of the dialog. reading left-to-
right it should be "noun, verb" as in "the object i want to operate on, the 
thing i want to do with it" since before deciding to eject i first need to 
find what to eject ...

looking at the code it seems that this is using Plasma::IconWidget to paint 
it; that icon should actually be in the corner of the icon itself, not the 
corner of the widget. so, that's a small bug in Plasma::IconWidget.

* the 128 px minimum size is probably a bit much. for preferred size it makes 
sense, but it should be possible to make them smaller than that.

otherwise, it's looking nice.. :)

-- 
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 Qt Software

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20090203/d33f7233/attachment.sig