GNOME Bugzilla – Bug 759013
various improvements for lua-factory
Last modified: 2018-09-24 09:39:16 UTC
next
Created attachment 316751 [details] [review] [PATCH 1/3] lua-factory: pass errors via callbacks Right now there is no way to pass errors to grilo callbacks from lua. This patch adds this possibility by the use of the callback function: callback ("error", "error description") three error types are supported: error - raises an error with the code, appropriate for the operation error_media_not_found - GRL_CORE_ERROR_MEDIA_NOT_FOUND error_authentication_token - GRL_CORE_ERROR_AUTHENTICATION_TOKEN Also, previously the broken sources that didn't do the finishing callbacks didn't send any errors to the client app. Now if the source is broken and the finishing callback is made automatically, it sends an error to the client app.
Created attachment 316752 [details] [review] [PATCH 2/3] lua-factory: add grl.goa_check_token () This patch adds the function add grl.goa_check_token that is a wrapper for the goa function goa_account_call_ensure_credentials. This functions checks if the credentials for this account are ok, and marks account for user attention otherwise. This function should be used together with the "error_authentication_token" type of error, which should also notify the client that goa account is marked for attention.
Created attachment 316754 [details] [review] [PATCH 3/3] lua-factory: add GRL_LUA_WARNING function This patch adds the GRL_LUA_WARNING function and changes GRL_WARNING to GRL_LUA_WARNING where applicable. GRL_LUA_WARNING is a function, that prints to log the message similar to GRL_WARNING, but instead of filename and linenumber of the C file in which the call happened, it logs the filename and linenumber of the lua string that caused an error. This function is intended for lua errors, that translate into grilo warnings. When debugging lua scripts we are not really interested in the linenumber in which C calls a warning, we want to know which lua line caused an error. This patch makes lua log much more readable. On the technical side, it's rather hacky. The printing of the filename and linenumber is very hardcoded into the depths of the grl-log, so in order to avoid this we should use the plain g_log, which means that we do not have an access to the grilo's G_LOG_DOMAIN_DEFAULT, and also will ignore grilo's loglevel. Suggestions how to solve this problem are welcome.
Created attachment 316759 [details] [review] [bonus track] lua-factory: change luaL_error calls I don't think that not calling the correct "return" will corrupt lua stack (it didn't so far), but better safe than sorry. Lua docs suggest it that way, so why not?
On top of master + https://bugzilla.gnome.org/show_bug.cgi?id=753141, I cannot apply this. Could you please rebase?
Created attachment 320070 [details] [review] [PATCH 1/4] lua-factory: pass errors via callbacks Rebased
Created attachment 320071 [details] [review] [PATCH 2/4] lua-factory: add grl.goa_check_token () Rebase
Created attachment 320072 [details] [review] [PATCH 3/4] lua-factory: add GRL_LUA_WARNING function As a sample, here is what the warnings look like: (grilo-test-ui-0.3:15224): Grilo-WARNING **: [lua-factory] grl-lua-factory.c:1548: calling browse function fail: '/usr/share/grilo-plugins/grl-lua-factory/grl-vktest.lua:86: attempt to index a nil value (local 'options')' This is the best case scenario: the info about the problematic lua file is there in somewhere, which is not always the case. With this patch it looks like this: (grilo-test-ui-0.3:28617): Grilo-WARNING **: [grl-vk-photos] ./usr/share/grilo-plugins/grl-lua-factory/grl-vktest.lua:73: attempt to call a table value (local 'options') See Comment 3 about how the way it is implemented is maybe not the best possible.
Created attachment 320073 [details] [review] [PATCH 4/4] lua-factory: change luaL_error calls After reading the docs some more, I found out that there is no real technical need to do the calls like this, but more of an aesthetic decision.
(In reply to Morse from comment #8) > This is the best case scenario: the info about the problematic lua file is > there in somewhere, which is not always the case. And here's the worst case scenario: (grilo-test-ui-0.3:31378): Grilo-WARNING **: [lua-library] grl-lua-library.c:85: 'string' is not compatible for 'duration' (grilo-test-ui-0.3:31378): Grilo-WARNING **: [lua-library] grl-lua-library.c:85: 'date' is not a valid keyword Good luck finding out where this error resides. With the patch: (grilo-test-ui-0.3:7655): Grilo-WARNING **: [grl-vk-music] grl-vktest.lua:276: 'string' is not compatible for 'duration' (grilo-test-ui-0.3:7655): Grilo-WARNING **: [grl-vk-music] grl-vktest.lua:276: 'date' is not a valid keyword String 276 is the place in lua code which called the C function that caused the error.
Created attachment 320085 [details] [review] [PATCH 3/4] lua-factory: add GRL_LUA_WARNING function Memory leak fix + correctly handle both in-lua and out-lua cases (you maybe noticed "./usr/share/grilo-plugins/grl-lua-factory/grl-vktest.lua:73:" in Comment 8 while it should be just "grl-vktest.lua:73:")
(In reply to Morse from comment #1) > Created attachment 316751 [details] [review] [review] > [PATCH 1/3] lua-factory: pass errors via callbacks > > Right now there is no way to pass errors to grilo callbacks from lua. > > This patch adds this possibility by the use of the callback function: > > callback ("error", "error description") > > three error types are supported: > error - raises an error with the code, appropriate for the operation > error_media_not_found - GRL_CORE_ERROR_MEDIA_NOT_FOUND > error_authentication_token - GRL_CORE_ERROR_AUTHENTICATION_TOKEN > > Also, previously the broken sources that didn't do the finishing callbacks > didn't send any errors to the client app. Now if the source is broken and > the finishing callback is made automatically, it sends an error to the > client app. Yep, that's all true and we need it!. I would prefer to provide a table with the core errors to the source and make this simpler (IMHO) callback (grl.err.media_not_found) That would avoid any typing errors and it is a struct that we can deal and manage better in the library.
Review of attachment 320070 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +321,3 @@ GrlMedia *media = user_media; + g_assert (lua_istable (L, 1)); You mean that now grl.callback() is not possible anymore but we still use it and code below still consider it valid (nparam > 0) @@ +1019,3 @@ + luaL_argcheck (L, (lua_istable (L, 1) || lua_isnil (L, 1) || + (lua_isstring (L, 1) && lua_isstring (L, 2))), 2, + "expecting error description after the error type"); As the error should be the only return value, we should have a function to generate the GError, such as error = source_has_returned_error (L); if (error != NULL) { ... } @@ +1040,3 @@ + error_code = GRL_CORE_ERROR_AUTHENTICATION_TOKEN; + else + luaL_error (L, "unknown error type: %s", error_type); And those checks should be in this function... with minor changes in this grl_l_callback function Also, with several if/else if, always use { }
(In reply to Morse from comment #8) > Created attachment 320072 [details] [review] [review] > [PATCH 3/4] lua-factory: add GRL_LUA_WARNING function > > As a sample, here is what the warnings look like: > > (grilo-test-ui-0.3:15224): Grilo-WARNING **: [lua-factory] > grl-lua-factory.c:1548: calling browse function fail: > '/usr/share/grilo-plugins/grl-lua-factory/grl-vktest.lua:86: attempt to > index a nil value (local 'options')' > > This is the best case scenario: the info about the problematic lua file is > there in somewhere, which is not always the case. With this patch it looks > like this: > > (grilo-test-ui-0.3:28617): Grilo-WARNING **: [grl-vk-photos] > ./usr/share/grilo-plugins/grl-lua-factory/grl-vktest.lua:73: attempt to call > a table value (local 'options') > > See Comment 3 about how the way it is implemented is maybe not the best > possible. There is a distinction between Plugin and Source. We can improve the info without messing with grl-log stuff directly (by using g_log ourselves)
Review of attachment 320085 [details] [review]: I don't mind a wrapper function to proper deal with the Source name, Lua file + line number but we should using the log-domain from lua-factory for that. The main reason is that Plugin != Source. IMHO we should start improving the core to better handler Sources in things that are only available for plugins, such as GRL_DEBUG, GRL_PLUGIN_RANKS, ... Maybe GRL_DEBUG and friends should be smart to point out the Source name if the Plugins has more then one source (could be the case of UPnP, flickr, ... not just Lua-Factory ones) In case you prefer handling part of this in the core, open a new bug for that and mark the dependency ;) Otherwise, just improving this wrapper would do for Lua
Review of attachment 320071 [details] [review]: PS: This should be 3/4 Was this acked before? (So many threads with GOA...) ::: src/lua-factory/grl-lua-library.c @@ +1368,3 @@ +{ + /* we don't really care about results */ + goa_account_call_ensure_credentials_finish (GOA_ACCOUNT (source_object), NULL, res, NULL); I don't know much about goa but why isn't it interesting to check for Errors? I did not get much about what this should do from the docs [0] [0] https://developer.gnome.org/goa/stable/GoaAccount.html#goa-account-call-ensure-credentials
Review of attachment 320073 [details] [review]: > see http://pgl.yoyo.org/luai/i/luaL_error Better to include interesting part as quote here instead of a URL Patch looks good appart from the commit log. I'm not sure if it was being used with the fact that it does not return in mind... maybe there is leaks to be fixed due that.
(In reply to Victor Toso from comment #12) > Yep, that's all true and we need it!. > > I would prefer to provide a table with the core errors to the source and > make this simpler (IMHO) > > callback (grl.err.media_not_found) > > That would avoid any typing errors and it is a struct that we can deal and > manage better in the library. While it's not a problem to add the grl.err table, it wont help against typing errors. Since grl.err would be a table, passing grl.err.blahblahblah would be a totally legit code, and a 'nil' would be passed as a parameter. So instead of "unknown error type: blahblahblah" the user will see "unknown error type: (nil)". (In reply to Victor Toso from comment #13) > You mean that now grl.callback() is not possible anymore but we still use it > and code below still consider it valid (nparam > 0) Emm, no. callback() remains the finishing callback without media. Just like it was. > As the error should be the only return value, we should have a function to > generate the GError, such as > > error = source_has_returned_error (L); > if (error != NULL) { > ... > } Should it? I mean, even if we provide error types in grl.err table, the error description should still be there. I even made it mandatory, I thought it was important. > > @@ +1040,3 @@ > + error_code = GRL_CORE_ERROR_AUTHENTICATION_TOKEN; > + else > + luaL_error (L, "unknown error type: %s", error_type); > > And those checks should be in this function... with minor changes in this > grl_l_callback function > > Also, with several if/else if, always use { } Ok, noted. But before I submit the new patch we should settle on the error. I still think it should be "two strings" for the error, and not "one table", because "one table" is for the regular callbacks, i.e. "one table" means it's a media. So unless we want some elaborate code that would check whether some particular lua table is a media or an error, it's better to keep them apart. (In reply to Victor Toso from comment #15) > Review of attachment 320085 [details] [review] [review]: > > I don't mind a wrapper function to proper deal with the Source name, Lua > file + line number but we should using the log-domain from lua-factory for > that. Would be awesome. Problem is, grilo's logdomain is available at grilo's compile time only. It is not stored in headers and no API provides it. I also don't like how the logging is done in this patch. Problem is, I'm afraid that's the best we can get without changing grilo's logging API. Well, consider this patch a proof-of-concept, it doesn't add any critical functionality. (In reply to Victor Toso from comment #16) > Review of attachment 320071 [details] [review] [review]: > > PS: This should be 3/4 > > Was this acked before? (So many threads with GOA...) > > ::: src/lua-factory/grl-lua-library.c > @@ +1368,3 @@ > +{ > + /* we don't really care about results */ > + goa_account_call_ensure_credentials_finish (GOA_ACCOUNT (source_object), > NULL, res, NULL); > > I don't know much about goa but why isn't it interesting to check for Errors? > > I did not get much about what this should do from the docs [0] > > [0] > https://developer.gnome.org/goa/stable/GoaAccount.html#goa-account-call- > ensure-credentials Yes, this one is tricky. Lets assume a situation, when we try to use some OAuth token from GOA and find out that it's not working, and we need to tell the user about it. Two steps are required. First, the client app should be notified that something is wrong with one of the GOA accounts, so that it could show a warning to the user, and, ideally, suggest him to launch gnome-control-center. That's what GRL_ERROR_AUTHENTICATION_TOKEN is for. The second step is actually marking the online account for attention, so that when gnome-control-center does get launched, user could see which account has failed. For that, there is a function goa_account_set_attention_needed(), which is a wrapper for a read-only DBus property, so it only works from inside GOA. Clients do not have direct access to it. That's when ensure_credentials come into play. I don't know why the online doc missed this point, but in the documentation to DBus method you can find this piece of info: If this method fails because the token service indicates that authorization has expired, the #org.gnome.OnlineAccounts.Account:AttentionNeeded property will be set to %TRUE. On the other hand, if this property was already %TRUE and this method succeeds, it is set to %FALSE. [0] And this is also the reason why we do not really care about the result. This side-effect is the only thing we really need. [0] https://git.gnome.org/browse/gnome-online-accounts/tree/data/dbus-interfaces.xml#n280
(In reply to Morse from comment #18) > (In reply to Victor Toso from comment #12) > > Yep, that's all true and we need it!. > > > > I would prefer to provide a table with the core errors to the source and > > make this simpler (IMHO) > > > > callback (grl.err.media_not_found) > > > > That would avoid any typing errors and it is a struct that we can deal and > > manage better in the library. > > While it's not a problem to add the grl.err table, it wont help against > typing errors. Since grl.err would be a table, passing grl.err.blahblahblah > would be a totally legit code, and a 'nil' would be passed as a parameter. > So instead of "unknown error type: blahblahblah" the user will see "unknown > error type: (nil)". Yeah, I'll be more careful with my `brain dump` here now. My point is to provide the errors type that Grilo does [0]. The value of grl.err.media_not_found does not need to strictly be a string or a table. [0] https://developer.gnome.org/grilo/unstable/grilo-grl-error.html > > (In reply to Victor Toso from comment #13) > > You mean that now grl.callback() is not possible anymore but we still use it > > and code below still consider it valid (nparam > 0) > > Emm, no. callback() remains the finishing callback without media. Just like > it was. > > > As the error should be the only return value, we should have a function to > > generate the GError, such as > > > > error = source_has_returned_error (L); > > if (error != NULL) { > > ... > > } > > Should it? I mean, even if we provide error types in grl.err table, the > error description should still be there. I even made it mandatory, I thought > it was important. Yeah, it is. I mean, the value in callback() is only related to the error so we should deal with it in a specific function. I agree with the error description :) So, callback(grl.err.error_type, "error description") is good enough for me > > > > > @@ +1040,3 @@ > > + error_code = GRL_CORE_ERROR_AUTHENTICATION_TOKEN; > > + else > > + luaL_error (L, "unknown error type: %s", error_type); > > > > And those checks should be in this function... with minor changes in this > > grl_l_callback function > > > > Also, with several if/else if, always use { } > > Ok, noted. But before I submit the new patch we should settle on the error. > I still think it should be "two strings" for the error, and not "one table", > because "one table" is for the regular callbacks, i.e. "one table" means > it's a media. So unless we want some elaborate code that would check whether > some particular lua table is a media or an error, it's better to keep them > apart. It should be two parameters IMHO. One is the value off grl.err.error_type (could be integer, table, userdata, ...) the second one the error-type as string. > > (In reply to Victor Toso from comment #15) > > Review of attachment 320085 [details] [review] [review] [review]: > > > > I don't mind a wrapper function to proper deal with the Source name, Lua > > file + line number but we should using the log-domain from lua-factory for > > that. > > Would be awesome. Problem is, grilo's logdomain is available at grilo's > compile time only. It is not stored in headers and no API provides it. > > I also don't like how the logging is done in this patch. Problem is, I'm > afraid that's the best we can get without changing grilo's logging API. > Well, consider this patch a proof-of-concept, it doesn't add any critical > functionality. What is the outcome when you do GRL_DEBUG=lua-factory:* with this patch? I think the lua-sources should be inside lua-factory domain as I mentioned before and we can improve that later in the core. > > (In reply to Victor Toso from comment #16) > > Review of attachment 320071 [details] [review] [review] [review]: > > > > PS: This should be 3/4 > > > > Was this acked before? (So many threads with GOA...) > > > > ::: src/lua-factory/grl-lua-library.c > > @@ +1368,3 @@ > > +{ > > + /* we don't really care about results */ > > + goa_account_call_ensure_credentials_finish (GOA_ACCOUNT (source_object), > > NULL, res, NULL); > > > > I don't know much about goa but why isn't it interesting to check for Errors? > > > > I did not get much about what this should do from the docs [0] > > > > [0] > > https://developer.gnome.org/goa/stable/GoaAccount.html#goa-account-call- > > ensure-credentials > > Yes, this one is tricky. Lets assume a situation, when we try to use some > OAuth token from GOA and find out that it's not working, and we need to tell > the user about it. Two steps are required. First, the client app should be > notified that something is wrong with one of the GOA accounts, so that it > could show a warning to the user, and, ideally, suggest him to launch > gnome-control-center. That's what GRL_ERROR_AUTHENTICATION_TOKEN is for. The > second step is actually marking the online account for attention, so that > when gnome-control-center does get launched, user could see which account > has failed. For that, there is a function > goa_account_set_attention_needed(), which is a wrapper for a read-only DBus > property, so it only works from inside GOA. Clients do not have direct > access to it. That's when ensure_credentials come into play. I don't know > why the online doc missed this point, but in the documentation to DBus > method you can find this piece of info: > > If this method fails because the token service indicates that > authorization has expired, the > #org.gnome.OnlineAccounts.Account:AttentionNeeded property > will be set to %TRUE. On the other hand, if this property was > already %TRUE and this method succeeds, it is set to %FALSE. > > [0] > And this is also the reason why we do not really care about the result. This > side-effect is the only thing we really need. Still, would be nice to check the GError and warn if something is off, no? > > [0] > https://git.gnome.org/browse/gnome-online-accounts/tree/data/dbus-interfaces. > xml#n280
(In reply to Victor Toso from comment #19) > (In reply to Morse from comment #18) > > (In reply to Victor Toso from comment #12) > > > Yep, that's all true and we need it!. > > > > > > I would prefer to provide a table with the core errors to the source and > > > make this simpler (IMHO) > > > > > > callback (grl.err.media_not_found) > > > > > > That would avoid any typing errors and it is a struct that we can deal and > > > manage better in the library. > > > > While it's not a problem to add the grl.err table, it wont help against > > typing errors. Since grl.err would be a table, passing grl.err.blahblahblah > > would be a totally legit code, and a 'nil' would be passed as a parameter. > > So instead of "unknown error type: blahblahblah" the user will see "unknown > > error type: (nil)". > > Yeah, I'll be more careful with my `brain dump` here now. > > My point is to provide the errors type that Grilo does [0]. The value of > grl.err.media_not_found does not need to strictly be a string or a table. Still, it has to be something, and the list to choose from is not that big: number, string, table, function, userdata. Number is pointless, table is already used by a regular callback, function and userdata are an obvious over-complication. As to the list of errors that grilo provides, if you look closely at the list, you'll see that of all the errors that lua-source can possibly throw, most of them are of the "current operation failed" type. "media not found" and "token error" are pretty much the only errors that lua source is expected to throw. Maybe you can add "search null unsupported". Now to provide this list as an easy API... In lua you don't have much compile time (script-load time) checks. There is no way to find out that user made a typo in the error name, whether it's just "some_error" or grl.err.some_error (which is the same as grl.err["some_error"]). We can provide the errors as grl.err[0] grl.err[1] and so on, but that is as non-intuitive as it could get. So I think the best place to "provide" the list of supported errors is in the documentation :) > > (In reply to Victor Toso from comment #15) > > > Review of attachment 320085 [details] [review] [review] [review] [review]: > > > > > > I don't mind a wrapper function to proper deal with the Source name, Lua > > > file + line number but we should using the log-domain from lua-factory for > > > that. > > > > Would be awesome. Problem is, grilo's logdomain is available at grilo's > > compile time only. It is not stored in headers and no API provides it. > > > > I also don't like how the logging is done in this patch. Problem is, I'm > > afraid that's the best we can get without changing grilo's logging API. > > Well, consider this patch a proof-of-concept, it doesn't add any critical > > functionality. > > What is the outcome when you do GRL_DEBUG=lua-factory:* with this patch? I > think the lua-sources should be inside lua-factory domain as I mentioned > before and we can improve that later in the core. If I understand the logic behing grl-log correctly, GRL_DEBUG is totally ignored. This function will print even if you disable grl-log completely. And I don't see any way around it without changing grl-log. Maybe I should move it to separate bug, with the proposed changes to grl-log as well. > > (In reply to Victor Toso from comment #16) > > > Review of attachment 320071 [details] [review] [review] [review] [review]: > > > > > > PS: This should be 3/4 > > > > > > Was this acked before? (So many threads with GOA...) > > > > > > ::: src/lua-factory/grl-lua-library.c > > > @@ +1368,3 @@ > > > +{ > > > + /* we don't really care about results */ > > > + goa_account_call_ensure_credentials_finish (GOA_ACCOUNT (source_object), > > > NULL, res, NULL); > > > > > > I don't know much about goa but why isn't it interesting to check for Errors? > > > > > > I did not get much about what this should do from the docs [0] > > > > > > [0] > > > https://developer.gnome.org/goa/stable/GoaAccount.html#goa-account-call- > > > ensure-credentials > > > > Yes, this one is tricky. Lets assume a situation, when we try to use some > > OAuth token from GOA and find out that it's not working, and we need to tell > > the user about it. Two steps are required. First, the client app should be > > notified that something is wrong with one of the GOA accounts, so that it > > could show a warning to the user, and, ideally, suggest him to launch > > gnome-control-center. That's what GRL_ERROR_AUTHENTICATION_TOKEN is for. The > > second step is actually marking the online account for attention, so that > > when gnome-control-center does get launched, user could see which account > > has failed. For that, there is a function > > goa_account_set_attention_needed(), which is a wrapper for a read-only DBus > > property, so it only works from inside GOA. Clients do not have direct > > access to it. That's when ensure_credentials come into play. I don't know > > why the online doc missed this point, but in the documentation to DBus > > method you can find this piece of info: > > > > If this method fails because the token service indicates that > > authorization has expired, the > > #org.gnome.OnlineAccounts.Account:AttentionNeeded property > > will be set to %TRUE. On the other hand, if this property was > > already %TRUE and this method succeeds, it is set to %FALSE. > > > > [0] > > And this is also the reason why we do not really care about the result. This > > side-effect is the only thing we really need. > > Still, would be nice to check the GError and warn if something is off, no? Actually, "something is off" is the result we are expecting to get. Remember, that function is supposed to be called when the token is not working. Yes, we can get the result "something is off in an unexpected way", but I don't think you have to notify the user further. By the time this function is called the user should already know that "something is off", whether it's the way we expected or not. Compare: "something is wrong with your token, go check GOA" and "something is wrong with your token, go check GOA. also, 'ensure credentials' failed, so go check GOA double time".
(In reply to Morse from comment #20) > (In reply to Victor Toso from comment #19) > > My point is to provide the errors type that Grilo does [0]. The value of > > grl.err.media_not_found does not need to strictly be a string or a table. > > Still, it has to be something, and the list to choose from is not that big: > number, string, table, function, userdata. Feel free to use what suits you best. I just would like to avoid pure strings to identify errors (i.e grl.err.something instead of "something") > Number is pointless If it can identify the error in lua-library it isn't pointless > table is already used by a regular callback Either way you have to check for errors. If it is a table we could have { type = "lua-factory-error" } defined for all and that would be easy to check. PS: type from 0.3.0 is specific to media type so I don't think it would be a problem in the future either > function Yeah, I don't think this one would be good idea. > and userdata are an obvious over-complication. Good point of userdata is that lua should not be able to mess with it. Do we really care? Not really. So, yeah, userdata is no good as well IMHO. > As to the list of errors that > grilo provides, if you look closely at the list, you'll see that of all the > errors that lua-source can possibly throw, most of them are of the "current > operation failed" type. "media not found" and "token error" are pretty much > the only errors that lua source is expected to throw. Maybe you can add > "search null unsupported". Most sources don't even handle those correctly. Point is, IMHO, we should do the right thing in order to provide/verify errors. > Now to provide this list as an easy API... In lua you don't have much > compile time (script-load time) checks. There is no way to find out that > user made a typo in the error name, whether it's just "some_error" or > grl.err.some_error (which is the same as grl.err["some_error"]).We can > provide the errors as grl.err[0] grl.err[1] and so on, but that is as > non-intuitive as it could get. Either way we would need to check in lua-library for a valid error. > I think the best place to "provide" the > list of supported errors is in the documentation :) Agreed. > > What is the outcome when you do GRL_DEBUG=lua-factory:* with this patch? I > > think the lua-sources should be inside lua-factory domain as I mentioned > > before and we can improve that later in the core. > > If I understand the logic behing grl-log correctly, GRL_DEBUG is totally > ignored. This function will print even if you disable grl-log completely. > And I don't see any way around it without changing grl-log. Maybe I should > move it to separate bug, with the proposed changes to grl-log as well. Ok > > > > > > Yes, this one is tricky. Lets assume a situation, when we try to use some > > > OAuth token from GOA and find out that it's not working, and we need to tell > > > the user about it. Two steps are required. First, the client app should be > > > notified that something is wrong with one of the GOA accounts, so that it > > > could show a warning to the user, and, ideally, suggest him to launch > > > gnome-control-center. That's what GRL_ERROR_AUTHENTICATION_TOKEN is for. The > > > second step is actually marking the online account for attention, so that > > > when gnome-control-center does get launched, user could see which account > > > has failed. For that, there is a function > > > goa_account_set_attention_needed(), which is a wrapper for a read-only DBus > > > property, so it only works from inside GOA. Clients do not have direct > > > access to it. That's when ensure_credentials come into play. I don't know > > > why the online doc missed this point, but in the documentation to DBus > > > method you can find this piece of info: > > > > > > If this method fails because the token service indicates that > > > authorization has expired, the > > > #org.gnome.OnlineAccounts.Account:AttentionNeeded property > > > will be set to %TRUE. On the other hand, if this property was > > > already %TRUE and this method succeeds, it is set to %FALSE. > > > > > > [0] > > > And this is also the reason why we do not really care about the result. This > > > side-effect is the only thing we really need. > > > > Still, would be nice to check the GError and warn if something is off, no? > > Actually, "something is off" is the result we are expecting to get. > Remember, that function is supposed to be called when the token is not > working. Yes, we can get the result "something is off in an unexpected way", > but I don't think you have to notify the user further. By the time this > function is called the user should already know that "something is off", > whether it's the way we expected or not. Compare: "something is wrong with > your token, go check GOA" and "something is wrong with your token, go check > GOA. also, 'ensure credentials' failed, so go check GOA double time". Sure, thanks for the explanation. I would suggest to use this rationale in the commit log and/or in the doc of function wrapper.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/74.