Review Request #118311 - Created May 25, 2014 and submitted June 5, 2014, 12:17 p.m.
Added a command plug-in to control MPRIS enabled media players. For now, I have only handled the basic calls like play, pause, next etc. But I will extend it soon.
Tested with Plasma Media Center, Amarok, VLC.
No, this can't work. As soon as the application get's translated, those strings will not be the same. Instead, you ... Peter Grasch June 2, 2014, 12:10 p.m.
? Peter Grasch June 2, 2014, 9:39 a.m.
Put the type information (Play / Pause / etc) here. Peter Grasch June 2, 2014, 12:10 p.m.
Why a dynamic cast? Peter Grasch June 2, 2014, 10:14 a.m.
Might wanna move the definition of that constant to a more prominent place, maybe? Peter Grasch June 2, 2014, 9:57 a.m.
Stuff like this should go in deserializeConfig(). Creating a command manager should be as cheap as possible, as this will ... Peter Grasch June 2, 2014, 9:59 a.m.
Dynamic casts? Peter Grasch June 2, 2014, 10:13 a.m.
This seems off. If the service doesn't start with MprisPlayerPrefix, why should it be in the list? Even if it ... Peter Grasch June 2, 2014, 10:04 a.m.
Please use C++ style casts Peter Grasch June 2, 2014, 1:24 p.m.
Why? If the user had set it to only listen to a specific service, then tries out "listen to all" ... Peter Grasch June 2, 2014, 12:53 p.m.
Controlling all media players should be the default Peter Grasch June 2, 2014, 1:39 p.m.
Yeah, I already mentioned this in the "user" review, but these values are too large. (In VLC a volume change ... Peter Grasch June 3, 2014, 7:46 a.m.
You need to make those translatable. Please read the following links: http://techbase.kde.org/Development/Tutorials/Localization/i18n http://techbase.kde.org/Development/Tutorials/Localization/i18n_Mistakes They might seem a bit long, but ... Peter Grasch June 3, 2014, 7:49 a.m.
I suggest that you use i18nc and provide a comment for these, because the phrases are very short and could ... Peter Grasch June 4, 2014, 1:47 p.m.
Nah, that can't be the best way. Tried clear()-ing the items before adding them? Peter Grasch June 4, 2014, 1:55 p.m.
May 28, 2014, 8:56 a.m.
1. Design: I don't think that e.g. the creator widget should need to know DBus paths like "/org/mpris/MediaPlayer2". I think that the whole UI and everything around the plugin should stay on the high level "Play", "Pause", etc. labels (an enum would be nice) and this should only be translated to the necessary dbus call in the plugin and only when it gets invoked. That way, you'd also store (serialize) the *intention* of the user and not the actual implementation, which is always a better choice (theoretically: what if you need to send slightly different commands to $brokenPlayerX?).
2. This is related to 1 but please don't show the details of the dbus calls to the user (getValueMapPrivate).
3. Your trigger method has code to handle arguments. This is clearly code you copied from the dbus plugin (which is fine), but it really doesn't belong here anymore.
5. About the configuration: Requiring to enter the DBus Interface will be a no-go for > 99 % of users. How about this: Make the plugin talk to all registered services at org.mpris.MediaPlayer2.*, but provide a UI element to limit that to a specific player. Instead of a line edit you should use an editable combobox that is prefilled with all running org.mpris.MediaPlayer2.* interfaces (editable because the application the user wants to control may not be running). I made a mockup, I'll upload a screenshot.
May 28, 2014, 9:01 a.m.
May 31, 2014, 8:54 a.m.
Fixed the issues pointed by Peter.
June 2, 2014, 9:20 a.m.
Put the type information (Play / Pause / etc) here.
Might wanna move the definition of that constant to a more prominent place, maybe?
Stuff like this should go in deserializeConfig(). Creating a command manager should be as cheap as possible, as this will happen before the plugin is even assigned to a scenario.
If the service doesn't start with MprisPlayerPrefix, why should it be in the list? Even if it doesn't start with it and is in the list, why would we want to keep it in there?
June 2, 2014, 4:27 p.m.
June 3, 2014, 7:14 a.m.
3. Remove the "empty" entry in the list of interfaces (making the first media player, if we found one, the default).
4. The steps in "Increase / Decrease" volume are way too big. Keep in mind that it's really easy to make these steps bigger when writing the scenario by wrapping multiple invocations in a Composite command, but there is no way to make the steps smaller.
6. I think that a "Play / Pause" command would be really helpful. Could you add one, please?
7. The "Play" command does not work at all in Spotify. I tried with other MPRIS controls and although the Linux Spotify client seems to support only a subset of commands, Play / Pause is definitely among them (Pause works in your implementation). Could you take a look, please?
Other than that, the user side looks great!
June 3, 2014, 7:23 a.m.
You need to make those translatable.
They might seem a bit long, but please do take the 20 minutes to review these two pages, it'll safe you a *lot* of time in the long run.
June 3, 2014, 10:07 a.m.
3. Made the steps for Volume Increase/Decrease and Seek Back/Ahead finer.
5. Made the strings shown to user translatable.
Almost done. Very good work!
Nah, that can't be the best way. Tried clear()-ing the items before adding them?
June 4, 2014, 1:55 p.m.
June 4, 2014, 2:20 p.m.
June 4, 2014, 5:47 p.m.
This review has been submitted with commit 2e9d159a884e009a73a8657d42b6f3f3f8276989 by Ashish Madeti to branch mpris-plugin.
June 5, 2014, 8:19 a.m.
June 5, 2014, 8:20 a.m.
June 5, 2014, 8:37 a.m.
June 5, 2014, 8:39 a.m.
June 5, 2014, 12:17 p.m.