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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
