<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 22, 2015 at 11:10 AM, Harald Sitter <span dir="ltr"><<a href="mailto:sitter@kde.org" target="_blank">sitter@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">random update before akademy.<br>
<br>
d_ed asked me to perhaps land the volume thingy for 5.4 so to make<br>
that happen the applet UI is going to stay minimal and the KCM UI is<br>
going to stay a clone of the pavucontrol UI (which isn't the greatest<br>
of things but functionally pretty complete).<br>
both UIs should be more or less complete in master of<br>
scratch/sitter/plasma-volume-control. I encourage testing.<br>
<br>
also I added global shortcuts and OSD support which should be the two<br>
major features that were missing. also kindly note that no measures<br>
are being taken to replace kmix right now so with kmix running you'll<br>
get two OSDs, other than that everything should be fine.<br>
<br>
HS<br><br></blockquote><div><br></div><div>Oke, here is the review as promised.</div><div><br></div><div>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.</div><div><br></div><div>First the functional review (checking functionality, looking for weirdness, that kind of stuff).</div><div>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.</div><div>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]</div><div>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.</div><div>2. Volume up/down gives a nice short "popping" sound on kmix, it doesn't with this applet. Could you add that please?</div><div>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.</div><div>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.</div><div>5. The audio bars are too wide for 100% [2] The bars in the KCM don't suffer from this.</div><div>6. The icon is... ugly. It's basically all black! I think you should just replace kmix and use that icon.</div><div><br></div><div>Next up the code review. This will only be a compile and style review, not code correctness.</div><div><br></div><div>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.</div><div><br></div><div>The following are more general code notes since i didn't found the issue in one particular file, but spread throughout the code.</div><div>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.</div><div>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.</div><div>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.</div><div>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</div><div><br></div><div>That it for the review.</div><div>I wasn't too hard on you i hope? :) It definitely is meant as constructive feedback to improve the applet. I have it running now and will keep running it for the coming days. If i find anything else i will report back in this thread.</div><div>You did a great job so far already, keep up the good work.</div><div><br></div><div>[1] <a href="http://imgur.com/fKRXhcA">http://imgur.com/fKRXhcA</a> (ignore the semi opaque kscreengenie window on the right part of the screenshot..)</div></div>[2] <a href="http://imgur.com/SGwgsQc">http://imgur.com/SGwgsQc</a> (note the 100% and you still see a little bit of "bar" behind the knob)</div><div class="gmail_extra">[3] <a href="https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Artistic_Style_.28astyle.29_automatic_code_formatting">https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Artistic_Style_.28astyle.29_automatic_code_formatting</a></div></div>