[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Extender api review, round 2
From: "Aaron J. Seigo" <aseigo () kde ! org>
Date: 2008-07-30 19:12:15
Message-ID: 200807301312.15881.aseigo () kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/signed)]
On Wednesday 30 July 2008, Kevin Ottens wrote:
> Le Tuesday 29 July 2008, Rob Scheepmaker a écrit :
> > Q_PROPERTY(bool isPopup READ isPopup WRITE setIsPopup)
>
> setIsPopup should be setPopup to be consistent with the other Qt APIs...
> But then with such a name I wouldn't expect a bool. It seems to be trying
> to be the short form for something.
>
> So this property probably needs a rename altogether... I'll be cheating for
> a minute and look at the apidox of the relevant methods: "takes care of
> placing itself", "out of ordinary view" (odd term btw "ordinary view"). I
> guess it's used to change the state of the extender if it's inside the
> applet, or above the view in its own toplevel widget (or something like
> this... somehow referring to the "PopupApplet" class).
>
> Then I would propose something like "floating": isFloating() +
> setFloating(). Not sure it's the best term, but that looks like a decent
> start.
this is almost always used with classes or methods using the term "popup".
"floating" isn't used anywhere. with isPopup it's at least a decent hint that
it has something to do with popups (menus, PopupApplet, etc). i don't think
"floating" has any such useful connections.
> > ExtenderItem *extenderItem(const QString &name) const;
>
> Seems to be searching items by name... So, findItem()?
agreed.
> Aren't those signals enough? In fact it also makes me wonder if the hover
> events are useful, and if they are if they would deserve being signals only
> too.
good point, i wonder what the use of the hovert events are now as well, now
that you mention it.
> > void addAction(const QString &name, QAction *action);
> > QAction *action(const QString &name) const;
>
> Hmmm, I was wondering what it was for, so I looked at the apidox (yeah,
> cheater!). It asks for a rename I guess. I would propose something like:
> void addHandleAction(const QString &name, QAction *action);
> QAction *handleAction(const QString &name) const;
i don't agree; this exposes an implementation detail (that the action is used
in the handle) and addAction/action are consistent with QWidget API.
> > class PLASMA_EXPORT Applet : public QGraphicsWidget
> > {
> > public:
> > virtual void initExtenderItem(ExtenderItem *item);
>
> I can't help but wonder if that could be in another class. Could it be made
> a signal in Extender itself?
good idea if it works; does the consumer always have access to the Extender
object in this case? that would be my only concern. i suppose it would have
to, since it's the creation of an Extender object in the applet that enabed
Extender support in the applet in the first place.
a signal is also a bit more flexible, as it makes it easier to connect this
operation to other classes.
> > Extender *extender() const;
>
> I would remove this one. If someone needs to access its extender often
> he'll keep a pointer around.
and what about items outside of the Applet that would like access to the
Extenders? i wonder if Containment, Corona or any other classes would benefit
from that?
on the down side:
it's one more pointer in each applet.
it enforces a one extender per applet mechanism
on the up side:
using an extender is simply a matter of:
new Extender(this)
it enforces a one extender per applet mechanism ;)
--
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 Trolltech
["signature.asc" (application/pgp-signature)]
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic