Merge lp:~greg-johnston/do-plugins/quodlibet into lp:do-plugins

Proposed by Greg Johnston
Status: Rejected
Rejected by: Alex Launi
Proposed branch: lp:~greg-johnston/do-plugins/quodlibet
Merge into: lp:do-plugins
To merge this branch: bzr merge lp:~greg-johnston/do-plugins/quodlibet
Reviewer Review Type Date Requested Status
Alex Launi (community) Disapprove
Ian Warford (community) functionality Needs Fixing
Review via email: mp+2593@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Greg Johnston (greg-johnston) wrote :

Added a fairly simple plugin to execute Quod Libet queries from Gnome-Do. This should have no problem building, though it does require Quod Libet to be installed to run properly (obviously).

60. By Greg Johnston <gjohnston@gjohnston-desktop>

Fixed: will launch Quod Libet, then execute query, if QL is not already started

Revision history for this message
Ian Warford (iwarford) wrote :

This looks fine in terms of functionality.

I think that the action Name should be called something like "Query Quod Libet", or "Search Quod Libet", or something along those lines to distinguish it from the program start option.

The plugin name and description in the addin.xml should also probably be rephrased so it contains the word "Simple", or another kind of phrasing, that highlights that it only forwards a search query to QL. This is just expectation management -- there are two other music player plugins in Do that index music libraries, and it needs to be highlighted that this isn't that.

There's only really one functionality issue in the code that I see. You need to call quodlibet --set-browser=SearchBar for the query command to do what you expect. If the user doesn't have the search library window open as the main window, then the --query remote control doesn't do anything. (I'm running a ppa version of quodlibet 2.0, so this may have been different in 1.0, I didn't check).

You're also going to want to test it with some hairy boolean search queries to make sure that they aren't going to get interpreted by the shell (if there is a shell involved or not -- Mono docs are lacking). I know you can force it to not use the system shell if need be -- it's worth checking out if the |'s and &'s in the queries are going to make that necessary or not.

review: Needs Fixing (functionality)
61. By Greg Johnston <gjohnston@gjohnston-desktop>

Clarified the description, made sure we'll force Search view

Revision history for this message
Ian Warford (iwarford) wrote :

Still has a few problems:

Queries that have spaces don't work. i.e. album=/Secrets from the future/c this will result in the query "album=/Secrets" being sent to ql.

System.Diagnostics.Process.GetProcessesByName ("quodlibet"); doesn't seem to be working on my box. Using pidof quodlibet or killall quoat the command line doesn't work for me either, so it's not just mono doing it. This might be a side effect of using 2.0. But every time I run a query it waits five seconds and attempts to restart it.

review: Needs Fixing (functionality)
Revision history for this message
Ian Warford (iwarford) wrote :

Upon some further testing, I'd recommend the following series of commands.

quodlibet --pause
quodlibet --set-browser=SearchBar --query "album = /final boss/"
quodlibet --next

That should cause it to start playing the new query results immediately.

Revision history for this message
Alex Launi (alexlauni) wrote :

This plugin has suffered severe bitrot, it's no longer compatible with the current api.

review: Disapprove

Unmerged revisions

61. By Greg Johnston <gjohnston@gjohnston-desktop>

Clarified the description, made sure we'll force Search view

60. By Greg Johnston <gjohnston@gjohnston-desktop>

Fixed: will launch Quod Libet, then execute query, if QL is not already started

59. By Greg Johnston <gjohnston@gjohnston-desktop>

Actually added Quod Libet plugin

58. By Greg Johnston <gjohnston@gjohnston-desktop>

Added Quod Libet plugin.

57. By Jason Smith

Fix translate plugin

56. By Jason Smith

Merge in community-future updates (thanks to iwarford)

55. By David Siegel <david@dell-desktop>

Peng's RTM and Ping.FM plugins build with little fuss. Merging.

54. By David Siegel <david@dell-desktop>

SHould not be versioned.

53. By David Siegel <david@dell-desktop>

Transition to updated plugin interfaces.
Also format manifest files.

52. By Alex Launi

merged vbox changes

Subscribers

People subscribed via source and target branches