Feedback on KNotificationItem

[Marco Martin]

On Tuesday 15 September 2009, Michael Pyne wrote:
> Hi all,
>
> So I took the opportunity to try porting JuK's system tray icon to
> KNotificationItem.  Here's some feedback:
>
> * First off, I like the idea as I've always thought that embedded widgets
> was probably the wrong way to be going about notification item entries in
> the first place, and this has been working fine for me so far, which is
> good. 
> * The API documentation could use improvement.  I've fixed up some of
> the errors that I saw but here's some specifics:
> - I had to read the source to be sure that I could remove an overlay icon
> by doing setOverlayIconByName(QString()) for instance.

ok, will correct

> - What is "bool active" in activateRequested for?

activate/deactivate, is for the behaviour of clicking over icons: you click 
one time over the icon and it shows the app window, click again and it hides 
it (in this case an activateRequested(false) will be emitted

> - There's a lot of mixing of QIcon and "pixmap" (more on that in a bit)
> - What does eventFilter do for an icon with no widget?  It's protected, not
> private, so it should still be documented a bit I think.
hides the popup menu, quite of an ugly hack, i'll see if i can move it in one 
of the private classes

> - It may be prudent to give an example of a "secondary" activate action, as
> mentioned in the source code (middle-click)
ok
> - setAttentionIconByName mentions a pixmap paramete
> - The detailed description mentioned the Dbus spec.  Is the spec available
> online to be linked to?
at the moment is just on my clone of the xdg specs gitorious repo

hope will be in a more accessible place soon :)
> - Several signals are referenced in the APIDOX but not defined (presumably
> they're private?)  For instance newToolTip() and newStatus().
yeah, signals of the dbus interface, should be removed from there
> - What are the title and subtitle for in tool tips?  Can they be HTML/rich
> text? (It's easy enough to test, but it would be nice to be clear in the
> documentation).
will do. btw title is just text, subtitle can have html
> - showMessage() is completely undocumented from what I can tell.
> * "These two will die..." in knotificationitem.h.  If it's true that the
> setAttentionMovie overloads for the QVectors will not make it I'd recommend
> removing it now (or BIC Monday) before the release so we don't forget.
> * Is it not possible to use const QMovie * instead of QMovie * in
> setAttentionMovie?  I see no reason a read-only movie can't be shown in the
> system tray.
hmm, i think QMovie *movie could be replaced with const *QMovie ?
> * setToolTip(const QPixmap&, const QString &, const QString &) is declared
> in the .h but never implemented in the .cpp :(
fixed

> Of course that's all minor stuff that didn't interfere in the port (except
> for the tooltips, but I'll make it work).  I was pleasantly surprised to
> see that both scroll wheel and middle-mouse click detection were present
> for instance.
>
> One thing that is hampering the port is that JuK has a "track announcement
> popup" that pops up next to the system tray icon.  Without the embeddable
> widget, JuK doesn't know where the icon is.
>
> So, can I use KNotify for this (the popup would need to support graphics,
> rich text, and at least two buttons)?  Would KNotification show up in the
> right spot?
yes, i think knotification would be the proper way to do it.
notifications are always displayed near the systray and they support an image, 
html and an (i yhink) arbitrary number of buttons

> If this isn't the case, what would be the best way to get access to that
> information?  Preferably we wouldn't have to re-tool the spec at this late
> stage.

as i explained in the other mail it's a bit difficult to do it reliably. 
perhaps possible but i really hope it could be avoided :)

> Thanks for your time,
>
> Regards,
>  - Michael Pyne

will try to correct all points adderred :)

Cheers,
Marco Martin