new class KBusyIndicatorWidget similar to QtQuick's BusyIndicator
ClosedPublic

Authored by sitter on Jul 10 2019, 1:08 PM.

Details

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.

Diff Detail

Repository
R236 KWidgetsAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13883
Build 13901: arc lint + arc unit
sitter created this revision.Jul 10 2019, 1:08 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 10 2019, 1:08 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Jul 10 2019, 1:08 PM

Cooooool!

src/kbusyindicatorwidget.cpp
29

Q_DECL_HIDDEN

32

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

39

Plasma's BusyIndicator uses 1500ms

63

Is that legal in the C++ standard allowed by frameworks?

104

is q->isVisible() checked in this method already updated at this point or should the hideEvent be processed before? Likewise for showEvent

112

Does this widget need a sizeHint of a button or default icon size or something?

src/kbusyindicatorwidget.h
52

override instead

66

override is enough

66

I heard for good measure one should always re-implement the generic event just in case

sitter added inline comments.Jul 10 2019, 1:44 PM
src/kbusyindicatorwidget.cpp
112

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.

broulik added inline comments.Jul 10 2019, 1:47 PM
src/kbusyindicatorwidget.cpp
112

How about PM_LargeIconSize

cfeck added a comment.Jul 10 2019, 1:48 PM

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.

cfeck added inline comments.Jul 10 2019, 1:51 PM
src/kbusyindicatorwidget.h
49

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

sitter added inline comments.Jul 10 2019, 2:05 PM
src/kbusyindicatorwidget.cpp
112

Oh, that would work!

src/kbusyindicatorwidget.h
49

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.

cfeck added inline comments.Jul 10 2019, 3:02 PM
src/kbusyindicatorwidget.h
49

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

73

*const d

sitter updated this revision to Diff 61522.Jul 10 2019, 3:57 PM
sitter marked 14 inline comments as done.
  • 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?

kossebau added inline comments.
src/kbusyindicatorwidget.h
41

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

58–59

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<class KBusyIndicatorWidgetPrivate> const d;

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

tests/kbusyindicatorwidgettest.cpp
32

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

sitter added inline comments.Jul 11 2019, 3:30 PM
src/kbusyindicatorwidget.cpp
63

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

src/kbusyindicatorwidget.h
41

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.

66

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

sitter updated this revision to Diff 61603.Jul 11 2019, 3:32 PM

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

apol accepted this revision.Jul 11 2019, 6:41 PM

LGTM

This revision is now accepted and ready to land.Jul 11 2019, 6:41 PM
sitter updated this revision to Diff 61659.Jul 12 2019, 2:07 PM
  • add simple example code + image
  • add @since tag
  • override event
cfeck added inline comments.Jul 12 2019, 3:03 PM
src/kbusyindicatorwidget.h
52

5.61.0

tests/kbusyindicatorwidgettest.cpp
43

typo

sitter updated this revision to Diff 61789.Jul 15 2019, 1:53 PM

typos--

cfeck accepted this revision.Jul 15 2019, 2:19 PM
This revision was automatically updated to reflect the committed changes.
rjvbb added a subscriber: rjvbb.Oct 3 2019, 8:17 AM

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

cfeck added a comment.Oct 3 2019, 9:51 AM

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

rjvbb added a comment.Oct 3 2019, 10:58 AM
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.

The demo doesn't even use this widget

breeze

oxygen

rjvbb added a comment.Oct 3 2019, 1:20 PM

The demo doesn't even use this widget

That wasn't the question I answered by referring to oxygen-demo

breeze

oxygen

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:

I'd do the same snapshot for kbusyindicatorwidgettest but I'd have to recompile KWidgetsAddons without my tweak first :)

sitter added a comment.Oct 3 2019, 1:41 PM

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.

rjvbb added a comment.Oct 3 2019, 7:01 PM

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.

rjvbb added a comment.Oct 4 2019, 1:20 PM

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.