[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: plasma volume: global shortcuts and osd
From: Mark Gaiser <markg85 () gmail ! com>
Date: 2015-07-24 16:45:09
Message-ID: CAPd6JnHOu5V2VfNXxRjRgcLKVBQrG0NpGkjpkmcL=cXEirY84A () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Wed, Jul 22, 2015 at 11:10 AM, Harald Sitter <sitter@kde.org> wrote:
> random update before akademy.
>
> d_ed asked me to perhaps land the volume thingy for 5.4 so to make
> that happen the applet UI is going to stay minimal and the KCM UI is
> going to stay a clone of the pavucontrol UI (which isn't the greatest
> of things but functionally pretty complete).
> both UIs should be more or less complete in master of
> scratch/sitter/plasma-volume-control. I encourage testing.
>
> also I added global shortcuts and OSD support which should be the two
> major features that were missing. also kindly note that no measures
> are being taken to replace kmix right now so with kmix running you'll
> get two OSDs, other than that everything should be fine.
>
> HS
>
>
Oke, here is the review as promised.
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]
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.
2. Volume up/down gives a nice short "popping" sound on kmix, it doesn't
with this applet. Could you add that please?
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.
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.
5. The audio bars are too wide for 100% [2] The bars in the KCM don't
suffer from this.
6. The icon is... ugly. It's basically all black! I think you should just
replace kmix and use that icon.
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.
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
That it for the review.
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.
You did a great job so far already, keep up the good work.
[1] http://imgur.com/fKRXhcA (ignore the semi opaque kscreengenie window on
the right part of the screenshot..)
[2] http://imgur.com/SGwgsQc (note the 100% and you still see a little bit
of "bar" behind the knob)
[3]
https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Artistic_Style_.28astyle.29_automatic_code_formatting
[Attachment #5 (text/html)]
<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>
[Attachment #6 (text/plain)]
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic