Plasma::ToolTipManager API review

[Aaron J. Seigo]

On Tuesday 21 October 2008, Kevin Ottens wrote:
> 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).

hm.. ok, that one can go internal...

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

right..

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

thing is that the tooltip data itself isn't tiny (it's not HUGE either, 
though) and it often isn't set once but highly context sensitive. see the 
taskbar or clock items for examples of this. so we'd be store good amounts of 
bogus data.

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

one and four, actually.

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

it already has been =)

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

always the optimist ;)

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

the idea is for devices where we *never* want tooltips we also tend to be more 
memory constrained ... sooooo ...

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

yeah... 

>   // no default, on purpose ;-)

all that does it make people write more lines of code. which is silly.

-- 
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/20081021/221ccbbd/attachment.sig