Created attachment 306852 [details] [review]
initial goa support

Initial support. Only deals with source creation-destruction.

Also includes a patch from bug 751786

Review of attachment 306852 [details] [review]:

One patch per attachment otherwise you're breaking the review tool. My patch is already in bugzilla as well...

Created attachment 306867 [details] [review]
initial goa support

Ok. I just wanted to stress out that the patch is not based on git master. I opted to do it since it changed the very same code.

Review of attachment 306867 [details] [review]:

> The new field in 'source' struct called 'vk_account_type' is added.

It's not called vk_account_type :)

This patch seems overly complicated. I would:
- In grl_lua_factory_source_new(), add a new return value that, when you return %NULL because the goa type isn't available, sets a new variable.
Something like: static GrlLuaFactorySource *grl_lua_factory_source_new (gchar *lua_plugin_path, GList *configs, gboolean is_goa)
- Make sure to populate _GrlLuaFactorySourcePrivate with additional metadata to allow you to know what account/usage that enabled source is connected to (for example, a VK.com/Music source)
- Whenever goa accounts change, go through 1) the list of enabled sources to see which ones you can disable 2) go through the list of filepaths you set aside when grl_lua_factory_source_new failed to try and enable them again
- Finally, make sure that, in the lua source, you need to explicitely mention the type of Account, and the type of Usage. Your current implementation wouldn't allow to create a Video source from the Pocket/Read Later combination
- Also make sure that, if there are multiple accounts of the same type (say, 2 VK accounts) that you get 2 sources created.

Thanks for working on this!
 When we parse the sources one-by-one, in grl_lua_factory_plugin_init(), we iterate through a list of paths. In grl_lua_factory_source_new(), I add a new return value

> - In grl_lua_factory_source_new(), add a new return value that, when you return %NULL because the goa type isn't available, sets a new variable.
Something like: static GrlLuaFactorySource *grl_lua_factory_source_new (gchar *lua_plugin_path, GList *configs, gboolean is_goa)

I didn't get that. Why would grl_lua_factory_source_new() ever return NULL? The reason why I introduced a whole new set of functions is simple: not only every GOA account can spawn two (and perhaps three in the future, if we ever add a "video" provider) sources, but there also may be more than one account of the same type. And moreover, I don't always need all the accounts I can create. During the update I need only a single particular source to create/destroy.

> - Make sure to populate _GrlLuaFactorySourcePrivate with additional metadata to allow you to know what account/usage that enabled source is connected to (for example, a VK.com/Music source)

Well, the source-id already incorporates both the account-id and the type of media. I don't see how it can help though. I do have the need to access goa account from the lua-factory-lib in order to implement some basic functions, like getting the oauth2 token, but since I can't (or, rather, shouldn't) access ->priv structure from there, I was thinking about pushing it to lua global.

>- Finally, make sure that, in the lua source, you need to explicitely mention the type of Account, and the type of Usage. Your current implementation wouldn't allow to create a Video source from the Pocket/Read Later combination

That's because GOA doesn't provide the video feature. Yet. And I don't think it's a good idea to implement the way to override that. The user will get the source he can't disable in GOA.

- Also make sure that, if there are multiple accounts of the same type (say, 2 VK accounts) that you get 2 sources created.

While I din't check it, it should work fine. As I said, it creates as many sources as needed, and manages them all dynamically. So while yes, the patch is not the simplest, I don't see a way to simplify it much.

Also take notice, that I really kept #ifdefs quite localized. If I would start messing with grl_lua_factory_source_new the code would quickly become an ifdef-mess.

Created attachment 307110 [details] [review]
lua-factory: Add GOA support

Make it possible for sources to use gnome-online-accounts to get
access_token and consumer key for a particular account provider
(Google, VK, etc.) and account feature (Music, Read Later, etc.).

Note that this code is completely untested. I'm not happy with the "sync" call to get the access_token. There's also no way to get the secret access token. Do you need it?

Ok, I'll look into patch tomorrow, but one thing bothers me from the start: you use OperationSpec to get oauth2_token, which means that it won't work in grl_source_init(), which is the most logical place to retrieve the token.

Created attachment 307121 [details] [review]
lua-factory: Add GOA support

Make it possible for sources to use gnome-online-accounts to get
access_token and consumer key for a particular account provider
(Google, VK, etc.) and account feature (Music, Read Later, etc.).

Based on original patch by RadistMorse <radist.morse@gmail.com>

Created attachment 307122 [details] [review]
lua-factory: Add missing config.h includes

(In reply to Morse from comment #8)
> Ok, I'll look into patch tomorrow, but one thing bothers me from the start:
> you use OperationSpec to get oauth2_token, which means that it won't work in
> grl_source_init(), which is the most logical place to retrieve the token.

Noted, that would solve the problem with the the "sync" calls for the tokens.

Created attachment 307219 [details] [review]
auxilary GOA functions

It has also unrelated stuff which I didn't clear out (like the "grl.encode" function, that complements the "grl.decode"), but is mostly about the GOA functions.

Review of attachment 307122 [details] [review]:

You forgot to dereference source_id in enable_goa_source (). After fixing this, everything works fine. Checked with two accounts of the same type, with two lua-sources for each (music and photos). Everything is fine.

Created attachment 307309 [details] [review]
lua-factory: Add GOA support

Make it possible for sources to use gnome-online-accounts to get
access_token and consumer key for a particular account provider
(Google, VK, etc.) and account feature (Music, Read Later, etc.).

Based on original patch by RadistMorse <radist.morse@gmail.com>

Review of attachment 307219 [details] [review]:

Will try to integrate this into the existing patch shortly.

::: src/lua-factory/grl-lua-common.h
@@ +90,3 @@
 
+#ifdef GOA_ENABLED
+void grl_lua_library_save_goa_data (lua_State *L, GoaObject *goa_object);

Any reason this can't be a gpointer or a GObject instead.

::: src/lua-factory/grl-lua-library.c
@@ +1092,3 @@
+ */
+static gint
+grl_l_encode (lua_State *L)

Already in https://bugzilla.gnome.org/show_bug.cgi?id=645799

@@ +1267,3 @@
+
+/**
+ * grl.get_goa_oauth2_token

The documentation doesn't match what the code does.

Review of attachment 307309 [details] [review]:

Marking as needs-work as per the missing functionality mentioned in previous reviews.

>Any reason this can't be a gpointer or a GObject instead.
It is cast to lua userdata anyway, so whatever.

>The documentation doesn't match what the code does.
It was the quick and dirty proof-of-concept. I never intended it to be mainlined as-is.

On the unrelated note, it looks like I won't have time today to change globals to references in lua callbacks. I'll do it later.

Upcoming patch(es) changes:
- Avoid incorrect source id, name, description when creating the source (and calling the source init!)
- make creation of the GoaClient cancellable, and fix memory leak when GoaClient creation failed
- destroy the goa sources when the GrlPlugin is deinit'ed
- support calling goa lua functions from the source init

Created attachment 307504 [details] [review]
lua-factory: Add GOA support

Make it possible for sources to use gnome-online-accounts to get
access_token and consumer key for a particular account provider
(Google, VK, etc.) and account feature (Music, Read Later, etc.).

Based on original patch by RadistMorse <radist.morse@gmail.com>

Review of attachment 307504 [details] [review]:

Please, split this patch in two.
- First one with the including all goa support to grl-lua-factory including the grl_lua_library_save_goa_data function;
- The second patch with the new API for Lua sources which currently is access_token and consumer_key;

I'm probably missing things as I'm new to GOA. The VK source will be using this right?

More comments below

::: src/lua-factory/grl-lua-factory.c
@@ +34,3 @@
+#include <goa/goa.h>
+#endif
+

This could go to grl-lua-common.h as it is used by both grl-lua-factory.c and grl-lua-library.c

@@ +665,3 @@
+      return GOA_REMOVE;
+  } else if (g_strcmp0 (feature, "music") == 0) {
+    if (!goa_object_peek_music (object))

I don't have goa_object_peek_music and goa_account_get_music_disabled in JHBuild. Is there any BZ with patches for it?

Anyway, configure needs improvement to avoid breaking the build.

::: src/lua-factory/grl-lua-library.c
@@ +38,3 @@
+#define GOA_API_IS_SUBJECT_TO_CHANGE
+#include <goa/goa.h>
+#endif

Ditto.

@@ +1263,3 @@
+
+  os = grl_lua_library_get_current_operation (L);
+  g_return_val_if_fail (os != NULL, 0);

Check grl.callback, I think lua_Error is better for the developer to identify the error instead system error

@@ +1265,3 @@
+  g_return_val_if_fail (os != NULL, 0);
+
+#ifndef GOA_ENABLED

I think wrapping those functions with #ifdef GOA_ENABLE is more readable then #ifdef + #else, like:
{
#ifdef GOA_ENABLE
.. code
return
#endif

GRL_WARNING ("Source is broken...")
return;
}

@@ +1274,3 @@
+    GoaOAuth2Based *oauth2 = NULL;
+
+    object = grl_lua_library_load_goa_data (L);

This returns GObject *
"warning: assignment from incompatible pointer type"

@@ +1300,3 @@
+
+  os = grl_lua_library_get_current_operation (L);
+  g_return_val_if_fail (os != NULL, 0);

Ditto.

@@ +1312,3 @@
+    GoaOAuth2Based *oauth2 = NULL;
+
+    object = grl_lua_library_load_goa_data (L);

Ditto.

@@ +1583,3 @@
+  lua_setglobal (L, GOA_LUA_NAME);
+#else
+  GRL_WARNING ("grl_lua_library_save_goa_data() called but GOA support disabled.");

Isn't this covered by g_return_if_fail above?

@@ +1597,3 @@
+{
+  GoaObject *goa_object;
+

This should be inside #ifdef

@@ +1603,3 @@
+  lua_pop(L, 1);
+
+  return goa_object;

return G_OBJECT (goa_object);

Adding depend on bug 705969 for the music support in gnome-online-accounts.

(In reply to Victor Toso from comment #20)
> Review of attachment 307504 [details] [review] [review]:
> 
> Please, split this patch in two.
> - First one with the including all goa support to grl-lua-factory including
> the grl_lua_library_save_goa_data function;
> - The second patch with the new API for Lua sources which currently is
> access_token and consumer_key;

OK.

> I'm probably missing things as I'm new to GOA. The VK source will be using
> this right?

And the Pocket one. Possibly flickr as well.

> More comments below
> 
> ::: src/lua-factory/grl-lua-factory.c
> @@ +34,3 @@
> +#include <goa/goa.h>
> +#endif
> +
> 
> This could go to grl-lua-common.h as it is used by both grl-lua-factory.c
> and grl-lua-library.c

Done.

> @@ +665,3 @@
> +      return GOA_REMOVE;
> +  } else if (g_strcmp0 (feature, "music") == 0) {
> +    if (!goa_object_peek_music (object))
> 
> I don't have goa_object_peek_music and goa_account_get_music_disabled in
> JHBuild. Is there any BZ with patches for it?
> 
> Anyway, configure needs improvement to avoid breaking the build.

Needs a non-upstreamed patch.

> ::: src/lua-factory/grl-lua-library.c
> @@ +38,3 @@
> +#define GOA_API_IS_SUBJECT_TO_CHANGE
> +#include <goa/goa.h>
> +#endif
> 
> Ditto.
> 
> @@ +1263,3 @@
> +
> +  os = grl_lua_library_get_current_operation (L);
> +  g_return_val_if_fail (os != NULL, 0);
> 
> Check grl.callback, I think lua_Error is better for the developer to
> identify the error instead system error

Urgh. That's left-over code, it's not needed.

> @@ +1265,3 @@
> +  g_return_val_if_fail (os != NULL, 0);
> +
> +#ifndef GOA_ENABLED
> 
> I think wrapping those functions with #ifdef GOA_ENABLE is more readable
> then #ifdef + #else, like:
> {
> #ifdef GOA_ENABLE
> .. code
> return
> #endif
> 
> GRL_WARNING ("Source is broken...")
> return;
> }

It actually makes it more of a spaghetti code, as I'd need to wrap the variable declarations to avoid mixing declarations and code...

> @@ +1274,3 @@
> +    GoaOAuth2Based *oauth2 = NULL;
> +
> +    object = grl_lua_library_load_goa_data (L);
> 
> This returns GObject *
> "warning: assignment from incompatible pointer type"

Fixed.

> @@ +1300,3 @@
> +
> +  os = grl_lua_library_get_current_operation (L);
> +  g_return_val_if_fail (os != NULL, 0);
> 
> Ditto.

Removed as well.

> @@ +1312,3 @@
> +    GoaOAuth2Based *oauth2 = NULL;
> +
> +    object = grl_lua_library_load_goa_data (L);
> 
> Ditto.

Same comment as above.

> @@ +1583,3 @@
> +  lua_setglobal (L, GOA_LUA_NAME);
> +#else
> +  GRL_WARNING ("grl_lua_library_save_goa_data() called but GOA support
> disabled.");
> 
> Isn't this covered by g_return_if_fail above?

The warning has been removed above. I want to make sure that the API is still accessible from lua, but fails in those cases. Defensive programming, in case we somehow create a GOA source incorrectly.

> @@ +1597,3 @@
> +{
> +  GoaObject *goa_object;
> +
> 
> This should be inside #ifdef

Yes.

> @@ +1603,3 @@
> +  lua_pop(L, 1);
> +
> +  return goa_object;
> 
> return G_OBJECT (goa_object);

Fixed.

Created attachment 307547 [details] [review]
lua-factory: Add GOA support

Make it possible for sources to be spawned for each account of a
specific provider (Google, VK, etc.) and specific feature (Music, Read
Later, Videos).

Based on original patch by RadistMorse <radist.morse@gmail.com>

Created attachment 307548 [details] [review]
lua-factory: Add GOA Lua access functions

Add functions to access the access_token and consumer key for
GOA Lua sources.

Created attachment 307552 [details] [review]
lua-factory: Add GOA Lua access functions

Add functions to access the access_token and consumer key for
GOA Lua sources.

Comment on attachment 307122 [details] [review]
lua-factory: Add missing config.h includes

Attachment 307122 [details] pushed as 4fabe6f - lua-factory: Add missing config.h includes

Review of attachment 307552 [details] [review]:

::: src/lua-factory/grl-lua-library.c
@@ +1380,3 @@
     {"unzip", &grl_l_unzip},
+    {"access_token", &grl_l_access_token},
+    {"consumer_key", &grl_l_consumer_key},

Actually, I think it would make sense to (1) move those functions to lua-library folder and (2) put them under goa table like:

grl.goa.access_token()
grl.goa.consumer_key()

Feel free to ignore (1) if goa library is not going to grow; But I think (2) makes sense.

** I'm thinking that the 'lua' table is not useful and we should drop it in the future 
e.g: grl.lua.json -> grl.json

(In reply to Victor Toso from comment #27)
> Review of attachment 307552 [details] [review] [review]:
> 
> ::: src/lua-factory/grl-lua-library.c
> @@ +1380,3 @@
>      {"unzip", &grl_l_unzip},
> +    {"access_token", &grl_l_access_token},
> +    {"consumer_key", &grl_l_consumer_key},
> 
> Actually, I think it would make sense to (1) move those functions to
> lua-library folder and (2) put them under goa table like:
> 
> grl.goa.access_token()
> grl.goa.consumer_key()
> 
> Feel free to ignore (1) if goa library is not going to grow; But I think (2)
> makes sense.

I'd prefer keeping everything in the same namespace, such as grl.goa_access_token(), or grl.goa_consumer_key().

I don't mind moving the code, but in the worst case, we'd add one function to get the consumer_secret, and we can always move things then if you think it's too big. I don't think grl-lua-library.c is that big, 1600 lines with self-contained functions.

(In reply to Bastien Nocera from comment #28)
> (In reply to Victor Toso from comment #27)
> > Review of attachment 307552 [details] [review] [review] [review]:
> > 
> > ::: src/lua-factory/grl-lua-library.c
> > @@ +1380,3 @@
> >      {"unzip", &grl_l_unzip},
> > +    {"access_token", &grl_l_access_token},
> > +    {"consumer_key", &grl_l_consumer_key},
> > 
> > Actually, I think it would make sense to (1) move those functions to
> > lua-library folder and (2) put them under goa table like:
> > 
> > grl.goa.access_token()
> > grl.goa.consumer_key()
> > 
> > Feel free to ignore (1) if goa library is not going to grow; But I think (2)
> > makes sense.
> 
> I'd prefer keeping everything in the same namespace, such as
> grl.goa_access_token(), or grl.goa_consumer_key().

Thanks! That's fine by me as well. I just would like to have a standard to keep this sane as the library grows. The goa table suggestion is to follow the json but I'm fine about moving the json function to grl table to keep the same namespace in the future (and grl.lua.inspect too)

> 
> I don't mind moving the code, but in the worst case, we'd add one function
> to get the consumer_secret, and we can always move things then if you think
> it's too big. I don't think grl-lua-library.c is that big, 1600 lines with
> self-contained functions.

1600 lines with self-contained functions is not big indeed but it can grow easily IMHO. The only problem with moving code around is losing git history.
Feel free to decide about this.

Attachment 307547 [details] pushed as bb4b13e - lua-factory: Add GOA support
Attachment 307552 [details] pushed as 9a709c8 - lua-factory: Add GOA Lua access functions

Was it intentional to push these before the dependency in bug 705969 landed? Probably a good idea to push forward with the music patch to unbreak the build ...

Fixed in master, see also bug 752899 about re-adding that support.