Review Request: Unified ItemBackground in new Device Notifier

[Aaron Seigo]

> On 2009-10-06 10:16:00, Jacopo De Simoi wrote:
> > /trunk/kdereview/plasma/applets/devicenotifier-refactor/deviceitem.cpp, line 390
> > <http://reviewboard.kde.org/r/1790/diff/3/?file=12370#file12370line390>
> >
> >     Now that the left(hehe)action stays activated when !isCollapsed() we should call m_leftActionIcon->setVisible(m_hovered) here
> >

the real problem here is that mouse release off of the item is accepted; it shouldn't be possible to collapse with a mouse click and not still be hovered. fixed in my local copy.


> On 2009-10-06 10:16:00, Jacopo De Simoi wrote:
> > /trunk/kdereview/plasma/applets/devicenotifier-refactor/notifierdialog.cpp, line 484
> > <http://reviewboard.kde.org/r/1790/diff/3/?file=12372#file12372line484>
> >
> >     We have a problem here: 
> >     itemHovered is connected to targetReached, however since now we have a timer (100ms) to hide the itemBackground it happens that a user could hoverLeave the selected item right before this signal is triggered; which leaves the item in the hovered state.
> >     
> >     I suggest adding a m_hoveredItem which is set/reset in eventFilter and is "promoted" to m_selectedDevice here.
> >     itemHovered should be called only if (item == m_hoveredItem)
> >

item->setHovered(false) is called on HoverLeave in NotifierDialog::eventFilter. the timer is not responsible for setting an item as not hovered, only for setting it as hovered.

as for the timer, m_clearItemBackgroundTargetTimer.start() is called when any device item gets a HoverLeave, collapsed or selected. it is also called when a selected item gets a HoverEnter. so it is always started except when a non-collapsed (aka selected) item gets a HoverEnter. i don't see where this timer could get out of step of the events.

so i don't see a problem in either case.


> On 2009-10-06 10:16:00, Jacopo De Simoi wrote:
> > /trunk/kdereview/plasma/applets/devicenotifier-refactor/deviceitem.cpp, line 327
> > <http://reviewboard.kde.org/r/1790/diff/3/?file=12370#file12370line327>
> >
> >     shouldn't this be (m_hovered || !isCollapsed()) as well?

yes, fixed in local copy.


> On 2009-10-06 10:16:00, Jacopo De Simoi wrote:
> > /trunk/kdereview/plasma/applets/devicenotifier-refactor/deviceitem.cpp, line 152
> > <http://reviewboard.kde.org/r/1790/diff/3/?file=12370#file12370line152>
> >
> >     Here we should make sure to hide() the description Label;

playing around with it some more, it feels odd to have some parts disappearing and others staying ... i've made all the header items remain when the item is expanded. the downside to this is that the capacity bar won't update when the item is expanded.


- Aaron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1790/#review2562
-----------------------------------------------------------


On 2009-10-05 23:27:10, Aaron Seigo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1790/
> -----------------------------------------------------------
> 
> (Updated 2009-10-05 23:27:10)
> 
> 
> Review request for Plasma, Jacopo De Simoi and Giulio Camuffo.
> 
> 
> Summary
> -------
> 
> proof of concept showing how the item background could be unified into NotifierDialog; requires today's libplasma for fixes to the ItemBackground widget
> 
> 
> Diffs
> -----
> 
>   /trunk/kdereview/plasma/applets/devicenotifier-refactor/deviceitem.h 1031762 
>   /trunk/kdereview/plasma/applets/devicenotifier-refactor/deviceitem.cpp 1031762 
>   /trunk/kdereview/plasma/applets/devicenotifier-refactor/notifierdialog.h 1031762 
>   /trunk/kdereview/plasma/applets/devicenotifier-refactor/notifierdialog.cpp 1031762 
> 
> Diff: http://reviewboard.kde.org/r/1790/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aaron
> 
>