D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[René J . V . Bertin]

rjvbb added a comment.


  A little tinker tool:
  
  github.com/RJVB/kbusygadget.
  
  An inter-frame freeze duration of 75ms already decreases CPU load (according 
to `top`) to approx. 3.6% (= almost 4x). I cannot really say if I notice the 
effect of this short a freeze on the rotation smoothness or speed.
  
  I will use this to "take things up wit Qt" as suggested - i.e. on the 
interest ML.
  
  R.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[René J . V . Bertin]

rjvbb added a comment.


  And my point is that you are doing 720 translations and 360 rotations per 
cycle, with subsequent smoothing of an image, continuously and with sufficient 
temporal resolution to get a fluid animation that is completely overkill here. 
Indicating a busy state (a two-state entity) is not the same as indicating 
progress and could be done by something like a stoplight changing colour.
  
  User experience ... do you seriously expect anyone to judge KDE on this sort 
of thing (that'd be like judging a service provider on the waiting music they 
stuff down your ears while you're on hold). Maybe among the angry teenager 
crowd who spend most of their computer time customising the looks of their 
desktops ... and possibly the designers of the fake interfaces you see in yet 
another CSI-like series.
  
  In my book an interface shouldn't get in the way, neither in its use of space 
nor in terms of required computing resources, and should continue to be 
responsive even if the system is swamped doing the actual work I gave it t do. 
FWIW, even Apple have made more and more of the the animations introduced after 
iOS 6 optional because they killed the actual user experience on all but the 
latest iDevices (as well as battery life).
  
  I'm not blaming Q*Animation, and I don't think anything is inherently wrong 
with it (apart possibly from an apparent lack of control over 
granularity/temporal resolution). That lack does make it the wrong tool IMHO.
  
  I'm tinkering with an implementation that follows my idea of storing the 
calculated rotated icons in a QVector, using the answer to this question 
(https://stackoverflow.com/questions/4665606/rotate-image-in-qt). That should 
remove the computational overhead after the first animation cycle and give the 
same wonderful user experience as the current implementation. If it doesn't 
decrease the CPU overhead then maybe indeed there's a problem with Q*Animation 
in the Qt version I'm using.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added a comment.


  My point is that your lamentations have nothing to do with this class but 
with Q*Animation on your system. So you need to find out what's wrong and talk 
to Qt. I am 100% against a workaround that degrades the user experience when 
the bug isn't even in this class.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[René J . V . Bertin]

rjvbb added a comment.


  In D22375#541399 , @sitter wrote:
  
  > The demo doesn't even use this widget
  
  
  That wasn't the question I answered by referring to oxygen-demo
  
  > breeze
  >  F7506213: Peek 2019-10-03 13-46.gif 
  > 
  > oxygen
  >  F7506215: Peek 2019-10-03 13-45.gif 
  
  The fact you are seeing much lower loads on your machine doesn't mean that no 
machine is going to be slowed down by this widget or other comparably "useful" 
animations. For all we know you have a rig with 256 cores so you're not going 
to notice if one of those is  used at even near 100% for a bit of eye candy.
  
  Also, I prefer to use lower level measuring tools that interfere less with 
the system being measured than a fancy tool like ksysguard - and guess what: 
top gives me a completely different reading:
  F7506717: sysload1.png 
  
  I'd do the same snapshot for `kbusyindicatorwidgettest` but I'd have to 
recompile KWidgetsAddons without my tweak first :)

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added a comment.


  The demo doesn't even use this widget
  
  breeze
  F7506213: Peek 2019-10-03 13-46.gif 
  
  oxygen
  F7506215: Peek 2019-10-03 13-45.gif 

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[René J . V . Bertin]

rjvbb added a comment.


  >   Does it happen with every code that uses QPropertyAnimation, or just with 
this KBusyIndicator?
  
  I don't know, neither for QVariantAnimation (which is used here).
  
  Testing just now (on the N3150 machine) with the Sliders page of the 
oxygen-demo app I get between 30% and 35% CPU (!) with Fusion, Breeze and 
Oxygen (which all use something based on QAbstractAnimation). QtCurve uses an 
internal, QTimer-based implementation that uses about half that (still too much 
I realise now).
  
  I should add that I use a traditional icon theme, pixmap instead of svg 
based. Rotating pixmaps is maybe much more expensive than rotating and svg, and 
both probably depend on how much of it is done in vector intrinsics.
  
  Traditionally, this kind of animation used either pre-calculated frames (cf. 
the busy cursors) or else colourmap animation (which is *very* cheap). The 
latter is not going to be feasible here but building table (QVector?) of a 
reasonable number of pre-rotated pixmaps and looping over that should be 
possible (can even be done "online" by caching the frames as you display them). 
Of course my current workaround is a lot easier and should be fine a long as 
the animation doesn't run on the same thread as the actually busy one.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added a comment.


  F7505620: Peek 2019-10-03 12-23.gif 

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added a comment.


  Does it happen with every code that uses QPropertyAnimation, or just with 
this KBusyIndicator?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[René J . V . Bertin]

rjvbb added a comment.


  I'll repeat here what I muttered on the associated commit page:
  
  This widget adds a lot of CPU overhead, too much IMHO: the dedicated test 
tool runs at a bit over 10%CPU, and that is not counting the additional 
overhead from the displaying layers (X server, the Mac WindowServer, etc). This 
overhead appears to scale with the CPU: it's in the same order of magnitude on 
my lowly N3150 Linux machine as on my (older but still) much faster Mac that 
has an i7.
  
  Adding a simple
  
animation.thread()->mSleep(250);
  
  after the `q->update();` line does make the animation a bit choppier but 
reduces the test tool's CPU load to under 1%.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: rjvbb, ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
GB_2, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:2631be903f94: new class KBusyIndicatorWidget similar to 
QtQuicks BusyIndicator (authored by sitter).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22375?vs=61789=61794

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  docs/pics/kbusyindicatorwidget.png
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck accepted this revision.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter updated this revision to Diff 61789.
sitter added a comment.


  typos--

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22375?vs=61659=61789

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  docs/pics/kbusyindicatorwidget.png
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kbusyindicatorwidget.h:52
> + *
> + * @since 5.60.0
> + */

5.61.0

> kbusyindicatorwidgettest.cpp:43
> +
> +auto toggle = new QPushButton(QStringLiteral("Toggle Visibile"), 
> );
> +

typo

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter updated this revision to Diff 61659.
sitter added a comment.


  - add simple example code + image
  - add @since tag
  - override event

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22375?vs=61603=61659

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  docs/pics/kbusyindicatorwidget.png
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
sbergeron, michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  LGTM

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck, apol
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter updated this revision to Diff 61603.
sitter added a comment.


  get rid of need for decl_hidden and extend the test to play with visiblity

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22375?vs=61522=61603

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> broulik wrote in kbusyindicatorwidget.cpp:62
> Is that legal in the C++ standard allowed by frameworks?

Well, it builds at least under c++11 which is what kf5 is compatible with.

According to this page 
 
it should be fine (assuming the caveats mentioned are exhaustive anyway).

> kossebau wrote in kbusyindicatorwidget.h:40
> Any chance of getting some samples how this class is supposed to be used?
> 
> Sounds one should show & hide the complete widget when needed? How to best 
> integrate in one's layout? As overlay?
> 
> BTW, the KDE HIG does not mention such a spinner. So the purpose from a KDE 
> developer following the HIG raises a question with me wearing my naive hat :)
> https://hig.kde.org/components/assistance/progress.html

I am not sure there is a generally useful code sample to give here. Certainly 
not a helpful one.

  auto indicator = KBusyIndicatorWidget(this);
  layout().addWidget(indicator);
  auto label = QLabel("Busy watering the folowers", this);
  layout().addWidget(this);

Maybe this, but then I am not convinced of its usefulessness. That's just how 
one would use any widget ^^

You could overlay it on something, or HBox it next to a label, or add it as 
permanent widget to a StatusBar, or VBox it with something. Sky's the limit 
really.

> broulik wrote in kbusyindicatorwidget.h:65
> I heard for good measure one should always re-implement the generic `event` 
> just in case

Do we have this document somewhere? That's the sort of thing that sounds like 
an urban myth someone started in the 90's ^^

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: ngraham, kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, 
michaelh, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kbusyindicatorwidget.h:40
> + * is more specific.
> + */
> +class KWIDGETSADDONS_EXPORT KBusyIndicatorWidget : public QWidget

Any chance of getting some samples how this class is supposed to be used?

Sounds one should show & hide the complete widget when needed? How to best 
integrate in one's layout? As overlay?

BTW, the KDE HIG does not mention such a spinner. So the purpose from a KDE 
developer following the HIG raises a question with me wearing my naive hat :)
https://hig.kde.org/components/assistance/progress.html

> kbusyindicatorwidget.h:57-58
> +private:
> +class Private;
> +Private *const d;
> +};

You can make this a non-nested class by implcitly forward declaring here:

  class KBusyIndicatorWidgetPrivate *const d;

This allows not needing the Q_DECL_HIDDEN for the symbols of the then no longe 
nested private class.

Even more fancy:

  QScopedPointer const d;

No longer the need to delete the d explicitely :)
Though perhaps not familiar in look of cod too many.

> kbusyindicatorwidgettest.cpp:31
> +w.setBaseSize(128, 128);
> +w.show();
> +

Could this test be somehow extended to cover repeated show & hide?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: kossebau, broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, 
ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter updated this revision to Diff 61522.
sitter marked 14 inline comments as done.
sitter added a comment.


  - running property gone since we have no use case + simplified code since we 
no longer account for it
  - dropped superfluous virtual keywords
  - bumped duration to 1.5s
  - moved to qvariantanimation + alloc animation on stack
  - now has minimumsizehint based on PM_SmallIconSize
  
  I am actually not sure about what needs overriding for best sizehiniting, I 
figure minimumsizehint is the bare minimum? Should we also set a sizehint?

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22375?vs=61506=61522

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> sitter wrote in kbusyindicatorwidget.h:48
> Mimics the QML API, haven't given it any thought TBH. You can have the 
> spinner visible but paused, I am not sure why exactly you'd want a paused 
> spinner but that's what the property does anyway.
> 
> i.e.
> 
> - you can have a visible but !running spinner which would simply be the 
> spinner at whatever the last rotation update was.
> - a visibile and running spinner = animation
> - a running but !visible spinner = effectively noop
> 
> That said, **I** have no use case for it so we can remove it for the time 
> being if you'd prefer.

Please remove it unless you find a justification. I don't like its name either 
(the widget isn't running).

> kbusyindicatorwidget.h:72
> +class Private;
> +Private *d;
> +};

`*const d`

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> broulik wrote in kbusyindicatorwidget.cpp:111
> How about `PM_LargeIconSize`

Oh, that would work!

> cfeck wrote in kbusyindicatorwidget.h:48
> Why is this property needed? If the (parent) widget is no longer busy, this 
> spinner needs to be hidden anyway.

Mimics the QML API, haven't given it any thought TBH. You can have the spinner 
visible but paused, I am not sure why exactly you'd want a paused spinner but 
that's what the property does anyway.

i.e.

- you can have a visible but !running spinner which would simply be the spinner 
at whatever the last rotation update was.
- a visibile and running spinner = animation
- a running but !visible spinner = effectively noop

That said, **I** have no use case for it so we can remove it for the time being 
if you'd prefer.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kbusyindicatorwidget.h:48
> + */
> +Q_PROPERTY(bool running READ running WRITE setRunning NOTIFY 
> runningChanged)
> +public:

Why is this property needed? If the (parent) widget is no longer busy, this 
spinner needs to be hidden anyway.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Christoph Feck]

cfeck added a comment.


  Ideally you can create it with any size as an overlay to an existing widgets 
(also to block input there), but the spinner itself is only rendered at a 
smaller centered position.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> sitter wrote in kbusyindicatorwidget.cpp:111
> I was thinking about it, and honestly I am not sure. I don't think we have 
> access to anything to do with icon units, so we could hardcode some 
> dimensions (22,22 I guess) but that's about it.

How about `PM_LargeIconSize`

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> broulik wrote in kbusyindicatorwidget.cpp:111
> Does this widget need a `sizeHint` of a button or default icon size or 
> something?

I was thinking about it, and honestly I am not sure. I don't think we have 
access to anything to do with icon units, so we could hardcode some dimensions 
(22,22 I guess) but that's about it.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D22375

To: sitter, cfeck
Cc: broulik, kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

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

D22375: new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator

[Harald Sitter]

sitter created this revision.
sitter added a reviewer: cfeck.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
sitter requested review of this revision.

REVISION SUMMARY
  this mimics QQC's BusyIndicator and more specifically our styling of it.
  
  KBIW loads an icon from the icon theme, scales it to the widget size
  and rotates it 360 degrees every second for as long as it is running.
  
  the intent here is to give an easy to use spinner implementation that looks
  like and feels (to the developer) like the one seen in plasma/kirigami.
  this does however somewhat infringe on the business of kpixmapsequence,
  so here's why KBIW is better for this specific use case:
  
  - not pixmap based
  - because it's not pixmap based scaling works much better for SVGs
  - since we paint a QIcon directly we don't have to manually faff about with 
pixmap copies/segments
  - more robust as themes may incorrectly or not at all implement the animation 
icon spec (which is rather offhandedly specified really). KBIW takes care of 
the animation so the theme need only supply a very standard icon and there is 
no change for things to go wrong more or less
  - because this fully leverages QIcon/KIconThemes we get full advantage of SVG 
coloring. i.e. when using a dark theme the icon is correctly using a 
contrasting color
  - users of KBIW no longer need to explicitly use KIconLoader to resolve a 
pixmap path

TEST PLAN
  widget works.
  not sure an autotest is worth here, there's not much to assert.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D22375

AFFECTED FILES
  src/CMakeLists.txt
  src/kbusyindicatorwidget.cpp
  src/kbusyindicatorwidget.h
  tests/CMakeLists.txt
  tests/kbusyindicatorwidgettest.cpp

To: sitter, cfeck
Cc: kde-frameworks-devel, apol, LeGast00n, michaelh, ngraham, bruns