plasma volume: global shortcuts and osd

[Harald Sitter]

On Fri, Jul 24, 2015 at 6:45 PM, Mark Gaiser <markg85 at gmail.com> wrote:
> Oke, here is the review as promised.

Thanks. much appreciated.

> Disclaimer: i removed kmix after installing this one. Just to be sure. I'm
> on a fully updated archlinux setup. Nothing runs from git besides this
> applet.
>
> First the functional review (checking functionality, looking for weirdness,
> that kind of stuff).
> 1. Mute key doesn't work! It can be fixed though, but when fixing it i found
> two other issues. First how i fixed it. When you go to the global keyboard
> shortcut settings you can simple select this new volume applet -> mute and
> set the mute key you want. That's the fix.
>
> 1.1 The first bug here is that you have two - yes two - "Mute" entries in
> the "Audio Volume" component. Setting the mute key in one of them works, the
> other entry seems to be ignored. Here is a screenshot of that [1]

^ I think these two are related and indicative of kglobalaccel getting
confused about something (possibly kmix vs. plasma volume?). Alas, I
fail to replicate this right now.  A broken kglobalaccelrc would
probably be helpful to track down the problem. As I understand it
Aleix has this problem as well so maybe he'll let me look at his once
I find him ;)

> 1.2 I wanted to look at the keyboard shortcuts in the applet itself. That..
> err... crashes plasma. I can open the applet settings (which are also empty
> btw), but can't click on the keyboard shortcuts. It just doesn't open
> anything. When closing the settings afterwards, plasma crashes.

Given the disclaimer you were probably using Plasma 5.3 and the
settings bit actually makes plasma crash in 5.3 because it is missing
a feature. I have seen it happen on another 5.3 system and Marco
hunted it down to the fact that a required workspace change happened
after 5.3 so workspace doesn't know what to do with the load request
for the kcm. Works on 5.4 though. So yeah, the applet settings are
known to be incompatible.

> 2. Volume up/down gives a nice short "popping" sound on kmix, it doesn't
> with this applet. Could you add that please?

Absolutely.

> 3. Pressing mute always shows me an OSD with the content: "Audio muted" even
> if i unmute. It does toggle the mute just fine, just the OSD content isn't
> what i would expect. Additional, unmuting should also give that nice short
> popping sound.

Fixed the OSD. sound as per above.

> 4. The "Audio Volume" settings KCM defaults to the Applications tab. If no
> application plays audio then that tab is completely empty! And since the
> tabs are on top, that gives the KCM a weird empty look.

"Nothing to show" label or something is on my todo.

> 5. The audio bars are too wide for 100% [2] The bars in the KCM don't suffer
> from this.

At a glance the only impactful difference between the two is that the
KCM uses a QtQuickControl slider where the applet uses a
PlasmaComponent one. So best guess I have to offer right now is that
this is a style problem. On my todo for more investigation.

> 6. The icon is... ugly. It's basically all black! I think you should just
> replace kmix and use that icon.

That is the breeze KMix icon :/
It is very black-boxy though.

> Next up the code review. This will only be a compile and style review, not
> code correctness.
>
> Namespace issue. You use the "QPulseaudio" namespace. Q<class> is - by good
> habit - reserved for Qt itself when it adds new classes. You don't have to
> follow that guideline, but it would be appreciated if you do. If you did
> actually make a Qt pulseaudio module (which this isn't either) then a name
> like "QtPulseaudio" (note the Qt, not Q) would have been appropriate. It
> would be best if you took another name for this one.

Probably should actually be reflective of the library name anyway. So
yes... On todo.

> The following are more general code notes since i didn't found the issue in
> one particular file, but spread throughout the code.
> 1. Please don't add implementation code in headers. You did that in
> volumnobject.h, maps.h, port.h, context.h, card.h, and some more. It's a
> good habit to keep the headers for definitions only and implementations in
> source files.
> 2. You're using qDebug and the other q<LogType> classes throughout the code.
> Please consider using qCDebug, qCWarning, qC... You would allow a tool like
> kdebugdialog to disable/enable the logging of specific subjects.
> 3. You're coding style is nearly OK conforming the frameworks coding style,
> but it would be best to also run AStyle over all the cpp/h files. You can
> find that here [3]. A think i keep noticing is an if statement with one line
> and no curly braces. The Qt internal coding style might allow that, the
> frameworks coding style wants braces even if you have just one line.
> 4. A compile spawns quite some warning (which you defined with #warning).
> Could you please fix those? They are in card.cpp, context.cpp and
> volumeobject.h

^ All on todo

Thanks again <3

HS