New devicenotifier moved to kdereview

[Jacopo De Simoi]

On Sunday 04 October 2009 20:57:11 Aaron J. Seigo wrote:
> On October 4, 2009, Jacopo De Simoi wrote:
> >  now, we believe that it is ready for merging into trunk. Of course we need
> >  your feedback first, so grab it, try it out and tell us what you think!
> 
> with options out of the way :) some thoughts on the plasmoid itself:
> 
> * it still says "Devices recently plugged in:" when there are no devices 
> listed. that's always seemed a bit odd. it should probably be replaced with an 
> applicable label and the divider line removed in that case

The divider line was put there to avoid the bad "cropped" feeling of the scrollwidget when scrolled down; 
this is actually being addressed in the scrollwidget itself, with the nice shadows that now appear when the scrollbar is visible, so I believe that it can go back to having it getting along with the categories.
Still I don't know if it makes sense to have one also for the first category.

> * when an item is expanded, should the background remain painted? it might 
> make it more evident that the item is "open", and it would also allow a way to 
> solve the next point, too. (and if the background remains, perhaps the 
> capacity bar should too?)

The background could  in principle stay, for the capacity bar there is an issue: there is no way to make a refresh signal 
"free space changed"; that's why we show it only on hover (and for the same reason it is like that for kfileplacesitem); 
 
> * each DeviceItem gets its own ItemBackground in case it has multiple actions. 
> it would be nice if they could share the one ItemBackground between them as 
> only one at a time can be shown anyways so anything more seems like a waste; 
> it would also allow the hover effect to track with the mouse more effectively 
> when there are multiple DeviceItems in the list. 

I don't think I understand this; if we had one itemBackground shared by all items we would see the selection bar move from a device to another one. Do I understand correctly?

> * when there is more than one action, and an action is selection, perhaps the 
> device item should collapse back to closed. this does two things: it gives 
> some immediate feedback that an action was selected, it saves a second click 
> needed to collapse the item again. the assumption here is that the click 
> action is almost always needed once, which seems to fit the use cases pretty 
> clearly. for a single device item this might not matter, but if there are 
> multiple devices this could help avoid having to scroll more than needed or 
> click around a lot. 

Makes sense!

> 
> * in DeviceItem there is this line:
> 
>     Plasma::IconWidget *actionItem = new Plasma::IconWidget();
> 
> where does this object get deleted?

good point.

> * in NotifierDialog there is a Plasma::Label ("category") and a 
> Plasma::Separator ("separator") created in 
> NotifierDialog::searchOrCreateDeviceCategory. they are deleted in 
> removeDevice, but that is only called when the source from the DataEngine goes 
> away, the device is hidden by the user or resetDevices is called (e.g. after 
> configuration). it seems these will leak if the applet is simply closed

right.

> 
> * there is another separator created in NotifierDialog::buildDialog that has 
> no parent. 
> 
> * i committed a work around for a Qt bug that caused the capacity bar to 
> sometimes be shown in the wrong place (QGraphicsLayout strikes again!)
Excellent, thanks.

 --J