Created attachment 296489 [details] [review]
vk patch

Review of attachment 296489 [details] [review]:

I'm not going to bother reviewing this. A few quick notes:
- Don't use C++ comments ('//')
- Don't use C99 declarations (declarations always at the top of the block)
- Do error checking. Stuff like this is way ugly:
  grl_media_set_url (media, json_node_get_string (json_object_get_member (json_node_get_object (element_node), "url")));

Created attachment 297065 [details] [review]
vk patch

- Changed comments
- Moved declarations to the top everywhere except where it would cause to spawn unnecessary ifdefs
- Handled every possible and impossible corner cases (i think)

Review of attachment 297065 [details] [review]:

Please submit patches in git format, so they contain authorship details.

This also needs test cases, see the tests/ sub-directory, where you can fake the network responses.

::: src/vkontakte/grl-vkontakte.c
@@ +55,3 @@
+#define URL_VK_BROWSE                URL_VK_BASE "audio.get?format=json&access_token=%s" VK_API_VER
+#define URL_VK_SEARCH                URL_VK_BASE "audio.search?format=json&q=%s&count=%d&offset=%d&access_token=%s" VK_API_VER
+#define URL_VK_SEARCH_AUTO_COMPLETE  URL_VK_SEARCH "&auto_complete=1"

This is unused.

@@ +283,3 @@
+  static GList *keys = NULL;
+  if (!keys) {
+    keys = grl_metadata_key_list_new (/*GRL_METADATA_KEY_ID,*/

Either remove or implement those keys.

KEY_ID is required though, even if you generate the ID yourself from other information.

@@ +291,3 @@
+                                      GRL_METADATA_KEY_URL,
+                                      GRL_METADATA_KEY_INVALID);
+  }                                   /*Commented for future use*/

Hmm? Remove that.

@@ +306,3 @@
+
+  vk_http_search = g_strdup_printf (URL_VK_SEARCH,
+                                    ss->text,

This is completely incorrect. This won't work for non-ASCII values, values with spaces, etc. (and it allows the users to pass extra parameters to the HTTP query)

@@ +357,3 @@
+  GrlVkGoaData *grl_data = (GrlVkGoaData *) user_data;
+
+  if (strcmp (goa_account_get_provider_type (acc), GOA_VK_NAME) != 0) {

No need for braces for one-line conditionals.

In this case, the whole condition should be removed, you've already checked that value in grl_vkontakte_goa_init().

@@ +361,3 @@
+  }
+
+

Extra empty lines.

@@ +365,3 @@
+
+  if (status && !g_hash_table_contains (grl_data->sources, account_id)) {
+    /*plugin is enabled, but source is not registered. creating source*/

Move the comment above the check, change that to a question, add spaces before and after the comment itself, and capitalise the sentence.

@@ +473,3 @@
+    GoaAccount *acc = goa_object_peek_account (tmp->data);
+
+    if (strcmp (goa_account_get_provider_type (acc), "vk") == 0) {

g_strcmp0()

>+#define URL_VK_SEARCH_AUTO_COMPLETE  URL_VK_SEARCH "&auto_complete=1"
>
>This is unused.

The idea was that it would be nice to provide the ability to search the music both with and without this parameter. The problem is, the gnome-music UI doesn't have an option to provide the custom switches for the search engines. I hope that it will, at some point.

>KEY_ID is required though, even if you generate the ID yourself from other information.

Does it mean that I should construct the ID myself manually, or that it would be constructed automatically? If the former, then what are the requirements for such an ID, apart from uniqueness? Are there any helper functions to construct it?

>Either remove or implement those keys.

I left it as a TODO for myself, I'll remove it.

>This is completely incorrect. This won't work for non-ASCII values, values with spaces, etc. (and it allows the users to pass extra parameters to the HTTP query)

Actually, it worked for non-ASCII and spaces and stuff. But I see the security issues here. Would some escaping be enough? Does grilo have some helper functions for that?

>@@ +357,3 @@
>+  GrlVkGoaData *grl_data = (GrlVkGoaData *) user_data;
>+
>+  if (strcmp (goa_account_get_provider_type (acc), GOA_VK_NAME) != 0) {

>In this case, the whole condition should be removed, you've already checked that value in grl_vkontakte_goa_init().

No, it's different. grl_vkontakte_goa_update() and grl_vkontakte_goa_remove() are called whenever ANY GOA accounts are updated or removed, not only VK (see g_signal_connect()'s).

Although when we call it manually from grl_vkontakte_goa_init() we indeed call it only for VK accounts.

(In reply to Morse from comment #5)
> >+#define URL_VK_SEARCH_AUTO_COMPLETE  URL_VK_SEARCH "&auto_complete=1"
> >
> >This is unused.
> 
> The idea was that it would be nice to provide the ability to search the
> music both with and without this parameter. The problem is, the gnome-music
> UI doesn't have an option to provide the custom switches for the search
> engines. I hope that it will, at some point.

Right, but this isn't an API documentation. You can add that information to a separate bug once the VK source is committed.

> >KEY_ID is required though, even if you generate the ID yourself from other information.
> 
> Does it mean that I should construct the ID myself manually, or that it
> would be constructed automatically? If the former, then what are the
> requirements for such an ID, apart from uniqueness? Are there any helper
> functions to construct it?

They just need to be unique, and for you to be able to recognise what it "is". When browsing for example, I should be able to tell the VK source to start browsing "here" by just passing a GrlMediaBox with the right ID.

> >Either remove or implement those keys.
> 
> I left it as a TODO for myself, I'll remove it.

OK.

> >This is completely incorrect. This won't work for non-ASCII values, values with spaces, etc. (and it allows the users to pass extra parameters to the HTTP query)
> 
> Actually, it worked for non-ASCII and spaces and stuff.

That's lucky then ;)

> But I see the
> security issues here. Would some escaping be enough? Does grilo have some
> helper functions for that?

g_uri_escape_string(). The vimeo or jamendo plugins use it.

(In reply to Morse from comment #6)
> >@@ +357,3 @@
> >+  GrlVkGoaData *grl_data = (GrlVkGoaData *) user_data;
> >+
> >+  if (strcmp (goa_account_get_provider_type (acc), GOA_VK_NAME) != 0) {
> 
> >In this case, the whole condition should be removed, you've already checked that value in grl_vkontakte_goa_init().
> 
> No, it's different. grl_vkontakte_goa_update() and
> grl_vkontakte_goa_remove() are called whenever ANY GOA accounts are updated
> or removed, not only VK (see g_signal_connect()'s).
> 
> Although when we call it manually from grl_vkontakte_goa_init() we indeed
> call it only for VK accounts.

OK, might want to add a comment to that effect in the code. It should use g_strcmp0() as commented about goa_init().

Created attachment 298516 [details] [review]
vk_patch

added the test unit and the initial VK-service error handling

Created attachment 298521 [details] [review]
vk_patch

Fix the regexps for the test unit

Created attachment 298530 [details] [review]
vk_patch

Necessary changes for tests to work in non-GOA environments.

Created attachment 298538 [details] [review]
grile patch

Add the new error type

Created attachment 298539 [details]
vk_patch

Make use of the new error type

Created attachment 298540 [details] [review]
vk_patch

Changed the content type to "patch".

Created attachment 298710 [details]
vk_patch

change the source naming pattern

Created attachment 298711 [details] [review]
vk_patch

I should really stop forgetting pressing this "patch" checkbox

Review of attachment 298538 [details] [review]:

Please create your patches with "git format-patch", not as the output of "git show"

::: src/grl-error.h
@@ +50,3 @@
  * @GRL_CORE_ERROR_NOTIFY_CHANGED_FAILED: Failed to start changed notifications
  * @GRL_CORE_ERROR_OPERATION_CANCELLED: The operation was cancelled
+ * @GRL_CORE_ERROR_GOA_ERROR: Invalid OAuth2 token provided by GOA

I would make this more generic. GRL_CORE_ERROR_AUTHENTICATION_TOKEN ?

Review of attachment 298711 [details] [review]:

Nearly there!

::: src/Makefile.am
@@ +61,2 @@
 if MAGNATUNE_PLUGIN
+SUBDIRS += magnatune

Why is there a whitespace change here?

::: src/vkontakte/grl-vkontakte.c
@@ +51,3 @@
+/* --- URLs --- */
+
+#define VK_API_VER                   "&v=5.28"

I would do:
#define VK_API_VER "5.28"
and later:

@@ +53,3 @@
+#define VK_API_VER                   "&v=5.28"
+#define URL_VK_BASE                  "https://api.vk.com/method/"
+#define URL_VK_BROWSE                URL_VK_BASE "audio.get?format=json&access_token=%s" VK_API_VER

#define URL_VK_BROWSE                URL_VK_BASE "audio.get?format=json&access_token=%s&v=" VK_API_VER

@@ +67,3 @@
+
+#define PERSONAL_SOURCE_ID      "grl-vk-%s"
+#define PERSONAL_SOURCE_NAME    _("VK.com (%s)")

You'll need a translator comment here.

@@ +372,3 @@
+                                                      NULL,
+                                                      &error)) {
+      GRL_ERROR ("Can't get VK token for %s: %s\n", vk_username, error->message);

GRL_WARNING() instead.

@@ +427,3 @@
+  source = (GrlVkontakteSource *)g_hash_table_lookup (grl_data->sources, account_id);
+
+  if (!source) {

Remove the braces here. Not needed for one-line conditionals.

@@ +481,3 @@
+  g_signal_connect (cl, "account-changed", G_CALLBACK (grl_vkontakte_goa_update), user_data);
+
+  for (tmp = g_list_first (vk_acc_list); tmp; tmp = g_list_next (tmp)) {

for (l = vk_acc_list; l != NULL; l = l->next) {

@@ +525,3 @@
+  }
+
+  /*{"response":{"count":####,"items":[{"id":####,"owner_id":####,"artist":"####","title":"####","duration":####,"url":"####","lyrics_id":####,"genre_id":####}, ... ]}*/

Add spaces before and after the comment text

@@ +527,3 @@
+  /*{"response":{"count":####,"items":[{"id":####,"owner_id":####,"artist":"####","title":"####","duration":####,"url":"####","lyrics_id":####,"genre_id":####}, ... ]}*/
+
+  /*{"error":{"error_code":####,"error_msg":"####","request_params":[{"key":"####","value":"####"}, ... ]}}*/

Ditto.

@@ +534,3 @@
+                         GRL_CORE_ERROR_SEARCH_FAILED,
+                         "%s",
+                         "VK server sent an incorrect JSON");

unhandled JSON I'm guessing.

@@ +558,3 @@
+    err_code = json_object_get_int_member (json_obj, "error_code");
+    err_msg = json_object_get_string_member (json_obj, "error_msg");
+    switch (err_code) {

Split the massive switch statement into another function.

@@ +636,3 @@
+
+out:
+  /*finish the search operation*/

Ditto about the space before comments.

@@ +657,3 @@
+
+  if (!JSON_NODE_HOLDS_OBJECT(element_node)) {
+    GRL_WARNING ("erroneous media JSON in VK response");

Unhandled? We should probably print the data, at least in a GRL_DEBUG().

@@ +671,3 @@
+  id = g_strdup_printf("audio_%lx", (guint64) json_object_get_int_member (json_obj, "id"));
+  grl_media_set_id (media, id);
+  grl_media_audio_set_artist (GRL_MEDIA_AUDIO(media), string_or (json_object_get_string_member (json_obj, "artist"), _("Unknown")));

This should use a context, and have a translator comment.

See C_() in the glib docs.

@@ +672,3 @@
+  grl_media_set_id (media, id);
+  grl_media_audio_set_artist (GRL_MEDIA_AUDIO(media), string_or (json_object_get_string_member (json_obj, "artist"), _("Unknown")));
+  grl_media_set_title (media, string_or (json_object_get_string_member (json_obj, "title"), _("Unknown")));

Ditto.

::: tests/vkontakte/test_vk.c
@@ +82,3 @@
+  g_assert_cmpstr (grl_media_audio_get_artist (GRL_MEDIA_AUDIO (media)),
+                   ==,
+                   "Foo");

Is this with real data you got from the site, or manufactured data?

Real data would be nicer (you can certainly scrub things like access tokens).

Review of attachment 298711 [details] [review]:

::: src/vkontakte/grl-vkontakte.c
@@ +351,3 @@
+  GoaAccount *acc = goa_object_peek_account (object);
+  const gchar *account_id = goa_account_get_id (acc);
+  gboolean status = !(goa_account_get_chat_disabled (acc)); /*TODO: make it music*/

enabled would be good instead.

>I would make this more generic. GRL_CORE_ERROR_AUTHENTICATION_TOKEN ?

Authentication token what? AUTH_TOKEN_ERROR sounds more coherent then.

> ::: src/Makefile.am
> @@ +61,2 @@
>  if MAGNATUNE_PLUGIN
> +SUBDIRS += magnatune
> 
> Why is there a whitespace change here?

Trailing whitespace autoremoved, I guess. It happened accidentally, but I don't see any harm from it.

> ::: tests/vkontakte/test_vk.c
> @@ +82,3 @@
> +  g_assert_cmpstr (grl_media_audio_get_artist (GRL_MEDIA_AUDIO (media)),
> +                   ==,
> +                   "Foo");
> 
> Is this with real data you got from the site, or manufactured data?
> 
> Real data would be nicer (you can certainly scrub things like access tokens).

This is a real data, I just changed the content of the text fields and id's and stuff like that. And tokens, obviously.

>::: src/vkontakte/grl-vkontakte.c
>@@ +351,3 @@
>+  GoaAccount *acc = goa_object_peek_account (object);
>+  const gchar *account_id = goa_account_get_id (acc);
>+  gboolean status = !(goa_account_get_chat_disabled (acc)); /*TODO: make it >music*/
>
>enabled would be good instead.

Agreed. Too bad there is no such function :)
Or at least there is no such function according to the docs.

Other than that, I will submit the new version soon.

(In reply to Morse from comment #20)
> >I would make this more generic. GRL_CORE_ERROR_AUTHENTICATION_TOKEN ?
> 
> Authentication token what? AUTH_TOKEN_ERROR sounds more coherent then.

An error related to the authentication token. The domain is called GRL_CORE_ERROR, the error must have that prefix, and there's no need to duplicate the ERROR in there.

> > ::: src/Makefile.am
> > @@ +61,2 @@
> >  if MAGNATUNE_PLUGIN
> > +SUBDIRS += magnatune
> > 
> > Why is there a whitespace change here?
> 
> Trailing whitespace autoremoved, I guess. It happened accidentally, but I
> don't see any harm from it.

It's unrelated to your change. Feel free to send a separate patch for it if you can that much about it ;)

> > ::: tests/vkontakte/test_vk.c
> > @@ +82,3 @@
> > +  g_assert_cmpstr (grl_media_audio_get_artist (GRL_MEDIA_AUDIO (media)),
> > +                   ==,
> > +                   "Foo");
> > 
> > Is this with real data you got from the site, or manufactured data?
> > 
> > Real data would be nicer (you can certainly scrub things like access tokens).
> 
> This is a real data, I just changed the content of the text fields and id's
> and stuff like that. And tokens, obviously.

Ok, it's just that the "foo", "bar", etc. in the names looked bizarre.

> >::: src/vkontakte/grl-vkontakte.c
> >@@ +351,3 @@
> >+  GoaAccount *acc = goa_object_peek_account (object);
> >+  const gchar *account_id = goa_account_get_id (acc);
> >+  gboolean status = !(goa_account_get_chat_disabled (acc)); /*TODO: make it >music*/
> >
> >enabled would be good instead.
> 
> Agreed. Too bad there is no such function :)
> Or at least there is no such function according to the docs.

I meant "gboolean enabled = ...". I know that there's no "disabled" variant of the GOA function, I wrote the pocket grilo plugin which uses GOA as well :)

> Other than that, I will submit the new version soon.

Great, thanks.

Created attachment 299011 [details] [review]
vk_patch

- Comments fixed
- One-lined if's fixed
- switch moved to the separate func
- small issues taken care of

About "unhadled" and "invalid": I changed it, but only where it really "unhandled". In places where the error would indicate that the answer is different from what is expected by API docs, I left "invalid". It's not our fault if VK is not following it's own API (I never really expect this to happen, so it's mostly academical).

Created attachment 299013 [details] [review]
grilo_patch

Change error name and description

Not sure how you created those patches, but they have Windows line endings, and don't apply without fuzz to grilo master.

Could you recreate those patches using git-format patch from git master?

Created attachment 299029 [details] [review]
grilo_patch

>Not sure how you created those patches

git show > vk_patch.diff
And then I opened it in the gedit and copypasted the text into the "File" window of the bugzilla. Can't imagine where the windows LEs could infiltrate.

Now I'll try to upload the file as an attachment, maybe it'll help.

Created attachment 299030 [details] [review]
vk_patch

As attached file

(In reply to Morse from comment #25)
> Created attachment 299029 [details] [review] [review]
> grilo_patch
> 
> >Not sure how you created those patches
> 
> git show > vk_patch.diff

Use:
git format-patch -1 --stdout > vk_patch.diff
instead. Or "git format-patch origin/master" which will create patches in the current directory. I already mentioned that in comment 17.

Review of attachment 299029 [details] [review]:

"git format-patch" please.

Review of attachment 299030 [details] [review]:

"git format-patch" please.

Created attachment 299038 [details] [review]
vk_patch

format-patch

Sorry, didn't notice it in the comment.

Created attachment 299039 [details] [review]
grilo_patch

format-patch

Review of attachment 299038 [details] [review]:

Marking as reviewed, it's only missing the goa music support.

Comment on attachment 299039 [details] [review]
grilo_patch

The grilo patch is now in.

(In reply to Bastien Nocera from comment #32)
> Review of attachment 299038 [details] [review] [review]:
> 
> Marking as reviewed, it's only missing the goa music support.

Also, VK-goa is not quite ready. I didn't work on it for some time, but perhaps it's time to finish the job. I'll try to do it during the week.

Created attachment 305844 [details] [review]
vk_patch

- added a mechanism to notify GOA if the OAuth2 token is invalid
- changed chat to music (now requires the patches from #705969 to compile)
- improved the browsing functionality based on users feedback (support for albums and friend's music)
- updated the tests for the new browsing behavior
- fixed one memory leak

tested with rhythmbox's grilo integration plugin

Review of attachment 305844 [details] [review]:

Marking as rejected, as we said we'd implement this in Lua instead.

Please split off the tests into a separate commit.

Add to the "request" component.

Created attachment 314068 [details] [review]
Add support for VK.com provider

Now in lua! And with photos too.

Review of attachment 314068 [details] [review]:

A couple of nitpicks.

> Add support for VK.com provider

Add a "lua-factory: " as a prefix.

Looks fine otherwise, waiting on the lua-factory grilo changes and the goa support.

::: src/lua-factory/sources/grl-vk-music.lua
@@ +173,3 @@
+    else
+      -- TODO: error handling
+      print ("[grl-lua-vk]incorrect box ID: "..table.concat(media_id, "_"))

Use grl.lua.inspect() instead.

@@ +199,3 @@
+  if (json.error) then
+    -- TODO: error handling
+    print ("some error")

Can you add debug output that would allow to fix the problem instead?
Also use grl.lua.inspect().

@@ +251,3 @@
+  if (json.error) then
+    -- TODO: error handling
+    print ("some error")

Ditto.

::: src/lua-factory/sources/grl-vk-photos.lua
@@ +173,3 @@
+    else
+      -- TODO: error handling
+      print ("[grl-lua-vk]incorrect box ID: "..table.concat(media_id, "_"))

Ditto.

@@ +199,3 @@
+  if (json.error) then
+    -- TODO: error handling
+    print ("some error")

Ditto.

@@ +225,3 @@
+  if (json.error) then
+    -- TODO: error handling
+    print ("some error")

Ditto.

@@ +251,3 @@
+  if (json.error) then
+    -- TODO: error handling
+    print ("some error")

Ditto.

> Can you add debug output that would allow to fix the problem instead?
That's not about debug output. It's about error handling. These TODOs are waiting for the error throwing to be implemented.

Created attachment 316766 [details] [review]
lua-factory: add support for VK.com provider

Updated source. Now with error handling. Requires patches from #759013 (error reporting & token check).

Created attachment 342126 [details] [review]
Makefile.am rebase

Tried testing in rhythmbox. The plugin is not getting loaded by lua-factory.

First of all, the last bunch of changes needed for this plugin was never merged (error handling).

But more importantly, vk blocked its audio api for all non-official apps (i.e. pretty much all of them). Theoretically, there is a way to gain this "official" status, but I'm pretty sure it's nigh impossible to get it for an open source project.

From December 16, 2016, the public API for working with audio files will be disabled. Existing methods for the audio section will be unavailable for calling, except for methods regarding audio file uploads.

Source: https://vk.com/dev/audio_api

Comment on attachment 342126 [details] [review]
Makefile.am rebase

Marking patch obsolete. Refer comment 44 and comment 45.

Closing the bug then