[Panel-devel] Tooltips for Plasma::Widget's

[Jason Stubbs]

On Thursday 20 December 2007 13:08:00 Dan Meltzer wrote:
> On 12/18/07, Jason Stubbs <j.stubbs at linkthink.co.jp> wrote:
> > On Wednesday 19 December 2007 02:57:03 Dan Meltzer wrote:
> > > On 12/18/07, Aaron J. Seigo <aseigo at kde.org> wrote:
> > > > On Monday 17 December 2007, Dan Meltzer wrote:
> > > > > Feedback is much appreciated!
> > > >
> > > > just did a review of the patch on irc with Dan, just so everyone
> > > > knows i haven't ignored this one ;)
> > >
> > > And here's the updated patch.
> >
> > You might want to do a grep in lib/plasma{,widgets} to make sure there's
> > nothing deriving from Widget that reimplements hover* without calling the
> > parent. I'm at work so can't check (easily) but I'm pretty sure Icon
> > would need adjusting.
>
> I'm attaching a patch that, along with other things, fixes the places
> where I found this.

Plasma::Icon is already doing the right thing, but there's also 
Plasma::CheckBox and Plasma::RadioButton. I'm not sure anything is using them 
at the moment, but may as well fix them before they become a problem for 
somebody. :)

> This patch also adds a delay on showing the tooltip and is in general,
> a bit cleaner.

The delay code looks good, although it took me a while to figure out exactly 
what the isShown flag is all about. You might want to add a comment in show() 
and hide() just before the if blocks to explain how isShown could be either 
true or false. It might be immediately obvious after witnessing its behaviour 
though - I haven't applied yet. ;)

In Widget::hoverEnterEvent(), you skip showing a tooltip if any of the 
possible fields are not set. Is there any reason to require an icon and/or 
subtext? A simple tooltip might come in useful too.

One last thing (which might be a code convention that I don't know about) but 
why the e->accept()s? It doesn't do anything in hover events and always 
defaults to accept() in methods where it is relevant.

Likewise with calling QGraphicsItem::hoverEvent() as it doesn't do anything 
except possibly call update(). The documentation is unclear on whether it 
even does that much - in my experience, it doesn't.

Oh, and seeing Aaron told me off... ;)

+ if (d->toolTip.image.isNull()
* || d->toolTip.subText.isNull()
+ || d->toolTip.mainText.isNull()) {

should be

+ if (d->toolTip.image.isNull() ||
*         d->toolTip.subText.isNull() ||
+         d->toolTip.mainText.isNull()) {

> > Also, perhaps ToolTip (other than ToolTip::Data) might be better in a
> > widget_p.h and widget.cpp so as to not make it part of the public API? Or
> > is there some reason that ToolTip needs to be accessible?
>
> I've also wondered this, the question would be.. what to do with the
> struct?  Should it become Widget::ToolTipData ?

Not sure... Having a QWidget derived class in plasma/widgets doesn't sit right 
though. Widget::ToolTipData would work, as would three new properties on 
Widget although (and I'm playing Aaron again here ;) you'd need to keep it as 
a struct internally anyway else memory requirements will increase.

I'd love to hear what other people think on this point.

-- 
Jason Stubbs