Synapse

Merge lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin into lp://staging/synapse-project

mpc-plugin
Merge into trunk

Proposed by Bidossessi Sodonon on 2011-05-25

Status:	Needs review
Proposed branch:	lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin
Merge into:	lp://staging/synapse-project
Diff against target:	667 lines (+641/-0) 3 files modified src/plugins/Makefile.am (+1/-0) src/plugins/mpc-plugin.vala (+639/-0) src/ui/synapse-main.vala (+1/-0)
To merge this branch:	bzr merge lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Bidossessi Sodonon (community)		Needs Resubmitting on 2011-05-31
Alberto Aldegheri		Needs Fixing on 2011-05-30
Michal Hruby	2011-05-25	Needs Fixing on 2011-05-27
Review via email: mp+62308@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-05-25.

Description of the change

Changed MpcEngine's methods to static to avoid unnecessary instantiation.

Revision history for this message

Alberto Aldegheri (albyrock87) wrote on 2011-05-25: Posted in a previous version of this proposal

First of all, thank you for your contribution! :)

Some comments:
1)
MpcEngine class is instantiated each time I execute an action.
But MpcEngine doesn't have "attributes" that needs to be instantiated.
So I suggest to turn each method and attribute of MpcEnginge into a static one.
In example:
private static string bin = "mpc"; //this can be also "const"
public static void play_pause ()...

In this way you can call directly:
MpcEngine.play_pause ();

2) what's the meaning of
>>search the database for a "type" and turn results to uris on the fly<< ?

3) Synapse's configuration capabilities will come asap (mhr3 and I are very busy in this moment)

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-25: Posted in a previous version of this proposal

Thanks for your prompt reply.

About 1), blame my lack of programming experience with compiled code. I will update this asap.

Now, for 2), from man mpc:

search <type> <query> [<type> <query>]...
Searches for songs where all of the given tag <type>s match the given <query>s. Any number of tag type and query combinations can be specified. Possible tag
types are: artist, album, title, track, name, genre, date, composer, performer, comment, disc, filename, or any (to match any tag).

Here is a sample of the list returned:

[b_sodonon@sysadmin synapse-project]$ mpc search genre world
Ali Farka Toure & Toumani Diabate/In the Heart of the Moon/02. Kala.mp3
Ali Farka Toure & Toumani Diabate/In the Heart of the Moon/03. Mamadou Boutiquier.mp3

The resulting file paths are relative to mpd's "musid_dir" configuration.

what needs to happen is the following (python):

abs_path = music_dir + rel_path
uri = 'file://' + str(urllib.quote(mpd_path, safe='/~'))

I use that code to create uris I feed into zeitgeist.

I have no idea how to do that in vala.
Not to mention that, to enqueue files in mpd, I have to provide relative_path as an argument, which means going from uri to relative path. No idea how I could do that either. So until I become a bit more conversant with vala, I'll stick to the basic functionality.

Once again, thanks for the reply. It feels good be finally giving back to the community. :)

Cheers.

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-25:

I hope this is the right way to go about updating my merge proposal.
If not, I'm sorry.

Cheers

Revision history for this message

Alberto Aldegheri (albyrock87) wrote on 2011-05-25:

:) Do not resubmit the proposal (I made the same mistake the first time I used Launchpad :P).
Just add a comment an I (or mhr3) will come here and look at changes.

I'll look at your merge proposal asap!

lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin updated on 2011-05-27

443. By Bidossessi Sodonon on 2011-05-26: Forgot to add the "stop" command
444. By Bidossessi Sodonon on 2011-05-26: Forgot some strings to make Stop appear
445. By Bidossessi Sodonon on 2011-05-26: Fixed unreachable catch issues
446. By Bidossessi Sodonon on 2011-05-27: Feature-complete code, as compared to banshee-plugin

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-27:

Plugin is now feature-complete, using banshee as a feature basis.

It does the following actions:
- start, stop, pause and resume playback
- enqueue a uri in mpd
- play a track, clearing the playlist

I reinstated instantiation in prevision of Synapse's configuration capabilities.
That way, (almost) no refactoring will be needed.

I'm sure the code is not optimized, but, that's my level, for now.

Cheers

lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin updated on 2011-05-27

447. By Bidossessi Sodonon on 2011-05-27: Fixed the location for this file.
448. By Bidossessi Sodonon on 2011-05-27: Updated the pot file.

Revision history for this message

Michal Hruby (mhr3) wrote on 2011-05-27:

A few notes:

- please revert the changes to hybrid-search-plugin
- line 210-217 and others - why do you catch the error, if you only re-throw it?

Other than that it looks ok...

review: Needs Fixing

lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin updated on 2011-05-30

449. By Bidossessi Sodonon on 2011-05-28: * reverted hybrid-search-plugin to previous code
* stopped catching errors in public methods in mpc-plugin (no real need)
* updated synapse.pot to

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-30:

> A few notes:
>
> - please revert the changes to hybrid-search-plugin
> - line 210-217 and others - why do you catch the error, if you only re-throw
> it?
>
> Other than that it looks ok...
Well, I kept catching until vala stopped giving me unhandled error messages. They were kind of scary. But I admint there's no real need to do that.

I believe that with 449, this plugin is ready.

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-30:

Please check if more needs to be done for this plugin.
Thank you.

review: Needs Resubmitting

Revision history for this message

Alberto Aldegheri (albyrock87) wrote on 2011-05-30:

Please, enter in Synapse's folder, and execute:
bzr revert --revision 440 src/plugins/hybrid-search-plugin.vala

And:

bzr revert --revision 440 po/synapse.pot

(we'll update translations later)

At this point: return path[music_root.length:path.length]; (line 411 of the diff)
Are you sure that music_root is a prefix of path ?

review: Needs Fixing

lp://staging/~bidossessi-sodonon/synapse-project/mpc-plugin updated on 2011-05-31

450. By Bidossessi Sodonon on 2011-05-31: Don't try to play files that are not in MPD's music root (since it will fail anyway).
451. By Bidossessi Sodonon on 2011-05-31: Reverted pot file and hybrid-search-plugin to rev 440.

Revision history for this message

Bidossessi Sodonon (bidossessi-sodonon) wrote on 2011-05-31:

> Please, enter in Synapse's folder, and execute:
> bzr revert --revision 440 src/plugins/hybrid-search-plugin.vala
>
> And:
>
> bzr revert --revision 440 po/synapse.pot
>
> (we'll update translations later)
>
Done, and sorry about that.
> At this point: return path[music_root.length:path.length]; (line 411 of the
> diff)
> Are you sure that music_root is a prefix of path ?
Got it: files not in mpd's music_root are now handled.

review: Needs Resubmitting

Revision history for this message

Antono Vasiljev (antono) wrote on 2012-01-27:

Any updates here?

Unmerged revisions

451. By Bidossessi Sodonon on 2011-05-31: Reverted pot file and hybrid-search-plugin to rev 440.
450. By Bidossessi Sodonon on 2011-05-31: Don't try to play files that are not in MPD's music root (since it will fail anyway).
449. By Bidossessi Sodonon on 2011-05-28: * reverted hybrid-search-plugin to previous code
* stopped catching errors in public methods in mpc-plugin (no real need)
* updated synapse.pot to
448. By Bidossessi Sodonon on 2011-05-27: Updated the pot file.
447. By Bidossessi Sodonon on 2011-05-27: Fixed the location for this file.
446. By Bidossessi Sodonon on 2011-05-27: Feature-complete code, as compared to banshee-plugin
445. By Bidossessi Sodonon on 2011-05-26: Fixed unreachable catch issues
444. By Bidossessi Sodonon on 2011-05-26: Forgot some strings to make Stop appear
443. By Bidossessi Sodonon on 2011-05-26: Forgot to add the "stop" command
442. By Bidossessi Sodonon on 2011-05-25: Turned MpcEngine instance method to class methods (static)