[Kget] [PATCH] KGet Tray Icon and its Overlay

[Fabian Henze]

On Saturday the 25th April 2009 00:27:24 UTC, Lukas Appelhans wrote:
> Hey!
>
> On Donnerstag 23 April 2009 22:54:41 Fabian Henze wrote:
> > Hi,
> > Currently KGet4 shows a small blinking play icon above its normal tray
> > icon, if a download is going on at the moment. This patch removes the
> > blinking as well as the pause icon and moves the play icon to the lower
> > right corner. There are several reasons for this:
> > a) Blinking is bad, as the user has a 50% chance not to see the icon when
> > he wants to check if his/her download is still running. That is annoying
> > and slows down workflow.
>
> Mmh, but animating means, something is running... :)

I still think, that a blinking animation is not the right way to go. Maybe we 
should ask the usability folk?

> > b) What is the pause icon for? It does not provide any valuable feedback
> > for the user.
>
> pause is just shown one time...

I know, but what for? If its just shown one time on an area of 16x16 pixels 
the user wont see it in more than 90% of the cases.

> > c) About the placement: Amarok 2.1 shows the same icon in the lower right
> > corner, so I moved it there for consistencies sake.
>
> no offense, but we really shouldn't care what Amarok does... it has imo no
> value as an argument...

Maybe you are right, but the placement has no huge impact on anything. I just 
changed it because I like a consistent look in my systray.

> > I don't know if my approach to change this behaviour is the right one,
> > but it seems to work and so I decided to publish the patch.
> > While reading though tray.cpp I also wondered what the three "big" Icons
> > are for (baseIcon, grayedIcon and alternateIcon). Maybe you can answer
> > that question.
> > This is my first patch to a KDE related program, so please tell me if I
> > did any mistakes :-)
>
> Just took a short look at it, but it seems that playOverlayVisible and
> m_running pretty much indicate the same thing... ;) (although I have to say
> that I'm not much into that code, so others might have better comments...
> :))

Yeah, I noticed that too today. I also found that Tray::blendOverlay() is a 
bit long winded (I think I will publish an updated patch later that day). 
However I still don't understand what Tray::paintIcon() does in detail (or 
rather why it does some things).

> Well anyway great that you took a look into the code and prepared a
> patch... Imo the way to go on would be to have the play-overlay animated
> (aka fade ins and outs...) and only do that with animations enabled... else
> just show the overlay as done in this patch...

In my opinion an animation would generate too much CPU wakeups for no good 
reason.


-- Fabian 'Flyser' Henze