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

[David Jarvie]

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

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.


- David


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