GNOME Bugzilla – Bug 670939
Reminder to change out of office status
Last modified: 2013-04-11 20:49:57 UTC
Created attachment 208547 [details] Out of office reminder Evolution 3.3.91 In evolution-exchange, user gets a pop up of out of office reminder. It might be good to inform user once his given time period for out of office is over.
Created attachment 240458 [details] [review] Bug #670939 - Reminder to change out of office status
Review of attachment 240458 [details] [review]: I cannot compile the code for some reason, I did clean my sources, then applied the patch, then ./autogen.sh && ./configure && make && make install and I get this error during compilation: camel-ews-enumtypes.c:4:33: fatal error: camel-ews-enumtypes.h: No such file or directory compilation terminated. make[3]: *** [libcamelews_la-camel-ews-enumtypes.lo] Error 1 Nonetheless, the overall code reading looks good, just fine-tune it and that's all. Please note that some notices below are applicable to more places than the one I wrote it, like any user visible string should be localized (include <glib/gi18n-lib.h> and then use _("Text to localize")). ::: src/camel/camel-ews-store.c @@ +144,3 @@ + +void +camel_ews_store_set_has_ooo (CamelEwsStore *ews_store, the property is names "has-ooo-set", I know it's kind of unfortunate here, the "set suffix", but it's a good habit to follow property names in function names they touch them. Please fix it (same as the has_ooo function parameter name). @@ +154,3 @@ + ews_store->priv->has_ooo_set = has_ooo; + if (ews_store->priv->has_ooo_set) + g_object_notify (G_OBJECT (ews_store), "has-ooo-set"); g_object_notify() is to notify about property change, thus it should be called always. @@ +174,3 @@ + return; + + ews_store->priv->ooo_alert_state= state; a) space before '=' b) missing g_object_notify() call @@ +602,3 @@ + if (error != NULL) { + g_warning ("Unable to get out of office settings: %s\n", + error->message); The error print is rather a debug thing, isn't it? @@ +656,3 @@ + if (success) { + e_ews_oof_settings_new ( This is called always, not only the first time on connect after offline or fresh start. ::: src/configuration/e-ews-ooo-notificator.c @@ +18,3 @@ + +#ifdef HAVE_CONFIG_H +#include <config.h> Make it #include "config.h" please @@ +70,3 @@ + if (error != NULL) { + g_warning ("Unable to set out of office settings: %s \n", + error->message); This should come to the alert itself, replace the second text or something like that, thus uses know that something was wrong. @@ +92,3 @@ + if (error != NULL) { + g_warning ("Unable to get out of office settings: %s\n", + error->message); This should come to the alert itself, replace the second text or something like that, thus uses know that something was wrong. @@ +98,3 @@ + } + + cancellable = g_cancellable_new (); The cancellable is leaking here and in the below function. There is no gain to pass a cancellable which user is not able to cancel. @@ +185,3 @@ + + account_name = camel_service_get_display_name(CAMEL_SERVICE (ews_store)); + msg = g_strdup_printf ("Your account \"%s\" has the status set as " Localize the strings. @@ +200,3 @@ + "ooo-unset-on-server", + "Unset on Server", + "Change the status to \"In the Office\"", Localize the strings (both above) @@ +210,3 @@ + g_hash_table_insert (extension->priv->alerts, ews_store, alert); + e_alert_sink_submit_alert (E_ALERT_SINK (shell_content), alert); + g_free(msg); missing space after g_free. @@ +246,3 @@ + + ews_store = CAMEL_EWS_STORE (service); + ews_ooo_notificator_show_notification (ews_store, extension); it should not show notification, but it should tell the ews_store that it can recheck OOO state (it should, ideally, set that on disable of the service). Also, common practice is to have as the first parameter the object to which the function is dedicated, where here it's the 'extension', not 'ews_store'. @@ +335,3 @@ + if (!connection) + return; + e_ews_oof_settings_new ( do not do this at all, this is already done on connection in CamelEwsStore @@ +387,3 @@ + session = e_mail_backend_get_session (E_MAIL_BACKEND (backend)); + account_store = e_mail_ui_session_get_account_store ( + E_MAIL_UI_SESSION (session)); no need to split on two lines @@ +403,3 @@ + ews_store = CAMEL_EWS_STORE (service); + connection = camel_ews_store_ref_connection (ews_store); + if (connection) { what is this? CamelEwsStore derives from CamelOfflineStore, which has camel_offline_store_get_online(), but you do not need that. What you need is to connect to "notify::has-ooo-set", and then check whether the current value is set to TRUE, and the notification isn't dismissed, and then show the bar (basically call ews_ooo_notificator_oof_state_cb()). @@ +406,3 @@ + ews_ooo_notificator_show_notification (ews_store, extension); + g_object_unref (connection); + } else { As noted above, the below should be done always, not only if the ews_store doesn't have connection. ::: src/configuration/e-ews-ooo-notificator.h @@ +22,3 @@ +#include <libebackend/libebackend.h> +#include <mail/e-mail-ui-session.h> +#include <shell/e-shell.h> the two includes above might not be needed here, right? @@ +59,3 @@ + +GType e_ews_ooo_notificator_get_type + (void) G_GNUC_CONST; a) why is it split, it obviously fits the above line b) I try to avoid G_GNUC_CONST here (partly personal preference, partly a feeling that I was told some time ago that it can cause trouble, but I can be wrong with that) ::: src/configuration/module-ews-configuration.error.xml @@ +22,3 @@ </error> + <error type="info" id="has-ooo-set"> you can setup primary and secondary text to the alert here, and use the {0} replacement like it is used above, rather than write the default text in the source code. ::: src/server/Makefile.am @@ +7,3 @@ ENUM_TYPES = e-ews-enums.h + forgotten extra empty line?
(In reply to comment #2) > Review of attachment 240458 [details] [review]: > > I cannot compile the code for some reason, I did clean my sources, then applied > the patch, then ./autogen.sh && ./configure && make && make install and I get > this error during compilation: > camel-ews-enumtypes.c:4:33: fatal error: camel-ews-enumtypes.h: No such file > or directory > compilation terminated. > make[3]: *** [libcamelews_la-camel-ews-enumtypes.lo] Error 1 Fixed. > > Nonetheless, the overall code reading looks good, just fine-tune it and that's > all. Please note that some notices below are applicable to more places than the > one I wrote it, like any user visible string should be localized (include > <glib/gi18n-lib.h> and then use _("Text to localize")). > > ::: src/camel/camel-ews-store.c > @@ +144,3 @@ > + > +void > +camel_ews_store_set_has_ooo (CamelEwsStore *ews_store, > > the property is names "has-ooo-set", I know it's kind of unfortunate here, the > "set suffix", but it's a good habit to follow property names in function names > they touch them. Please fix it (same as the has_ooo function parameter name). Fixed! > > @@ +154,3 @@ > + ews_store->priv->has_ooo_set = has_ooo; > + if (ews_store->priv->has_ooo_set) > + g_object_notify (G_OBJECT (ews_store), "has-ooo-set"); > > g_object_notify() is to notify about property change, thus it should be called > always. Fixed! > > @@ +174,3 @@ > + return; > + > + ews_store->priv->ooo_alert_state= state; > > a) space before '=' > b) missing g_object_notify() call > > @@ +602,3 @@ > + if (error != NULL) { > + g_warning ("Unable to get out of office settings: %s\n", > + error->message); > > The error print is rather a debug thing, isn't it? working on ... > > @@ +656,3 @@ > > + if (success) { > + e_ews_oof_settings_new ( > > This is called always, not only the first time on connect after offline or > fresh start. Fixed. Calling only if the state is _IGNORED (the initial state). > > ::: src/configuration/e-ews-ooo-notificator.c > @@ +18,3 @@ > + > +#ifdef HAVE_CONFIG_H > +#include <config.h> > > Make it #include "config.h" please > > @@ +70,3 @@ > + if (error != NULL) { > + g_warning ("Unable to set out of office settings: %s \n", > + error->message); > > This should come to the alert itself, replace the second text or something like > that, thus uses know that something was wrong. Working on ... > > @@ +92,3 @@ > + if (error != NULL) { > + g_warning ("Unable to get out of office settings: %s\n", > + error->message); > > This should come to the alert itself, replace the second text or something like > that, thus uses know that something was wrong. Working on ... > > @@ +98,3 @@ > + } > + > + cancellable = g_cancellable_new (); > > The cancellable is leaking here and in the below function. There is no gain to > pass a cancellable which user is not able to cancel. Fixed! > > @@ +185,3 @@ > + > + account_name = camel_service_get_display_name(CAMEL_SERVICE (ews_store)); > + msg = g_strdup_printf ("Your account \"%s\" has the status set as " > > Localize the strings. Fixed! > > @@ +200,3 @@ > + "ooo-unset-on-server", > + "Unset on Server", > + "Change the status to \"In the Office\"", > > Localize the strings (both above) Fixed! > > @@ +210,3 @@ > + g_hash_table_insert (extension->priv->alerts, ews_store, alert); > + e_alert_sink_submit_alert (E_ALERT_SINK (shell_content), alert); > + g_free(msg); > > missing space after g_free. Fixed! > > @@ +246,3 @@ > + > + ews_store = CAMEL_EWS_STORE (service); > + ews_ooo_notificator_show_notification (ews_store, extension); > > it should not show notification, but it should tell the ews_store that it can > recheck OOO state (it should, ideally, set that on disable of the service). Milan, if I understood what you said, I should, disabling the service, if the alert_state was not _CANCELED by the user, just reset the alert_state to _IGNORED (the initial one). Is it? If yes, it is currently done by hide_notification function. But it doesn't help so much if we don't (try to) show the notification here, once when an account is enabled CamelEwsStore::ews_connect_sync (where we verify for the OOO state) is not called anymore. > Also, common practice is to have as the first parameter the object to which the > function is dedicated, where here it's the 'extension', not 'ews_store'. Fixed! > > @@ +335,3 @@ > + if (!connection) > + return; > + e_ews_oof_settings_new ( > > do not do this at all, this is already done on connection in CamelEwsStore Fixed! > > @@ +387,3 @@ > + session = e_mail_backend_get_session (E_MAIL_BACKEND (backend)); > + account_store = e_mail_ui_session_get_account_store ( > + E_MAIL_UI_SESSION (session)); > > no need to split on two lines Fixed! > > @@ +403,3 @@ > + ews_store = CAMEL_EWS_STORE (service); > + connection = camel_ews_store_ref_connection (ews_store); > + if (connection) { > > what is this? CamelEwsStore derives from CamelOfflineStore, which has > camel_offline_store_get_online(), but you do not need that. What you need is to > connect to "notify::has-ooo-set", and then check whether the current value is > set to TRUE, and the notification isn't dismissed, and then show the bar > (basically call ews_ooo_notificator_oof_state_cb()). > Fixed! > @@ +406,3 @@ > + ews_ooo_notificator_show_notification (ews_store, extension); > + g_object_unref (connection); > + } else { > > As noted above, the below should be done always, not only if the ews_store > doesn't have connection. > Fixed! > ::: src/configuration/e-ews-ooo-notificator.h > @@ +22,3 @@ > +#include <libebackend/libebackend.h> > +#include <mail/e-mail-ui-session.h> > +#include <shell/e-shell.h> > > the two includes above might not be needed here, right? Right. They should be in .c file. Fixed! > > @@ +59,3 @@ > + > +GType e_ews_ooo_notificator_get_type > + (void) G_GNUC_CONST; > > a) why is it split, it obviously fits the above line > b) I try to avoid G_GNUC_CONST here (partly personal preference, partly a > feeling that I was told some time ago that it can cause trouble, but I can be > wrong with that) a) and b) fixed! > > ::: src/configuration/module-ews-configuration.error.xml > @@ +22,3 @@ > </error> > > + <error type="info" id="has-ooo-set"> > > you can setup primary and secondary text to the alert here, and use the {0} > replacement like it is used above, rather than write the default text in the > source code. Yeah, I put tje primary string here, using {0} as suggested. Fixed! > > ::: src/server/Makefile.am > @@ +7,3 @@ > ENUM_TYPES = e-ews-enums.h > > + > > forgotten extra empty line? Yes, fixed!
(In reply to comment #3) > (In reply to comment #2) > > Review of attachment 240458 [details] [review] [details]: > > > > I cannot compile the code for some reason, I did clean my sources, then applied > > the patch, then ./autogen.sh && ./configure && make && make install and I get > > this error during compilation: > > camel-ews-enumtypes.c:4:33: fatal error: camel-ews-enumtypes.h: No such file > > or directory > > compilation terminated. > > make[3]: *** [libcamelews_la-camel-ews-enumtypes.lo] Error 1 > > Fixed. > > > > > Nonetheless, the overall code reading looks good, just fine-tune it and that's > > all. Please note that some notices below are applicable to more places than the > > one I wrote it, like any user visible string should be localized (include > > <glib/gi18n-lib.h> and then use _("Text to localize")). > > > > ::: src/camel/camel-ews-store.c > > @@ +144,3 @@ > > + > > +void > > +camel_ews_store_set_has_ooo (CamelEwsStore *ews_store, > > > > the property is names "has-ooo-set", I know it's kind of unfortunate here, the > > "set suffix", but it's a good habit to follow property names in function names > > they touch them. Please fix it (same as the has_ooo function parameter name). > > Fixed! > > > > > @@ +154,3 @@ > > + ews_store->priv->has_ooo_set = has_ooo; > > + if (ews_store->priv->has_ooo_set) > > + g_object_notify (G_OBJECT (ews_store), "has-ooo-set"); > > > > g_object_notify() is to notify about property change, thus it should be called > > always. > > Fixed! > > > > > @@ +174,3 @@ > > + return; > > + > > + ews_store->priv->ooo_alert_state= state; > > > > a) space before '=' > > b) missing g_object_notify() call > > > > @@ +602,3 @@ > > + if (error != NULL) { > > + g_warning ("Unable to get out of office settings: %s\n", > > + error->message); > > > > The error print is rather a debug thing, isn't it? > > working on ... > > > > > @@ +656,3 @@ > > > > + if (success) { > > + e_ews_oof_settings_new ( > > > > This is called always, not only the first time on connect after offline or > > fresh start. > > Fixed. Calling only if the state is _IGNORED (the initial state). > > > > > ::: src/configuration/e-ews-ooo-notificator.c > > @@ +18,3 @@ > > + > > +#ifdef HAVE_CONFIG_H > > +#include <config.h> > > > > Make it #include "config.h" please > > > > @@ +70,3 @@ > > + if (error != NULL) { > > + g_warning ("Unable to set out of office settings: %s \n", > > + error->message); > > > > This should come to the alert itself, replace the second text or something like > > that, thus uses know that something was wrong. > > Working on ... > > > > > @@ +92,3 @@ > > + if (error != NULL) { > > + g_warning ("Unable to get out of office settings: %s\n", > > + error->message); > > > > This should come to the alert itself, replace the second text or something like > > that, thus uses know that something was wrong. > > Working on ... > > > > > @@ +98,3 @@ > > + } > > + > > + cancellable = g_cancellable_new (); > > > > The cancellable is leaking here and in the below function. There is no gain to > > pass a cancellable which user is not able to cancel. > > Fixed! > > > > > @@ +185,3 @@ > > + > > + account_name = camel_service_get_display_name(CAMEL_SERVICE (ews_store)); > > + msg = g_strdup_printf ("Your account \"%s\" has the status set as " > > > > Localize the strings. > > Fixed! > > > > > @@ +200,3 @@ > > + "ooo-unset-on-server", > > + "Unset on Server", > > + "Change the status to \"In the Office\"", > > > > Localize the strings (both above) > > Fixed! > > > > > @@ +210,3 @@ > > + g_hash_table_insert (extension->priv->alerts, ews_store, alert); > > + e_alert_sink_submit_alert (E_ALERT_SINK (shell_content), alert); > > + g_free(msg); > > > > missing space after g_free. > > Fixed! > > > > > @@ +246,3 @@ > > + > > + ews_store = CAMEL_EWS_STORE (service); > > + ews_ooo_notificator_show_notification (ews_store, extension); > > > > it should not show notification, but it should tell the ews_store that it can > > recheck OOO state (it should, ideally, set that on disable of the service). > > Milan, if I understood what you said, I should, disabling the service, if the > alert_state was not _CANCELED by the user, just reset the alert_state to > _IGNORED (the initial one). Is it? If yes, it is currently done by > hide_notification function. But it doesn't help so much if we don't (try to) > show the notification here, once when an account is enabled > CamelEwsStore::ews_connect_sync (where we verify for the OOO state) is not > called anymore. I got your point! But we have 2 scenarios here: 1 - Start evolution with the EWS account disabled and then enable the account Your approach will works perfectly in this case. 2 - Start evolution with the EWS account enabled, disable the account and then re-enable it. In this case CamelEwsStore::ews_connect_sync won't be called for the re-enable and we need to check if we should show or not the notification. Please, let me know about your thoughts, > > > Also, common practice is to have as the first parameter the object to which the > > function is dedicated, where here it's the 'extension', not 'ews_store'. > > Fixed! > > > > > @@ +335,3 @@ > > + if (!connection) > > + return; > > + e_ews_oof_settings_new ( > > > > do not do this at all, this is already done on connection in CamelEwsStore > > Fixed! > > > > > @@ +387,3 @@ > > + session = e_mail_backend_get_session (E_MAIL_BACKEND (backend)); > > + account_store = e_mail_ui_session_get_account_store ( > > + E_MAIL_UI_SESSION (session)); > > > > no need to split on two lines > > Fixed! > > > > > @@ +403,3 @@ > > + ews_store = CAMEL_EWS_STORE (service); > > + connection = camel_ews_store_ref_connection (ews_store); > > + if (connection) { > > > > what is this? CamelEwsStore derives from CamelOfflineStore, which has > > camel_offline_store_get_online(), but you do not need that. What you need is to > > connect to "notify::has-ooo-set", and then check whether the current value is > > set to TRUE, and the notification isn't dismissed, and then show the bar > > (basically call ews_ooo_notificator_oof_state_cb()). > > > > Fixed! > > > @@ +406,3 @@ > > + ews_ooo_notificator_show_notification (ews_store, extension); > > + g_object_unref (connection); > > + } else { > > > > As noted above, the below should be done always, not only if the ews_store > > doesn't have connection. > > > > Fixed! > > > ::: src/configuration/e-ews-ooo-notificator.h > > @@ +22,3 @@ > > +#include <libebackend/libebackend.h> > > +#include <mail/e-mail-ui-session.h> > > +#include <shell/e-shell.h> > > > > the two includes above might not be needed here, right? > > Right. They should be in .c file. > Fixed! > > > > > @@ +59,3 @@ > > + > > +GType e_ews_ooo_notificator_get_type > > + (void) G_GNUC_CONST; > > > > a) why is it split, it obviously fits the above line > > b) I try to avoid G_GNUC_CONST here (partly personal preference, partly a > > feeling that I was told some time ago that it can cause trouble, but I can be > > wrong with that) > > a) and b) fixed! > > > > > ::: src/configuration/module-ews-configuration.error.xml > > @@ +22,3 @@ > > </error> > > > > + <error type="info" id="has-ooo-set"> > > > > you can setup primary and secondary text to the alert here, and use the {0} > > replacement like it is used above, rather than write the default text in the > > source code. > > Yeah, I put tje primary string here, using {0} as suggested. > Fixed! > > > > > ::: src/server/Makefile.am > > @@ +7,3 @@ > > ENUM_TYPES = e-ews-enums.h > > > > + > > > > forgotten extra empty line? > > Yes, fixed!
(In reply to comment #4) > 1 - Start evolution with the EWS account disabled and then enable the account > Your approach will works perfectly in this case. > > 2 - Start evolution with the EWS account enabled, disable the account and then > re-enable it. > In this case CamelEwsStore::ews_connect_sync won't be called for the re-enable > and we need to check if we should show or not the notification. Evolution will connect to the server for you, there is no need to connect before hand. Just let the most things be done by someone else, not by the notificator. From comment #3 on this it feels like the function names you chose are confusing. When a function is named ews_ooo_notificator_show_notification() then I expect from it to "show notification", not to do anything much more than UI interaction. Similar for hide_notification().
Created attachment 240754 [details] [review] Bug #670939 - Reminder to change out of office status
Created attachment 240791 [details] [review] Bug #670939 - Reminder to change out of office status
Review of attachment 240791 [details] [review]: A quick testing (and slower patch reading) shows the functionality is working, only make the final changes as noted below. I also get this warning on evolution close: > WARNING **: Shell not finalized on exit Basically, you do not need to ref the shell, it's there for the whole application life time. I would not ref the account store too, it might not be needed - you can query for it in dispose() call the same way you did in the constructed. Please reorder the notificator GObject functions. Currently, you have: ews_ooo_notificator_constructed ews_ooo_notificator_dispose e_ews_ooo_notificator_init e_ews_ooo_notificator_finalize Make it: e_ews_ooo_notificator_init ews_ooo_notificator_constructed ews_ooo_notificator_dispose e_ews_ooo_notificator_finalize which is basically the order in which they are invoked by the GObject system. The finalize has for some reason the e_... prefix, while the other constructed and dispose not. I'm fine to use the prefix on all of them or none, just not here yes and there not. ::: src/camel/camel-ews-store.c @@ +626,3 @@ + break; + case E_EWS_OOF_STATE_DISABLED: + case E_EWS_OOF_STATE_SCHEDULED: Might not it set has-ooo-set to FALSE here? @@ +632,3 @@ + +exit: + g_object_unref (oof_settings); if you get here with the 'goto' above, then the ooof_setting is most likely NULL. @@ +649,3 @@ + data->error = error; + + e_ews_oof_settings_new ( You cannot do an asynchronous call here, the callback is called in a dedicated thread, and once leave the function the 'cancellable' and 'error' are invalidated. Thus call sync variant of the function here and do everything necessary here. @@ +690,3 @@ + state = camel_ews_store_get_ooo_alert_state (ews_store); + if (state == CAMEL_EWS_STORE_OOO_ALERT_STATE_UNKNOWN) + camel_session_submit_job ( It seems to me like we should provide one more state, and CAMEL_EWS_STORE_OOO_ALERT_STATE_CHECKED, thus it's not tested unexpectedly and repeatedly in the background. @@ +692,3 @@ + camel_session_submit_job ( + session, + (CamelSessionCallback) ews_update_has_ooo_set, Please do not typecast here. If the callback prototype will change you'll never notice it here (aka you lost a compiler warning benefit by this typecast). @@ +2810,3 @@ + state = e_ews_oof_settings_get_state (oof_settings); + if (state == E_EWS_OOF_STATE_DISABLED) + goto exit; missing camel_ews_store_unset_oof_settings_state_data_free (data); call @@ +2820,3 @@ + +exit: + g_object_unref (oof_settings); Similar to ews_connect_sync_oof_settings_new_cb(), the oof_settings can be NULL here. @@ +2838,3 @@ + + connection = camel_ews_store_ref_connection (ews_store); + e_ews_oof_settings_new ( The same comment as in ews_update_has_ooo_set() @@ +2851,3 @@ + GCancellable *cancellable, + GError **error) +{ I do not see this being called with other than "..., NULL, NULL)" parameters, thus let's remove the cancellable and error parameters and make the body simpler. @@ +2863,3 @@ + camel_session_submit_job ( + session, + (CamelSessionCallback) ews_store_unset_oof_settings_state, Like above, no typecasting, please. ::: src/configuration/e-ews-ooo-notificator.c @@ +114,3 @@ + +static void +ews_ooo_notificator_show_notification (EEwsOooNotificator *extension, CamelEwsStore *ews_store) { place bracket '{' on a new line @@ +152,3 @@ + + /* If the user doesn't cancel the notify, it will be hide automatically in 5 minutes */ + g_timeout_add_seconds (300, ews_ooo_notificator_hide_notification_by_timeout_cb, data); you should keep track of the returned ID and remove it with g_source_remove() on either alert close or dispose of the notificator. You can also use g_timeout_add_seconds_full() with added GDestroyNotify for 'data'. That way neither the extension, nor ews_store will be kept alive for the next 5 minutes regardless of its usage in the rest of the application. I agree that your checking in the callback is fine to have it this way too, I only think that the g_source_remove() way is cleaner. @@ +223,3 @@ + ews_store, "notify::has-ooo-set", + G_CALLBACK (ews_ooo_notificator_has_ooo_set_cb), extension); + extension->priv->stores = g_list_append (extension->priv->stores, ews_store); you unref in service-removed, but you do not ref it here. @@ +236,3 @@ + CamelEwsStore *ews_store = l->data; + + if (e_shell_get_online (shell)) Do this above the cycle, it doesn't make sense to check x-times whether shell is online when it can be done only once. Maybe you wanted to achieve of something else here? @@ +295,3 @@ + ews_store, "notify::has-ooo-set", + G_CALLBACK (ews_ooo_notificator_has_ooo_set_cb), extension); + extension->priv->stores = g_list_append (extension->priv->stores, ews_store); you should ref ews_store here too @@ +333,3 @@ + + if (extension->priv->account_store) { + g_signal_handlers_disconnect_by_func ( you can use g_signal_handlers_disconnect_by_data() @@ +370,3 @@ + extension = E_EWS_OOO_NOTIFICATOR (object); + + if (extension->priv->alerts != NULL) { I would do all the below memory free in dispose too. @@ +389,3 @@ + extension->priv->stores = NULL; + } +} forgotten parent's finalize call.
Please note that I'm fine with functionality, the core of it, the above is just about polishing and fixes on the session jobs.
(In reply to comment #8) > Review of attachment 240791 [details] [review]: > > A quick testing (and slower patch reading) shows the functionality is working, > only make the final changes as noted below. > > I also get this warning on evolution close: > > WARNING **: Shell not finalized on exit > Basically, you do not need to ref the shell, it's there for the whole > application life time. I would not ref the account store too, it might not be > needed - you can query for it in dispose() call the same way you did in the > constructed. I just think is much more straightforward keep the mail account reference than get a view, check name, get backend and then get the mail account. If you really prefer in this way, okay. > @@ +236,3 @@ > + CamelEwsStore *ews_store = l->data; > + > + if (e_shell_get_online (shell)) > > Do this above the cycle, it doesn't make sense to check x-times whether shell > is online when it can be done only once. Maybe you wanted to achieve of > something else here? I was re-enabling the notification inside the if. But, as we talked about, it is not necessary. Thanks for the catch.
(In reply to comment #10) > (In reply to comment #8) > > I would not ref the account store too, it might not be > > needed - you can query for it in dispose() call the same way you did in the > > constructed. > > I just think is much more straightforward keep the mail account reference than > get a view, check name, get backend and then get the mail account. > If you really prefer in this way, okay. I do not have any strong opinion on the EMailAccountStore references, you can keep it as is, just do not ref the EShell.
Created attachment 241126 [details] [review] Bug #670939 - Reminder to change out of office status
I got a crash with the above patch after: a) run evolution b) enter an EWS folder, a notification is shown c) close the notification with 'X' d) right click account name and pick Properties, verify on the OOO page that it's still set; cancel the dialog e) right click account name and pick Disable
+ Trace 231760
The next evolution start I got this crash. It's because doing gtk UI calls in a non-UI (main) thread.
+ Trace 231761
Thread 12 (Thread 0x7ffc33fff700 (LWP 2401))
Review of attachment 241126 [details] [review]: Two quick concerns below, apart of the crashes. ::: src/camel/camel-ews-store.c @@ +601,3 @@ + GError *local_error = NULL; + + camel_operation_push_message (cancellable, _("Updating has-ooo-set")); Good it's localized, because it's a user visible string, but most (if not all) translators will be confused hot to translate it to their target language. Could you make it a real sentence, similar to the one used in account Properties? ("Retrieving \"Out of Office\" settings", with s/Retrieving/Checking/) @@ +606,3 @@ + if (local_error != NULL) { + g_propagate_error (error, local_error); + g_clear_error (&local_error); Check documentation to g_propagate_error(), it 'eats' local_error, thus freeing it will lead to a use-after-free error and possibly crash as well. Please fix all similar places.
Created attachment 241206 [details] [review] Bug #670939 - Reminder to change out of office status
Created attachment 241227 [details] [review] Bug #670939 - Reminder to change out of office status
Review of attachment 241227 [details] [review]: When I click "Unset on Server" I get this warning on console: > GLib-CRITICAL **: g_hash_table_remove_internal: assertion `hash_table != NULL' failed and evolution eventually crashes (it didn't for the first time, but did the second time).
+ Trace 231765
::: src/camel/camel-ews-store.c @@ +606,3 @@ + if (local_error != NULL) { + g_propagate_error (error, local_error); + g_clear_error (&local_error); you forgot of this ::: src/camel/camel-ews-store.h @@ +103,3 @@ + (CamelEwsStore *ews_store, + CamelEwsStoreOooAlertState state); +CamelEwsStoreOooAlertState camel_ews_store_get_ooo_alert_state put the function name on a new line, aligned with other function names ::: src/configuration/e-ews-ooo-notificator.c @@ +57,3 @@ + E_TYPE_EXTENSION) + +/* Forward declarations */ are all these forward declarations needed? They do not seem to me. @@ +126,3 @@ + + if (data->extension->priv->timeout_id) + g_source_remove (data->extension->priv->timeout_id); make sure you'll set the timeout_id to 0 after removal too @@ +193,3 @@ + + /* If the user doesn't cancel the notify, it will be hide automatically in 5 minutes */ + extension->priv->timeout_id = g_timeout_add_seconds_full ( check if timout_id != 0, and if not, then remove and reschedule @@ +241,3 @@ + data->extension->priv->stores = g_list_remove ( + data->extension->priv->stores, data->ews_store); + g_object_unref (data->ews_store); can it happen that the data->ews_store is no inside data->extension->priv->stores? if yes, then you are removing a reference which doesn't belong to data->extension->priv->stores. @@ +265,3 @@ +/* + * GTK+ UI calls cannot be done in a dedicated thread. So, let's ensure that our functions (that do something in the + * UI) will run in the main thread, calling them through g_time_add_full(). typo in g_time_add_full(); you can split the comment into 3 lines too, with "better" wrapping ::: src/configuration/e-ews-ooo-notificator.h @@ +57,3 @@ + +GType e_ews_ooo_notificator_get_type + (void); this can be on the same line as the _get_type, right? I would not be shy and add one more tab too, thus it's 2 lines for two functions, not 4 lines. :) ::: src/configuration/module-ews-configuration.error.xml @@ +23,3 @@ + <error type="info" id="has-ooo-set"> + <_primary>Your account "{0}" has the status set as "Out of Office"</_primary> Your Exchange account "{0}" has the status set as "Out of Office". Do you want to unset it on the server?
(In reply to comment #18) > Review of attachment 241227 [details] [review]: > > can it happen that the data->ews_store is no inside > data->extension->priv->stores? if yes, then you are removing a reference which > doesn't belong to data->extension->priv->stores. No, it cannot happen. We are keeping track of all stores added and remove, don't worry.
Created attachment 241278 [details] [review] Bug #670939 - Reminder to change out of office status
Review of attachment 241278 [details] [review]: Looks good, please commit to master.
Please, before committing, drop the .template files and use those I just introduced in the commit for bug #681837. The only thing you'll do will be to change your Makefile.am rules similar as I did in there. Thanks. (It's just a coincidence that I finally figured out the issue with enums.) :)
Pushed! https://git.gnome.org/browse/evolution-ews/commit/?id=952805f476bee0a7eae4092aec690b8923d1514a