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

[Dan Meltzer]

On 12/20/07, Jason Stubbs <jasonbstubbs at gmail.com> wrote:
> 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. :)

Will do.

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

Yea, I need to comment that.. It took me a little while to figure out
all of the possibilities.

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

Left over code from testing when I was afraid that a null value might
be causing things not to work.  I'll remove.

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

I did it primarily to be safe.. in case the trolls decide in the
future that qgi::hoverEvent is important we don't lose anything.. I
don't think it adds problems to have?

>
> 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()) {
>

Okay, I took a random guess at how to format that..

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

Me too!
>
> --
> Jason Stubbs
> _______________________________________________
> Panel-devel mailing list
> Panel-devel at kde.org
> https://mail.kde.org/mailman/listinfo/panel-devel
>