Plasma::ToolTipManager API review

[Kevin Ottens]

Le Tuesday 21 October 2008, Aaron J. Seigo a écrit :
> > * 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.

Agreed, except for delayedHideToolTip(), AFAIK it's only used internally, this 
one is an implementation detail IMO (interestingly it's the only one not 
taking a QGraphicsWidget arg BTW).

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

OK, I see, it's the "problem" of having this tooltip system concurrently with 
the regular tooltip system of Qt.

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

Couldn't setContent() take care of the delayed creation itself then? It keeps 
the content around (obviously) but allocate the ToolTip object only when 
needed.

Actually that's what happens in Qt itself when you use setToolTip() on a 
widget. The widget only stores the content necessary to create the tooltip, 
but the tooltip is created by the QToolTip::showText() call which happens on a 
ToolTip event.

I guess the main problem with Plasma::ToolTipManager right now, is that it is 
both QWidget::setToolTip() and QToolTip::* feature wise.

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

Not necessarily, see above.

> registering a widget gets it watched, but it doesn't have to have any
> tooltip data registered yet.

It's the case when applications QToolTip from the Qt API, but it's done 
because of application needs not how the tooltip system is implemented 
underneath.

> > * 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? =)

Between one and three? Doesn't looks like a big deal to me, but maybe that's 
just me. :-)

> >   bool   isToolTipActivated (QGraphicsWidget *widget)
> >
> > Never used, remove until really needed?
>
> it's probably a functional duplication of widgetHasTooltip, no?

Seems to be a slightly different semantic, since you can have a tooltip, but 
this one not being activated... oh well. I still think it could be removed 
anyway. :-)

> > I'm not fully happy with it yet,
>
> but are you ever? ;-P

Well, when it's my own production: never. You already know that, whatever I do 
turns sour I think.

Now in your changes last night you added something about state of the manager: 
activated, deactivated, inhibited. I'm wondering about this three/state thing, 
wouldn't inhibited or not be enough? In your doc about deactivated, it means 
ignoring calls to setContent(), I'm not really confident with having a method 
silently discarding the content passed depending on a state which might be set 
somewhere else. Looks like a neat way to shoot yourself in the foot.

So, second try for the API taking into account your commits and the above:
------------------------------
class ToolTip
{
    static void show(QGraphicsWidget *widget,
                     const ToolTip::Content &content = ToolTip::Content());
    static void hide(QGraphicsWidget *widget);
    static void isVisible(QGraphicsWidget *widget);

    static void setInhibited(bool inhibited);
    static bool isInhibited();

    // no default, on purpose ;-)
    static void setContent(QGraphicsWidget *widget,
                           const ToolTip::Content &content);
};
------------------------------

I kept static because AFAIK you weren't against it, so I assumed it was 
planned but "not done yet".

Now, I also tried to make clear we had three groups of methods there:
1) show/hide/isVisible
I grouped them together to make it clearer: it looks a whole lot like QToolTip 
now. You could almost replace one API with the other, which is ++good IMO. 
Then show() would also be used for the case of "I don't want to set some 
content on an item, I manage my tooltip myself".

Possible room for improvement: consider that only one tooltip can be shown at 
a time (which seems logical to me, but that's your call), in which case 
isVisible() and hide() could loose their arg.

2) (set|is)Inhibited
Well... inhibit support. :-)
I wouldn't make more states to avoid the "Deactivated means discarding calls 
silently" situation which doesn't look healthy to me (as explained above).

3) setContent
This one somehow looks "out of place", it's basically the setToolTip() method 
we can't use on QGraphicsItem for our themed tooltip. That's what makes 
Plasma::ToolTip kind of an odd-duck, but that's still fine IMO. This one does 
the registering/unregistering itself, so no need for those ones in public API 
anymore.
Possible changes there:
 * get ride of ToolTip::Content and expand the args, but apparently you don't 
like that. :-)
 * alternatively, use a QString, and consider it rich text, would allow more 
free form tooltip contents in the future... otoh means less consistency.

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."
-------------- 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/20081021/49282b61/attachment.sig