D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190710/b97a0798/attachment-0001.html>