[Kde-pim] Review Request: Port KAlarm to KStatusNotifierItem

[Aurélien Gâteau]

> On 2010-04-10 12:06:39, David Jarvie wrote:
> > I've had a quick look at KAlarm with this patch. It looks good, but there are some issues.
> > 
> > 1) The systray icon doesn't change for me depending on the disabled/part disabled/enabled status. (I did check that the ox8-* icons had been installed properly.)
> > 
> > 2) I think that the notification needs to remain visible for longer - it hides itself very quickly, leaving not enough time to read the times for a list of alarms. Even with just one alarm, I have to move the mouse and re-show the notification just in order to read the time displayed, if I'm not concentrating and looking for the time.
> > 
> > 3) Am I right in supposing that KStatusNofifierItem can't use icons/pixmaps which have been programmatically generated, so that is the reason you've created a disabled icon file?
> > 
> > 4) The new icons will need to be created in a larger size (22x22). Probably the best way would be to do a screen dump of the old icons.
> > 
> > 5) Can you please adhere to KAlarm's coding style for braces and tabs - braces are on a new line, and braces aren't used for a single-line 'if'/'else' statement. Use tabs to indent (which you have done), but use spaces thereafter to indent further for statement continuation lines.
> 
> David Jarvie wrote:
>     I just looked at the contents of the two ox8-* icons, and realise that you are still using overlays after all, so please ignore comments 3 and 4.
>     
>     I'm not convinced that a tiny emblem is adequate to indicate that KAlarm is disabled - I think that the existing method of greying out the systray icon is better since it's more obvious.

1) Weird, it worked here, will give it another try.

2) I guess you are talking about the tooltip. This is in the hand of the visualization, ie the systemtray applet. There is no way to get a custom timeout for the tooltips. Maybe Plasma tooltips could be made smarter to use a timeout proportional to the amount of text?

3) It can use generated images, but it is:
- less efficient, because the whole image data must be sent over dbus, instead of sending only a filename
- limiting for the visualization. If the visualization knows the name of the image it can do things like loading a version at the appropriate size, or even replace it with a visualization specific version, for example to provide a consistent look

For these reasons, applications should pass images via filenames whenever possible.

Additionally, the GNOME implementation provided in the upcoming Ubuntu does not (at least for now) support generated images :/

5) Sure, will fix.

About the tiny emblem problem: I did this to be able to pass images by name, but I agree the emblem is less obvious (this may be fixable in the systemtray applet code, though). Maybe the best solution is to get the Oxygen designers to create a kalarm-disabled icon?


- Aurélien


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


On 2010-03-23 20:37:01, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3374/
> -----------------------------------------------------------
> 
> (Updated 2010-03-23 20:37:01)
> 
> 
> Review request for KDE PIM and David Jarvie.
> 
> 
> Summary
> -------
> 
> This is a port of KAlarm from KSystemTrayIcon to KStatusNotifierItem. Note that it makes use of overlays, so I created (warning, hacker "art"!) two emblems for it, which you can download here:
> http://people.canonical.com/~agateau/tmp/ox8-emblems-disabled.png
> http://people.canonical.com/~agateau/tmp/ox8-emblems-partdisabled.png
> 
> The emblems should be placed in kalarm/pixmaps/
> 
> (The ox22-emblems-partdisabled.png file from this dir can be removed)
> 
> Unfortunately, there is no way for a KStatusNotifierItem to be notified when the tooltip is about to be displayed so I had to resort to add code which updates the tooltip whenever necessary.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepim/kalarm/kalarmapp.cpp 1106735 
>   trunk/KDE/kdepim/kalarm/kalarmconfig.kcfg 1106735 
>   trunk/KDE/kdepim/kalarm/mainwindow.cpp 1106735 
>   trunk/KDE/kdepim/kalarm/traywindow.h 1106735 
>   trunk/KDE/kdepim/kalarm/traywindow.cpp 1106735 
> 
> Diff: http://reviewboard.kde.org/r/3374/diff
> 
> 
> Testing
> -------
> 
> - Created tasks, disabled some of them, disabled the whole of kalarm: icon was correctly updated.
> - Checked the tooltip is always up to date, including when it is setup to show the remaining time of an alarm and including following tooltip preferences changes.
> 
> 
> Thanks,
> 
> Aurélien
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/