[KDEREVIEW] DeviceNotifier refactor

[Alessandro Diaferia]

2009/2/3 Aaron J. Seigo <aseigo at 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 at kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
>

Cheers
-- 
Alessandro Diaferia
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20090203/9169324b/attachment-0001.htm