<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/112208/">https://git.reviewboard.kde.org/r/112208/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 12th, 2014, 11:19 a.m. UTC, <b>Christian Esken</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For me it looks fine, with some questions:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
1) Is this completely decoupled from KMix? I do not see a "open mixer" to open the main application.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
2) Is it supposed to be integrated in KMix<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
3) In general, the question is how to progress from here. Diego, do you want KMix integration, or a standalone app? Have any ideas or plans to get it in KDE?</p></pre>
</blockquote>
<p>On August 12th, 2014, 11:42 a.m. UTC, <b>Martin Klapetek</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note that new work on QML based KMix has started, however this will be Plasma5 only --> http://apachelog.wordpress.com/2014/08/11/volume/</p></pre>
</blockquote>
<p>On August 17th, 2014, 5:42 p.m. UTC, <b>Diego Casella</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll recap everything, hopefully once for all..<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This applet is meant to show the availabe mixers (with the option to show all of them, or just the Master one), and modify/mute/unmute them. That's all.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If you need to change Master channel or in general configure KMix, you need to run the "old" KMix application. That's why, in the QML applet, I added a button named "Mixer setup": you click it, and the "old" KMix should appear (in my opinion QML applets have be minimal and simple, so everything else must be done in a regular QtWidget based application).<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Anyway back on the topic: I said "should" because at the moment KMix does not appear, complaining that:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"QDBusConnection: session D-Bus connection created before QCoreApplication. Application may misbehave."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is the reason why I mentioned, in the very first description of this review request, that KMix needs to be patched :)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
As about "how to progess from here", I've mentioned some ideas in the beginning:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">patch Plasma QML bindings to provide mouse wheel events, so I can intercept scroll events in the panel and update volume levels accordingly, providing the same functionality the current KMix tray icon does;</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">patch Plasma QML slider to provide mouse wheel support (don't know if this has been already fixed in Plasma5);</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">patch KMix itself. It should run somewhat "daemonized" and, when QML applet calls "kmix" the regular, QtWidget-based application, will pop up. Since in the sourcecode I've seen references to "kmixd", which i think it stands for "KMix daemon", this may be simple to implement: the daemon provides the low-level connection to the hardware, and this applet and KMix itself connect to the daemon (via DataEngine/DBus mechanism) to communicate with it.</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That being said:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> I've been trying to update this applet in the last couple of years, but<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
</em> since KDE4/Plasma is now in maintenance mode (aka no new featured will be added), those patches won't happen anymore;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
* and since someone is already on the process to "reinvent" the wheel;</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm kinda sad/tired of how things went so far. The applet is pretty functional (I use it on a daily basis), and it could work fine in Plasma5 too. As far as I'm concerned, at this point I don't see any reason to keep this review open. If Harald wants to use any portion of this code, I'm 100% OK with that.</p></pre>
</blockquote>
<p>On August 18th, 2014, 11:34 p.m. UTC, <b>Christian Esken</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Diego, I am not sure what went so wrong here. I am defintely also not happy how things went on. What makes me most sad is, that you have a working QML applet that just needs really minor integration work. All these minimal issues like "scrollwheel not supported" (1) should really not stop us from giving the users the option. If they must have scrollwheel or media buttons, then they can use classic KMix dock. If they want a nicer integrated applet, they can use your applet (2). It can be a simple option in KMix's configuration dialog (Dock: Classic, Plasma, Off) instaed of (Dock: On, Off).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sigh :-|</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Whatever your decision is, I can understand you Diego. If you want to integrate it would make me happy (see some technical tips below). If you do not want it or you cannot (essential missing plasma features that you do not want to add yourselves), then it is possibly better to close this review request.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">--- Technical notes -----------</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About daemonizing KMix: That exists since forever: Simply disable "dock into systray" and close window, and KMix will vanish forever. The "KMix will show its GUI at every login" behavior/issue is gone since the support of hotplugging. To show (I cannot remember that you ever mentioned problems with that before) just do a DBUS call "show" on the Window (a standard for all Qt windows). If you want to try that, please head along. If </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Side note (as you mentioned it): There is also a real daemon (technically a kded module), but KMixD daemon has no GUI, and proviees just a Mixer DBUS interface.</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(1) Some ramblings about QML/Plasma: Things seem to progress sometimes slowly or not at all in QML/Plasma. Though much has happened Plasma/QML seems to still lack features. Initially there were no sliders at all in Plasma, and I knew it would be a very rocky way to get all the features in, like to add missing stuff like "mousewheel over slider". Then I made a clear decision to keep my foot out of QML completely.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(2) I actually still do not understand if integration can now be done or not. There was this mysterious comment "it would be pretty trivial to add a check in the systemtray [...]" that sounds like somebody needs to take responsibilty and add that.</p></pre>
</blockquote>
<p>On August 19th, 2014, 8:10 a.m. UTC, <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><disclaimer>I am have not been involved with Plasma for almost a year, but I keep getting CC'd on the comments here. ;)</disclaimer></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Firstly, Christian is the maintainer of KMix. It is ultimately his decision if this goes into kmix or not. Plasma developers can and should provide feedback because that is where the experience and expertise around QML in a Plasma workspace exists, but it is ultimately Christian's decision. If the Plasma team has some wishes in how KMix goes forward, they should coordinate those through Christian. This is basic common sense and courtesy for KDE's culture of development. So if Christian is happy with it, he should be able to push this into the kmix repository, end of story.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Wheel events: they are well supported by QML2. There is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">no reason</em> that they are not made available here. The MouseArea item gets wheel events; if anything what is called for is a "acceptWheelEvents" property on the iconic version of applets and hook those up to some signals for applets to get access to. For all the discussion about it, someone could have instead put such a patch in place.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Christian: as for your ramblings, they are unecessary and innacurate. I get that you would have liked to see this go faster and as the maintainer of kmix I totally understand that. But adding complaints about another project doesn't move anything further, it just increases the chances of people deciding not to work with each other. To address the content of your concerns: there have been sliders in Plasma pre- and post-QML for just about forever. There may have been short period of time when the QML widgets were first being made that there wasn't a QML slider, but that certainly wasn't a long period. Also, adding mousewheel over slider is a trivial patch if it isn't already there. (I haven't tested the QML2 slider for this at the time of writing, so it may already be there.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"There was this mysterious comment "it would be pretty trivial to add a check in the systemtray [...]" that sounds like somebody needs to take responsibilty and add that."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let me take the "mystery" out of it: in Plasma 4 it had not yet been decided how precisely to handle this. It was a "nice to have" feature and many other things were still on the "must be done" list. In Plasma 5 there is now an implementation: http://vizzzion.org/blog/2014/02/dbus-activated-systemtray-plasmoids/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So KMix should just need to have a DBus service it registers and the kmix applet should magically appear if it exists. What isn't there, at least afaict, is a way for kmix to know if the widget has appeared (and if not, show its old KSystemTrayIcon based UI). This could be done with another dbus service on the applet side that kmix watches for, but even then ... the more fundamental question for kmix looms:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is KMix meant to be a tightly integrated part of a Plasma workspace, or is it intended to be used in any "random" desktop environment?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Personally, I think there is a lot to be gained both in terms of user experience and simplicity in development for kmix to be Plasma-focused, and I don't think there are nearly as many users of KMix outside of Plasma as there are that use KMix with Plasma (I would expect it to be a "rounding error" type number). But that's your call, Christian.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If KMix does focus on Plasma, then a QML plasmoid with DBus activation as described in Sebas' blog entry above is all that is needed and this could go into kmix tomorrow at Christian's descrection.</p></pre>
</blockquote>
<p>On August 19th, 2014, 8:34 a.m. UTC, <b>Diego Casella</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Christian and @Aaron: thanks for your replies :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This morning I've modified a little the sourcecode in order to call "qdbus org.kde.kmix /kmix/KMixWindow org.qtproject.Qt.QWidget.show", disabled the "old" KMix tray icon, and it works. It's kinda clumsy, but it works :)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Regular KMix widget gets displayed when the corresponding button is clicked, and it won't appear at login anymore. These are already great news for me :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now, I think we have some options here:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> ship this applet <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">as is</em> in KDE4, it is functioning afterall;</em> port and/extend/improve this applet to get the most out of KDE5/Plasma/QML2;</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm ok with both of these, now you guys tell me what do you think.</p></pre>
</blockquote>
<p>On August 19th, 2014, 8:40 a.m. UTC, <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could it have a standalone, maybe extragear release for KDE4?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">then focus can be shifted to the plasma5 port</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Aaron, thanks for your comments and clear words. Please be assured my comment was not intended as a complaint. I sincerly believe that if one requires a new feature, it is ones own responsibility to take action. No one else too blame than oneself. I just wanted to explain, why I decided not to implement a QML applet myself. The text - as an immediate reaction on the former comment - reads very bad, though. It was not meant like that, and I do apologize.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the practical side, I'd like to answer the question: "Is KMix meant to be a tightly integrated part of a Plasma workspace, or is it intended to be used in any "random" desktop environment?": Well, optimal KDE integration will always get extra attention. But giving up cross-desktop functionality (Tray-protocol, MPRIS, DnD, ...) would be a step in the wrong direction. Thus, full functionality in KDE, and as much as possible under other desktops (XFCE, ...). A solution that is not cross-desktop compatible can be included, but not as default.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I now read the blog article http://vizzzion.org/blog/2014/02/dbus-activated-systemtray-plasmoids/ . Sounds rather interesting, especially how Plasma widgets can be managed, for showing, positioning and lifetime management. I am not sure how to apply this exactly to KMix, as the tray icon should typically always be shown (e.g. controlling the event volume) and KMix itself must also run all the time (e.g. for volume global shortcuts).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Diego: Lets start promoting it in KDE4. If you put it in Extragear, I can add an option for it in KMix (as kind of "sneak preview"). If the applet is not installed, installation instructions can be shown instead. Then lets collect feedback from users, and head for full integration in KDE5/Plasma/QML2.</p></pre>
<br />
<p>- Christian</p>
<br />
<p>On May 4th, 2014, 9:46 p.m. UTC, Diego Casella wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, Aaron J. Seigo, Christian Esken, Marco Martin, and Igor Poboiko.</div>
<div>By Diego Casella.</div>
<p style="color: grey;"><i>Updated May 4, 2014, 9:46 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kmix
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KMix qml applet.
As you can see from the screenshot, the applet is pretty much functional: you can display all the controls available, change its orientation, and decide to whether show all of them or just the Master Control, and refresh its status when new controls are added/removed/updated (such as Amarok current playing track). See screenshots below :)
Differences from the old kmix tray:
* no media player controls ( I never investigated how to get them, but honestly opening the audio applet to change/skip/pause audio track makes little sense to me ... if anyone wants this feature back, don't be shy and step in);
* the button used to select which Mixers are visible has been changed to open Phonon kcm page: since visible mixers are already configurable from KMix app, having a button to show KMix *and* a button to modify Mixers visibilty made little sense here too, so I preferred to give more visibility to Phonon kcm;
Known issues:
* there is still no way to get notified of mouse wheel events over the popupIcon, so it is not possible to scroll over to increase/decrease the master control volume;
* no scroll events over the sliders too;
* if you want to use the applet you most likely will disable KMix tray icon but, if you do so, KMix will show its GUI at every login and you have to close it manually. This requires KMix to be patched. Furthermore, if you click "KMix Setup" button, KMix window will not restored anymore: this needs to be pathed as well.
* resize doesn't work properly.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested against master and works fine.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/kmix-applet-qml/contents/ui/HorizontalControl.qml <span style="color: grey">(7e87c8e)</span></li>
<li>plasma/kmix-applet-qml/contents/ui/HorizontalMixerListDelegate.qml <span style="color: grey">(26f2968)</span></li>
<li>plasma/kmix-applet-qml/contents/ui/MixersList.qml <span style="color: grey">(66bda73)</span></li>
<li>plasma/kmix-applet-qml/contents/ui/VerticalControl.qml <span style="color: grey">(1702be7)</span></li>
<li>plasma/kmix-applet-qml/contents/ui/VerticalMixerListDelegate.qml <span style="color: grey">(bab9ac6)</span></li>
<li>plasma/kmix-applet-qml/contents/ui/kmixapplet.qml <span style="color: grey">(eecb91c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/112208/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet.png">Default look</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet1.png">Menu Actions</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet2.png">Applet Config Options</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/22/kmix_applet3.png">Vertical Control</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/24/kmix_applet5.png">ToolButton label and Config page after updates</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/08/27/kmix_applet6.png">Control Icon and Label left aligned</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/04/9d6b0ca4-5a75-45cc-ab8e-61b13d4079e6__kmix_horizontal_new.png">Kmix, horizontal view</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/04/7efb308a-c306-47c2-883f-64d1f32db5b5__kmix_vertical_new.png">Kmix applet, vertical view</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>