GNOME Bugzilla – Bug 728621
Add LastFM support
Last modified: 2015-09-14 17:12:20 UTC
Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
Created attachment 274769 [details] [review] Add LastFM support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
We are adding LastFM support to GNOME Music[0], and I find it better to have authentication in Goa's side. This patch adds LastFM support. Some topics we should discuss are: * we need icons. * "scrobble" is the act of telling LastFM that you're listening to a given song. I find it a horrible term, so maybe we could change it. * The API account is tied to my personal LastFM account. We could transfer it to a official GNOME account if you will. * I'm aware that you (Debarshi) don't want to have providers which aren't being used by apps. In doing so, you could wait until I have a LastFM scrobbler written in GNOME Music (which is the next thing I'm going to do). * This patch does not includes a test example: Should it? Thanks, Felipe. [0] https://bugzilla.gnome.org/show_bug.cgi?id=705070
(In reply to comment #2) Thanks for the patch Felipe. I have not tried it out yet, but it looks good. > This patch adds LastFM support. Some topics we should discuss are: > * we need icons. > * "scrobble" is the act of telling LastFM that you're listening to a given > song. I find it a horrible term, so maybe we could change it. Can't the user-facing string be "Music" in the Online Accounts panel? > * The API account is tied to my personal LastFM account. We could transfer it > to a official GNOME account if you will. Is it possible to have co-owners for the API account? Or is it tied to a single LastFM account? > * I'm aware that you (Debarshi) don't want to have providers which aren't > being used by apps. In doing so, you could wait until I have a LastFM scrobbler > written in GNOME Music (which is the next thing I'm going to do). First, every everybody (gnome-music maintainers & designers) needs to agree in principle that we will support this. And then the application (gnome-music in this case) should be ready in time for the next stable release (3.14.0?). Assuming that everybody agrees to have this, we can always commit the g-o-a patch while the application is being worked on. If it is not ready in time for the release, we can revert it and try again next cycle. > * This patch does not includes a test example: Should it? It is not mandatory, feel free to add one if you want. More important is to have some pointers about the provider in README. eg., a URL to the provider's OAuth2 documentation, scopes, etc.. Makes life easier when something breaks due to server-side changes.
(In reply to comment #3) > Can't the user-facing string be "Music" in the Online Accounts panel? Not sure about the status, but Last.FM can have a radio for some subscribers. So "Scrobbling" and "Radio" are the available options. For now we can settle for music, as we still don't have a grilo plugin for last.fm radio - and its status is unclear > > > * The API account is tied to my personal LastFM account. We could transfer it > > to a official GNOME account if you will. > > Is it possible to have co-owners for the API account? Or is it tied to a single > LastFM account? Its tied to the account and apparently there is no way to add a co-owner. > > > * I'm aware that you (Debarshi) don't want to have providers which aren't > > being used by apps. In doing so, you could wait until I have a LastFM scrobbler > > written in GNOME Music (which is the next thing I'm going to do). > > First, every everybody (gnome-music maintainers & designers) needs to agree in > principle that we will support this. And then the application (gnome-music in > this case) should be ready in time for the next stable release (3.14.0?). Agree, this feature will get a priority. I'm gonna try out the patch locally soon and propose a patch to gnome-music
(In reply to comment #4) > (In reply to comment #3) > > Can't the user-facing string be "Music" in the Online Accounts panel? > Not sure about the status, but Last.FM can have a radio for some subscribers. > So "Scrobbling" and "Radio" are the available options. I've already told you, and so did Emmanuele, the radios are closing down. > For now we can settle for music, as we still don't have a grilo plugin for > last.fm radio - and its status is unclear Their status is very clear. They're closed: http://www.last.fm/forum/21717/_/2226535 "From the 28th of April 2014, our subscription radio streaming service will no longer be available." Unless you want to write code that'll work for the next 6 days.
Review of attachment 274769 [details] [review]: Thanks for the patch Felipe. I have not tried it out, but it looks good. ::: data/dbus-interfaces.xml @@ +224,3 @@ + <!-- ScrobbleDisabled: + @since: 3.12.1 This should be MusicDisabled, and the @since should be updated to 3.14.0. Generally, in GOA, the names of the interfaces and the ON|OFF switches map to the names of the core applications. That way we have a clear association between the account, the data and the application. In this case the application is gnome-music. @@ +717,3 @@ <!-- + org.gnome.OnlineAccounts.Scrobble: + @since: 3.12.1 Ditto. Should be Music and the @since needs an update. ::: src/goabackend/goabackendenums.h @@ +90,3 @@ GOA_PROVIDER_FEATURE_READ_LATER= 1 << 10, GOA_PROVIDER_FEATURE_PRINTERS = 1 << 11, + GOA_PROVIDER_FEATURE_SCROBBLE = 1 << 12, Should be MUSIC. ::: src/goabackend/goalastfmprovider.c @@ +77,3 @@ + GoaObject *object) +{ + return g_strdup (_("LastFM")); Shouldn't it be "Last.fm"? @@ +89,3 @@ +get_provider_features (GoaProvider *_provider) +{ + return GOA_PROVIDER_FEATURE_BRANDED; It should also have GOA_PROVIDER_FEATURE_MUSIC.
Review of attachment 274769 [details] [review]: ::: data/dbus-interfaces.xml @@ +233,3 @@ + removed from the account asynchronously. + --> + <property name="ScrobbleDisabled" type="b" access="readwrite"/> We need to add some glue so that the documentation for this new DBus property and interface show up in devhelp. Look at doc/goa-docs.xml and doc/goa-sections.txt, and the recent commit that added org.gnome.OnlineAccounts.Maps.
Review of attachment 274769 [details] [review]: Last.FM's API documentation mentions 3 different flows: http://www.last.fm/api/authentication Why are we using the authentication path for web applications? I think the mobile authentication path (http://www.last.fm/api/mobileauth) would be better for us. Unlike the web or desktop options, it does not require sending the user to a last.fm web page. This is good because the last.fm web UI is too bulky for the small embedded web view that we have. I can't find a documented way to force a smaller version of the page. Secondly, dealing with a web UI that is out of our control is problematic. Look at all the DOM and cookie sniffing hacks that we have to populate the username in the web UI when the user is refreshing the tokens, or to hide our "cancel" button when the web UI already has one, etc.. These hacks are fragile, at best, and can break when the web UI changes. Even if we forget about these things, the web UIs have found innovative ways to break. See bug 726609 for an example. We can avoid this whole mess by using the mobile authentication path. As far as I can see, all we need is our own GtkDialog to input the username and password and do some REST calls to get the tokens. Somewhat similar to the way we handle ownCloud, email and Exchange.
Created attachment 306788 [details] [review] add Lastfm support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
Since someone has proposed a Last.fm scrobbler for Music in bug 742336, I've reworked this patch for it. It has a straightforward example you can test by running: ./lastfm-shout felipe10borges gnome And see the result at http://www.lastfm.com.br/user/felipe10borges/shoutbox Cheers!
(In reply to Felipe Borges from comment #9) > Created attachment 306788 [details] [review] [review] > add Lastfm support > > Based on the LastFM API documentation at: > http://www.lastfm.com.br/api/authentication Tested this one locally - works nice, please push
Review of attachment 306788 [details] [review]: ::: data/dbus-interfaces.xml @@ +248,3 @@ + <!-- MusicDisabled: + @since: 3.17 There's already code to add "Music" support in the VK support patch (bug 705969), this should be split out. ::: src/goabackend/goalastfmprovider.c @@ +459,3 @@ + gtk_dialog_set_response_sensitive (data->dialog, GTK_RESPONSE_OK, FALSE); + + //action_area = gtk_dialog_get_action_area (data->dialog); Those should get removed, or used. @@ +467,3 @@ + gtk_container_add (GTK_CONTAINER (grid0), data->progress_grid); + + //gtk_button_box_set_child_non_homogeneous (GTK_BUTTON_BOX (action_area), data->progress_grid, TRUE); Ditto.
Created attachment 307481 [details] [review] add Lastfm support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
Bug 705969 already contains a patch to add Music support.
Created attachment 307845 [details] [review] add Lastfm support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication https://bugzilla.gnome.org/show_bug.cgi?id=728621
Created attachment 307846 [details] [review] add Lastfm support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
I've removed everything on this patch that I think conflicts with https://bugzilla.gnome.org/attachment.cgi?id=303845
Review of attachment 307846 [details] [review]: One quick drive by comment: ::: src/goabackend/goalastfmprovider.c @@ +916,3 @@ + GtkGrid *grid, + G_GNUC_UNUSED GtkGrid *dummy) +{ With the changes in bug 752941 now in master, you shouldn't need to implement this anymore. You can remove it once https://bugzilla.gnome.org/show_bug.cgi?id=705969#c30 has been addressed.
Review of attachment 307846 [details] [review]: ::: src/goabackend/goalastfmprovider.c @@ +74,3 @@ +get_provider_features (GoaProvider *_provider) +{ + return GOA_PROVIDER_FEATURE_BRANDED; You also need GOA_PROVIDER_FEATURE_MUSIC for the default implementation of show_account to work.
Created attachment 308789 [details] [review] add Lastfm support I took the liberty to update the patch.
Thank you Rishi, you're my hero! ;-)
Review of attachment 308789 [details] [review]: Might be worth having some icons because it looks kind of empty now. We should also update the README. A few more comments below: ::: src/goabackend/goalastfmprovider.c @@ +456,3 @@ + + gtk_dialog_add_button (data->dialog, _("_Cancel"), GTK_RESPONSE_CANCEL); + data->connect_button = gtk_dialog_add_button (data->dialog, _("_Login"), GTK_RESPONSE_OK); The other providers use "C_onnect". I don't have a personal preference for either one, but I do want to be consistent. @@ +472,3 @@ + gtk_container_add (GTK_CONTAINER (data->progress_grid), spinner); + + label = gtk_label_new (_("Login…")); The others use "Connecting…". If we do stay with "_Login", then I think this one should be "Logging in…". @@ +527,3 @@ + rest_proxy_call_get_payload (call), + rest_proxy_call_get_payload_length (call), + &data->error)) While testing, I managed to hit this CRITICAL: json_parser_load_from_data: assertion 'data != NULL' failed Can you please move the rest_proxy_call_get_payload call out of the if condition, and add an explicit check for data == NULL? We can return the same "Could not parse response" error that we are returning now. We should fix this elsewhere too. @@ +619,3 @@ + rest_proxy_call_add_param (call, "format", "json"); + + g_cancellable_reset (data->cancellable); We shouldn't touch the GCancellable here. Plus we already reset it before the call. @@ +621,3 @@ + g_cancellable_reset (data->cancellable); + + rest_proxy_call_async (call, callback, NULL, data, &data->error); This call is not cancellable. That means that pressing the cancel button while the network communication is in progress will not have the desired effect. You can either listen to GCancellable::cancelled and use rest_proxy_call_cancel, or you can wrap it into standard GIO-ish cancellable async operation by using g_task_set_return_on_cancel. @@ +676,3 @@ + username, + provider_type, + (GoaPeekInterfaceFunc) goa_object_peek_password_based, Shouldn't this be goa_object_peek_oauth2_based ? This provider doesn't implement the PasswordBased interface. @@ +697,3 @@ + _("Dialog was dismissed (%s, %d): "), + g_quark_to_string (data.error->domain), + data.error->code); Pressing the cancel button leads to a crash at this point. Usually when an asynchronous operation is cancelled, it is expected to set G_IO_ERROR_CANCELLED. Since this code doesn't handle cancellation, I suspect (for some reason gdb is not giving me useful backtraces) that the error is NULL here. @@ +709,3 @@ + { + gtk_button_set_label (GTK_BUTTON (data.connect_button), _("_Ignore")); + } For branded providers like this one, we don't want to be lenient with SSL issues. The SSL handling is just for generic stuff like Exchange and ownCloud where one might be running there own server. In this case we should simply fail the connection, which is already the case because RestProxy:ssl-strict is TRUE by default. We simply don't need this branch.
(In reply to Felipe Borges from comment #21) > Thank you Rishi, you're my hero! ;-) No problem. I figured that it was easier for me to just fix it, rather than bother you and waste some more time. :) Anyway, I left some more items for you. By the way, the org.gnome.OnlineAccounts.Music interface is now in master.
Created attachment 309023 [details] [review] add Lastfm support Based on the LastFM API documentation at: http://www.lastfm.com.br/api/authentication
@@ +697,3 @@ + _("Dialog was dismissed (%s, %d): "), + g_quark_to_string (data.error->domain), + data.error->code); I just can't reproduce this after the last changes. Can you?
(In reply to Felipe Borges from comment #25) > @@ +697,3 @@ > + _("Dialog was dismissed (%s, %d): "), > + g_quark_to_string (data.error->domain), > + data.error->code); > > I just can't reproduce this after the last changes. Can you? I can't, either. I will take a closer look at the cancellation code tomorrow once again.
Review of attachment 309023 [details] [review]: Thanks for the new patch, Felipe. This looks better, but we lost a few minor things from the last version: ::: po/POTFILES.in @@ +25,3 @@ src/goabackend/goawebview.c src/goabackend/goawindowsliveprovider.c +src/goabackend/goalastfmprovider.c Let's keep these in alphabetical order. ::: src/goabackend/goalastfmprovider.c @@ +74,3 @@ +get_provider_features (GoaProvider *_provider) +{ + return GOA_PROVIDER_FEATURE_BRANDED; This should include GOA_PROVIDER_FEATURE_MUSIC so that ... @@ +937,3 @@ + GtkBox *vbox, + GtkGrid *grid, + G_GNUC_UNUSED GtkGrid *dummy) ... you don't have to implement this.
I recall that the desktop authentication flow [1] didn't require a web browser (see comment 8). But now, step 3 says "Your application needs to open a web browser ...". Did something change recently? [1] http://www.last.fm/api/desktopauth
Created attachment 311009 [details] [review] add Lastfm support Restoring the bits that we lost in the previous version of the patch. I still need to check the cancellation path.
Review of attachment 311009 [details] [review]: ::: src/goabackend/goalastfmprovider.c @@ +274,3 @@ + GoaAccount *account; + const gchar *username; + gchar *password; password should be initialized with NULL. Otherwise, we'll get a memory error in the error paths. eg., if the credentials were not found in the keyring.
Review of attachment 311009 [details] [review]: ::: src/goabackend/goalastfmprovider.c @@ +824,3 @@ + dialog = gtk_dialog_new_with_buttons (NULL, + parent, + GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT, We are missing GTK_DIALOG_USE_HEADER_BAR ... @@ +825,3 @@ + parent, + GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT, + _("_Cancel"), GTK_RESPONSE_CANCEL, ... and this isn't needed because we already have a "Cancel" button.
Created attachment 311012 [details] [review] add Lastfm support Fixed the above issues.
Tested locally, works well. Updated the blockers as well, the Music part of the patch is now merged.
Review of attachment 311012 [details] [review]: ::: configure.ac @@ +574,3 @@ Telepathy provider: ${enable_telepathy} Pocket provider: ${enable_pocket} (id:${with_pocket_client_id}) + LastFM provider ${enable_lastfm} (id:${with_lastfm_client_id} secret:${with_lastfm_client_secret}) Let's be consistent and use Last.fm everywhere. ::: src/goabackend/goalastfmprovider.c @@ +164,3 @@ + const gchar *username, + const gchar *password, + GError **error) This function has a lot in common with its async variant. I would like to refactor them into a GoaLastfmClient object - similar to GoaHttpClient, GoaEwsClient, etc.. This isn't a blocker for this. We can do it later. @@ +230,3 @@ + + root = json_parser_get_root (parser); + if (!root) After reading the json-glib code, I don't think this call can fail if we managed to load the data successfully. The documentation doesn't explicitly say this, but it doesn't deny it either. Also, we are using it that way in multiple places without any issues. @@ +258,3 @@ + + out: + g_object_unref (parser); Should be g_clear_object. @@ +538,3 @@ + + if (payload == NULL) + { It would be more correct to first look at the error passed by librest before checking the payload. This is relatively harmless, so we can do it when refactoring this code later. See above. @@ +542,3 @@ + GOA_ERROR, + GOA_ERROR_FAILED, + _("Could not pare response")); Typo alert: "parse", not "pare". @@ +607,3 @@ +on_rest_proxy_call_cancelled_cb (GCancellable *cancellable, RestProxyCall *call) +{ + rest_proxy_call_cancel (call); This is why cancellation works now. Perfect. @@ +737,3 @@ + + markup = g_strdup_printf ("<b>%s:</b>\n%s", + _("Error connecting to last.fm server"), "Error connecting to Last.fm" sounds better because there is only one Last.fm instance on the Internet. Right?
Created attachment 311144 [details] [review] Add Last.fm Since we are in string and UI freeze, I am going to ask for permission now.
Created attachment 311145 [details] [review] Add Last.fm Minor identation fix.
New strings are: * "Last.fm" * "Error connecting to Last.fm"
Created attachment 311146 [details] Screenshot showing an added Last.fm account
An icon would be nice as well...
Comment on attachment 311145 [details] [review] Add Last.fm Permission received from release team and the translators. Pushed to master.
(In reply to Bastien Nocera from comment #39) > An icon would be nice as well... I have asked Jimmac for one.