Review Request: Unified ItemBackground in new Device Notifier

[Giulio Camuffo]

> 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;
> 
>  wrote:
>     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.
> 
>  wrote:
>     As for making the description label less redundant, if we really feel bad about it, we could add a ":" at the end when the device gets expanded...
>     
>     However, the capacity bar  should /always/ display a proper value, I understand that it would get updated on hover anyways, but that seems to me more broken than hiding/showing it.
>     
>     Perhaps we should show it in a less glamorous way, for instance as a tiny capacity bar below the icon (a few pixels high) and the label (xxx Gib free) below the description... then seeing it appear and disappear should bother the user that much...
>     
>     Mmh, that doesn't even convince me.

now i'll say an absurdity :P isn't it possible to use the notifications events sent by the kernel to update the capacity bar? arent't inotify, dnotify and what not supposed to do that?


> 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)
> >
> 
>  wrote:
>     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.
> 
>  wrote:
>     The problem is the following: suppose you have 2 items: x and y; all times are expressed in ms:
>     
>     at time 0 you leave item x 
>     at time 50 you hover on item y, m_itemBackground starts its animation from item x to item y
>     at time 300 you leave item y and you do not hover any other item (e.g. out of the plasmoid), the timer is started; y->setHovered(false)
>     at time 350 m_itemBackground arrives on y, signals targetReached, which calls itemHovered, which calls y->setHovered(true)
>     at time 400 m_itemBackground gets hidden, but nobody resets the status of item y, which stays in the "hovered" state.
>     
>     Despite the fact that is seems complicated to reproduce, just waving your mouse over the plasmoid triggers this situation

yes, i could reproduce the bug. it's simple if you move quickly the mouse from the top to the right side of the scrollwidget hovering an angle.
maybe the solution proposed by Jacopo isn't so beautiful, but i can't think of any other.


- Giulio


-----------------------------------------------------------
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
> 
>