Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin to sync metadata from other applications. #1386

Merged
merged 5 commits into from Apr 18, 2015
Merged

Conversation

pprkut
Copy link
Collaborator

@pprkut pprkut commented Mar 28, 2015

This is the initial version of a plugin to synchronize metadata with other applications (at some point maybe even bi-directional). It allows syncing from multiple sources, specified either in the config file or on the command line.
One example source is already implemented in the form of syncing data from amarok over its dbus interface.
I'm not picky on the name, if someone has better suggestions I'm all ears. I picked 'psync' (player sync) to make it a bit better distinguishable from mbsync than maybe a plain 'sync' would be.

@tomjaspers
Copy link
Collaborator

This looks very cool! I've been meaning to make something to sync iTunes metadata, but a general player syncing plugin is even better, and make that task easier.

My two comments:

  • Wouldn't it be better if sources could register their own item_types to be used by the plugin?
  • In that same line of thinking, it could be nice if the plugin could load the sources automatically, so that we plugin doesn't have to be changed when adding new sources, and avoids the if player == u'amarok': stuff.

Name-wise: more of a fan of the full PlayerSync over the psync

@brunal
Copy link
Collaborator

brunal commented Mar 30, 2015

That's really cool!

  • name-wise, I agree with @tomjaspers. You could also name it something like "metadata_sync". Explicit is good.
  • source detection & loading: @tomjaspers is right (esp. the 1st point, psync/__init__.py should know nothing about amarok fields). Sources management already happens in beetsplug/fetchart.py and beetsplyg/lyrics.py, if you want to have a look @pprkut.

@sampsyo
Copy link
Member

sampsyo commented Mar 31, 2015

Cool indeed! Both comments above cover my thoughts too.

I wonder if a general mechanism for adding the player name as a prefix would be useful. If I were using this plugin, for example, I might not want to use the prefix at all—since I'd only sync with one player, just plain old "playcount" might be enough (along the lines of mpdsync).

@pprkut pprkut force-pushed the sync branch 2 times, most recently from bf1c60e to 5083471 Compare March 31, 2015 19:22
@pprkut
Copy link
Collaborator Author

pprkut commented Mar 31, 2015

I adapted the plugin loading to be a bit more dynamic now. Alas, the item_types are still in __init__.py, mostly because I don't know how forwarding that to the lower level classes would work and after a brief chat with @sampsyo on IRC we agreed that having them in __init__.py for now is good enough.
I'm not a big fan of rather long names as I don't really want to type that much ;) Maybe something like 'metasync' could work?
And about the app prefix, I don't mind any modifications and improvements happening in that space after the plugin is in. I do think that for now it's out of scope. Data should be stored in the database with the prefix anyway (unless we establish clear specifications on how playcount/rating/etc should be handled, that all plugins need to adhere to. But that would still be out of scope for this plugin IMHO)

@sampsyo
Copy link
Member

sampsyo commented Apr 6, 2015

If this is already useful for Amarok, should we merge this now and keep evolving it in tree? I too would lean toward playersync as a name, but that's purely cosmetic.

@pprkut
Copy link
Collaborator Author

pprkut commented Apr 6, 2015

No objections from my side :)
Anything left to change before you want to merge it?

@sampsyo
Copy link
Member

sampsyo commented Apr 17, 2015

Hey, I forgot to ask! Could you please write the documentation for the initial version? If this doesn't seem user-facing yet, we could of course keep it in a branch for a while until it seems documentation-worthy.

@pprkut pprkut force-pushed the sync branch 3 times, most recently from 07527ec to 4c65981 Compare April 18, 2015 12:13
@pprkut
Copy link
Collaborator Author

pprkut commented Apr 18, 2015

I think it's perfectly fine to be used by users already. I don't think any of the architectural changes left for discussion would have any notable impact on what's presented to the user at this point.
I added some documentation, please have a look. I tried to be thorough, but I might've missed something :)

@@ -104,6 +105,7 @@ Metadata
* :doc:`lastimport`: Collect play counts from Last.fm.
* :doc:`lyrics`: Automatically fetch song lyrics.
* :doc:`mbsync`: Fetch updated metadata from MusicBrainz
* :doc:`metasync`: Fetch metadata from local or remore sources
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: 'remore'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed

@sampsyo
Copy link
Member

sampsyo commented Apr 18, 2015

Looking great. Thank you for adding documentation.

I'm going to merge this now and describe it as "experimental" in the changelog—just because I imagine the way it works might evolve as we add more player backends.

@sampsyo
Copy link
Member

sampsyo commented Apr 18, 2015

Apropos of nothing, maybe this would be a good home for what currently lives in the lastimport plugin.

@sampsyo sampsyo merged commit 0ecc560 into beetbox:master Apr 18, 2015
sampsyo added a commit that referenced this pull request Apr 18, 2015
Plugin to sync metadata from other applications.
sampsyo added a commit that referenced this pull request Apr 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants