2009/2/3 Aaron J. Seigo <ase...@kde.org> > 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.
yop, i forgot to remove them :P > > > * 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? i was thinking about the same.. i feel i'll remove the NotifierDialog :) > > > * 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 :) I'm not sure i can imagine a case when set == false and m_unmountActions does not contain the entry. but i think i'll put the Q_ASSERT as you suggested :P > > > 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 ... do you mean i should put the icon action on the right? will do :) > > > 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. i will try to look at in and fix :P > > > * 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. right, just was waiting for suggestions =) > > > otherwise, it's looking nice.. :) thanks =) > > > -- > 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 > > > _______________________________________________ > Plasma-devel mailing list > Plasma-devel@kde.org > https://mail.kde.org/mailman/listinfo/plasma-devel > > Cheers -- Alessandro Diaferia
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
