GNOME Bugzilla – Bug 778960
Goa-depended notification errors could launch "Settings" (control-center)
Last modified: 2020-11-25 16:20:40 UTC
Created attachment 346268 [details] screenshot When gnome-photos has a problem with the credentials in online-accounts, the notification has a button which launches GNOME Control Center. That would be as helpful as the Details message. See also Bug 777492.
Created attachment 346784 [details] [review] Notify when credentials expire. (In reply to Felipe Borges from comment #0) > Created attachment 346268 [details] > screenshot > > When gnome-photos has a problem with the credentials in online-accounts, the > notification has a button which launches GNOME Control Center. That would be > as helpful as the Details message. > > See also Bug 777492. I saw the implementation of gnome-photos, but in this case I don't think that is necessary create a another type. Anyway If I'm wrong and is needed create a new type, only tell me. Regards! :)
Review of attachment 346784 [details] [review]: Thank you for the patch. It looks very good, and it took me a while to read all of it with attention. There is only one major thing I proposed changes: reuse the SHOW_ERROR_MESSAGE to be able to handle the action too (thus, removing the need of another signal). What do you think? ::: plugins/eds/gtd-plugin-eds.c @@ +142,3 @@ + { + + GoaAccount *account; Space between function name and '(l->data)'. @@ +159,3 @@ + + removed = g_hash_table_remove (self->providers_notified, id); + GtdProviderGoa *provider; This assertion is a bit of an overkill. I think it's reasonable to assume the code is correct, and don't assert it. (But it's a very good thing that you had the care to do this! This is the right path) @@ +167,3 @@ + g_signal_emit_by_name (self, "provider-changed", provider); + inserted = g_hash_table_insert (self->providers_notified, g_strdup (id), g_object_ref (provider)); + Ditto about overkill. ::: src/engine/gtd-manager.c @@ +75,3 @@ LIST_REMOVED, SHOW_ERROR_MESSAGE, + SHOW_ERROR_MESSAGE_WITH_ACTION, I think it'll be better if we just reuse the SHOW_ERROR_MESSAGE signal. API-wise, I think it's correct to allow optionally sending an action with the error message (i.e. we accept NULL actions). ::: src/engine/gtd-manager.h @@ +23,3 @@ +#include "notification/gtd-notification.h" +#include "notification/gtd-notification-widget.h" Better put those types inside 'gtd-types.h' and not include them here. ::: src/provider/gtd-provider-selector.c @@ +72,3 @@ + gpointer id) +{ + spawn (id,NULL); Space before "NULL" @@ +318,3 @@ + + name = gtd_provider_get_name (provider); + msg = g_strdup_printf (_("Your %s crendentials have expired"), name); Since you're allocation a new string here, 'msg' shouldn't be declared as 'const'. Also, this string should be free()'d after use. @@ +322,3 @@ + gtd_manager_emit_error_message_with_action (gtd_manager_get_default (), + msg, + This should be translatable, shouldn't it?
(In reply to Georges Basile Stavracas Neto from comment #2) > Review of attachment 346784 [details] [review] [review]: > > Thank you for the patch. It looks very good, and it took me a while to read Thanks for the review I will work on your proposals, and yeah it was a long patch, I think that should be uploaded in different patches, once for each source file, or type, what do you think ?.
(In reply to Kevin Lopez from comment #3) > I think that should be uploaded in different patches, once for each > source file, or type, what do you think ?. Good idea. Split it in 2 patches: 1. Add the action to 'show-error-message' signal 2. The rest of the changes I guess that'd make your patchset cleaner and more organized. What do you think?
(In reply to Georges Basile Stavracas Neto from comment #4) > (In reply to Kevin Lopez from comment #3) > 1. Add the action to 'show-error-message' signal I don't know if understand it good. I have to modify the show-error-message signal to allow sending an action ? > I guess that'd make your patchset cleaner and more organized. What do you > think? Sure! Regards!
(In reply to Georges Basile Stavracas Neto from comment #2) > Review of attachment 346784 [details] [review] [review]: > I think it'll be better if we just reuse the SHOW_ERROR_MESSAGE signal. > API-wise, I think it's correct to allow optionally sending an action with > the error message (i.e. we accept NULL actions). I' modified show_error_message signal to allow send an action or not, based in an enumerate that has as parameter the signal. > ::: src/engine/gtd-manager.h > @@ +23,3 @@ > > +#include "notification/gtd-notification.h" > +#include "notification/gtd-notification-widget.h" > > Better put those types inside 'gtd-types.h' and not include them here. But I'm stucking here, because I only need the GtdNotificationActionFunc type. And I don't know how to include this type to GtdManager.h in a right way, because the new gtd_manager_emit_error_message has this new function signature void gtd_manager_emit_error_message (GtdManager *manager, const gchar *primary_message, const gchar *secondary_message, GtdManagerShowErrorMessageMode mode, GtdNotificationActionFunc func, gpointer *user_data); To at least compile the app, I added this typedef void (*GtdNotificationActionFunc) (GtdNotification *notification, gpointer user_data); declaration, to gtd-types.h . But I don't like this solution because this type is also defined in gtd-notification.h. How I can solve this ? Thanks. Regards!
(In reply to Kevin Lopez from comment #6) > I' modified show_error_message signal to allow send an action or not, based > in an enumerate that has as parameter the signal. I'm not sure I understood. The idea is that, if the passed action (i.e. function) is NULL, we simply ignore it. > But I'm stucking here, because I only need the GtdNotificationActionFunc > type. > And I don't know how to include this type to GtdManager.h in a right way, > because > the new gtd_manager_emit_error_message has this new function signature > > void gtd_manager_emit_error_message (GtdManager > *manager, > const gchar > *primary_message, > const gchar > *secondary_message, > GtdManagerShowErrorMessageMode mode, > GtdNotificationActionFunc func, > gpointer > *user_data); > > > To at least compile the app, I added this > > typedef void (*GtdNotificationActionFunc) (GtdNotification *notification, > gpointer user_data); > > > declaration, to gtd-types.h . But I don't like this solution because this > type is also > defined in gtd-notification.h. How I can solve this ? Well, ok, let's not include gtd-types.h then. I'm not really sure how to declare an anonymous function pointer typedef either...
(In reply to Georges Basile Stavracas Neto from comment #7) > (In reply to Kevin Lopez from comment #6) > Well, ok, let's not include gtd-types.h then. I'm not really sure how to > declare an anonymous function pointer typedef either... So, what we can do ? Regards.
(In reply to Kevin Lopez from comment #8) > So, what we can do ? See this example: https://git.gnome.org/browse/gnome-todo/tree/src/gtd-task-list-view.c#n170
Created attachment 350614 [details] [review] Patch (In reply to Georges Basile Stavracas Neto from comment #9) > (In reply to Kevin Lopez from comment #8) > > > So, what we can do ? > > See this example: > https://git.gnome.org/browse/gnome-todo/tree/src/gtd-task-list-view.c#n170 Wow, I've never saw a typedef for a function pointer, thanks for the tip, really useful! I'm waiting for the review to start writing the second part, since it depends on this patch. Regards!
Review of attachment 350614 [details] [review]: LGTM.
Comment on attachment 350614 [details] [review] Patch Nice patch! Now the only task left is actually spawn GNOME Control Center at the expired account. Attachment 350614 [details] pushed as 9320437 - gtd-manager: allow pass an action to emit-error-message
Created attachment 350915 [details] [review] Second part of the patch I'm not sure if I selected the best names for the functions and signals etc... Also, regarding the code style, in these statements : if (!attention_needed && provider_notified) g_hash_table_remove (self->providers_notified, id); else if (attention_needed && !provider_notified) { g_signal_emit_by_name (self, "provider-changed", provider); g_hash_table_insert (self->providers_notified, g_strdup (id), g_object_ref (provider)); } I opted for adding braces around the first if. I know that it doesn't follow the rules, but It looks more clear for me. Anyway here's the patch. Regards!
Review of attachment 350915 [details] [review]: A few architectural considerings below. ::: plugins/eds/gtd-plugin-eds.c @@ +47,3 @@ /* Providers */ GList *providers; + GHashTable *providers_notified; Instead of using hash tables, I prefer you to use g_object_set_data() and get_data() instead. @@ +136,3 @@ + GoaAccount *account; + + account = goa_object_peek_account (object); Use an realy return here, if the account does not need attention. @@ +140,3 @@ + for (l = self->providers; l != NULL; l = l->next) + { + if (account == gtd_provider_goa_get_account (l->data)) First, please make this an early continue to avoid too much nesting. Besides that, you have to check if the provider is a GOA provider as well, thus: if (!GTD_IS_PROVIDER_GOA(l->data) || account != get_goa_account (l->data)) continue; @@ +147,3 @@ + const gchar *id; + + provider = GTD_PROVIDER_GOA (l->data); You don't need to cast l->data to GtdProviderGoa. Just use l->data directly in g_signam_emit_by_name(). ::: src/engine/gtd-plugin-manager.c @@ +149,3 @@ + 1, + G_TYPE_POINTER); + Not needed. ::: src/provider/gtd-provider-selector.c @@ +74,3 @@ + spawn (NULL, NULL); +} + This is wrong. This function should be in GtdPluginEds, not here. Also, it actually does nothing. @@ +328,3 @@ + g_free (msg); + +} Again, should be in EDS plugin, not here.
bugzilla.gnome.org is being replaced by gitlab.gnome.org. We are closing all older bug reports and feature requests in GNOME Bugzilla which have not seen updates for a while. If you still use gnome-todo and if you still see this bug / want this feature in a recent and currently supported version, then please feel free to report it at https://gitlab.gnome.org/GNOME/gnome-todo/-/issues/ by following the guidelines at https://wiki.gnome.org/Community/GettingInTouch/BugReportingGuidelines Thank you for creating this report and we are sorry it could not be implemented so far (volunteer workforce and time is limited).