GNOME Bugzilla – Bug 643129
u1ms links do not work with Ubuntu One Music Store extension
Last modified: 2011-02-28 18:17:01 UTC
Created attachment 181755 [details] [review] Patch to handle u1ms links Music store links do not currently work with the Ubuntu One Music Store. The attached patch adds a service to handle these links to the UbuntuOneMusicStore extension, and also enables that extension by default when it is built.
Do we want the extension enabled by default? My understanding was that upstream Banshee would have Amazon MP3 Store enabled by default and Ubuntu's Banshee would have Ubuntu One Music Store enabled by default, and that both distributions of Banshee would include both extensions. Unless I misunderstand what you mean by enabled? Banshee want's to ship with U1MS, but not ship with it enabled. If you fix that trivial detail then I'm sure a developer will review and commit a second patch, thanks :-)
Review of attachment 181755 [details] [review]: Looks fine, just a couple of style nitpicks. In terms of runtime-enabling it, I think it's probably fine since it's not built by default. ::: src/Extensions/Banshee.UbuntuOneMusicStore/Banshee.UbuntuOneMusicStore/UbuntuOneMusicStoreService.cs @@ +47,3 @@ + ServiceManager.Get<DBusCommandService> ().ArgumentPushed += OnCommandLineArgument; + + Log.Debug ("U1MS: Started service."); This logging should be removed; it should already be logged by the ServiceManager. @@ +90,3 @@ + + private void OnCommandLineArgument (string uri, object value, + bool isFile) this can go on previous line; we wrap at 120 chars @@ +99,3 @@ + // Handle u1ms:// URIs + if (uri.StartsWith ("u1ms://") && ServiceStartup ()) { + uoms_source.LoadURI (uri); Should be LoadUri ::: src/Extensions/Banshee.UbuntuOneMusicStore/Makefile.am @@ +6,3 @@ SOURCES = \ Banshee.UbuntuOneMusicStore/Tests/MusicStoreTests.cs \ + Banshee.UbuntuOneMusicStore/UbuntuOneMusicStoreService.cs \ also needs to be added to the .csproj
Go ahead an commit it w/ those changes. If the other maintainers disagree about the enabled bit, we can change it back later.
For the record, I'm fine with the U1MS extension being enabled by default in the .addin.xml. I think it was not the case when it was part of the community extensions only because they all are disabled by default. As for the patch, wouldn't it be easier to just add the OnCommandLineArgument event handler to the UbuntuOneMusicStoreSource class ? Unless I'm missing something, I don't think we have to create a UbuntuOneMusicStoreService class just for that.
Bertrand, I suppose we could put it in the Source class. It wasn't clear to me that was the right way to do it, from reading the EMusic and LastFM Streaming extensions, which I was referencing for writing this. If it's better for it to be in the Source class instead, I can move it I guess. It's not really clear to me what the differences between Source and Service are or should be.
Created attachment 181938 [details] [review] Updated patch to do everything in Source
Comment on attachment 181938 [details] [review] Updated patch to do everything in Source Change the addin xml True to true; besides that, looks fine
What does accepted-commit_now mean? It sounds like "committed the patch", but this bug is still unresolved, and Comment #7 sounds more like a "needs_work".
Matt, "accepted-commit_now" is used to indicate that a patch has been reviewed and can be committed. In this case a small change is needed, but there's no need for another review afterwards. This used when the person submitting the patch has the commit rights on the git repo, but we still require a review before accepting the patch. When Rodney commits his patch, he should update the bug status.
Comment on attachment 181938 [details] [review] Updated patch to do everything in Source Committed with case change in xml.