Plasma::ToolTipManager API review

[Aaron J. Seigo]

On Monday 20 October 2008, Kevin Ottens wrote:
> Surprisingly (or not) my conclusions on this one will be radical. I think

=P

> * ToolTipManager (QObject *parent=0)
> * ~ToolTipManager ()
>
> ToolTipManager being a singleton, I'd say they probably shouldn't be
> public. Even better, to get closer from QToolTip, we should probably have
> no ctor, dtor in this class and only static methods.

agreed

> * bool   isWidgetToolTipDisplayed (QGraphicsWidget *widget)
>
> Used only once (in digital-clock). Probably should be
> isToolTipVisible(widget)?

yes

> * void   showToolTip (QGraphicsWidget *widget)
> * void   delayedHideToolTip ()
> * void   hideToolTip (QGraphicsWidget *widget)

the first one was used in a recent patch until i changed the patch ;) but i 
think there are reasonable use cases for these things (action occurs -> show 
tip without wating for mouse hover) so we'll end up adding them at some point 
anyways.

> * void   registerWidget (QGraphicsWidget *widget)
>
> Used several times, but always followed by a setToolTipContent() call...
> first line of setToolTipContent() being registerWidget(). Very good
> candidate for removal IMO.

registerWidget allows you to register a widget, in other words say "watch this 
widget" without allocating memory right then for a (usually useless) tooltip.

what ought to happen is either all those calls to setToolTipContent be modified 
to use the delayed creation approach (nicer on memory usage) or just not call 
registerWidget in the first place.

> * void   unregisterWidget (QGraphicsWidget *widget)
>
> Used only once by a good citizen. Otherwise widgets are never unregistered,
> even if the tooltip content gets empty. Just like registerWidget() is
> called by setToolTipContent(), this method should probably call
> unregisterWidget() for empty content.

that implies we'd have to keep all tooltips in memory then. 

registering a widget gets it watched, but it doesn't have to have any tooltip 
data registered yet. we really don't want all those dozens of tooltips hanging 
in memory "just in case" especially since many of them need to be re-done when 
called anyways.

> * void   setToolTipContent (QGraphicsWidget *widget, const ToolTipContent
> &data)
>
> This one is very much used. Should stay. I'm not sure what to do about
> ToolTipContent though. Makes sense internally, but maybe in the public API
> you want to pass the parameters in the method. Not having to construct an
> object, set the members, then make the call.

have you seen how many items we pass in? =)

> * void   clearToolTipContent (QGraphicsWidget *widget)
>
> IMO useless, and used only once. Use setToolTipContent() with empty content
> instead. Would allow to be in sync with QToolTip behavior wise.

yes, makes sense.

> * bool   widgetHasToolTip (QGraphicsWidget *widget) const
>
> Never used, remove until really needed?

sure ...

> * void   setToolTipActivated (QGraphicsWidget *widget, bool enable)
>
> Used only in Kickoff. It really looks like an implementation detail which
> creeped out in public interface. I'd expect code to remove the tooltip then
> set it again. This way we get only one mecanism for the task.

in fact, there's quite an easy way to do this even: implement a slot called 
toolTipAboutToShow in your class. so yes, kickoff should be fixed and this 
removed.

>   bool   isToolTipActivated (QGraphicsWidget *widget)
>
> Never used, remove until really needed?

it's probably a functional duplication of widgetHasTooltip, no?

> I'm not fully happy with it yet,

but are you ever? ;-P

> but I'd like to get feedback to see if
> it's at least a good start. Improvements missing are in my opinion with
> removing the "ToolTip" part in the method names.

mm ... sure.. why not.

> Probably also ditching the
> "Manager" part in the class name would be an idea.

yes.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Qt Software

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20081020/f92463be/attachment-0001.sig