broulik added a comment.
Cooooool!
INLINE COMMENTS
> kbusyindicatorwidget.cpp:28
> +
> +class KBusyIndicatorWidget::Private : public QObject
> +{
`Q_DECL_HIDDEN`
> kbusyindicatorwidget.cpp:31
> + Q_OBJECT
> + Q_PROPERTY(qreal rotation MEMBER rotation WRITE setRotation)
> +public:
You probably want to be using `QVariantAnimation` instead of going through the
property system for that. Then your private also probably doesn't need to be a
`QObject`
> kbusyindicatorwidget.cpp:38
> + animation->setLoopCount(-1);
> + animation->setDuration(1000);
> + animation->setStartValue(0);
Plasma's `BusyIndicator` uses 1500ms
> kbusyindicatorwidget.cpp:62
> + QPropertyAnimation *animation = nullptr;
> + QIcon icon = QIcon::fromTheme(QStringLiteral("view-refresh"));
> + qreal rotation = 0;
Is that legal in the C++ standard allowed by frameworks?
> kbusyindicatorwidget.cpp:103
> +{
> + d->maybeToggleAnimation();
> + QWidget::hideEvent(event);
is `q->isVisible()` checked in this method already updated at this point or
should the `hideEvent` be processed before? Likewise for `showEvent`
> kbusyindicatorwidget.cpp:111
> +
> + d->paintCenter = QPointF(event->size().width() / 2.0,
> + event->size().height() / 2.0);
Does this widget need a `sizeHint` of a button or default icon size or
something?
> kbusyindicatorwidget.h:51
> + explicit KBusyIndicatorWidget(QWidget *parent = nullptr);
> + virtual ~KBusyIndicatorWidget();
> +
`override` instead
> kbusyindicatorwidget.h:65
> +protected:
> + virtual void showEvent(QShowEvent *event) override;
> + virtual void hideEvent(QHideEvent *event) override;
`override` is enough
> kbusyindicatorwidget.h:65
> +protected:
> + virtual void showEvent(QShowEvent *event) override;
> + virtual void hideEvent(QHideEvent *event) override;
I heard for good measure one should always re-implement the generic `event`
just in case
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D22375
To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns
