GNOME Bugzilla – Bug 701653
Send To option is still displayed on item right click even if there is no email client
Last modified: 2013-06-07 21:55:24 UTC
The Send To option is still available to the user if there is no email client installed on the system. Furthermore if the Send To option is selected the current nautilus window crashes since nautilus-sendto is executed even though there is no mailer for it to start.
Created attachment 246087 [details] [review] My proposed fix for this issue
Review of attachment 246087 [details] [review]: Thanks for the patch! Looks generally good to me, with some comments below. ::: nautilus-sendto-extension/nautilus-nste.c @@ +74,3 @@ return NULL; + + if (!check_available_mailer ()) Indentation should follow the surrounding coding style. @@ +171,3 @@ + GAppInfo *app_info; + + app_info = g_app_info_get_default_for_uri_scheme ("mailto"); You need to unref app_info when not NULL - you can use g_clear_object(&app_info); that also takes care of the NULL check for you. @@ +174,3 @@ + if (app_info) + { + GAppInfo *app_info; I don't think you need to explicitly check the name here; just return FALSE if app_info is NULL.
Created attachment 246133 [details] [review] Updated patch
Thanks for the review! I uploaded the modified patch. I think it's worthwhile to keep the name-check since nautilus-sendto only supports and recognizes these 4 email clients (any others are marked as unknown). And even though it would still try to launch an email client even if it's unknown, this usually leads to a crash (since different email clients need different parameters in order to be launched from command line).
(In reply to comment #4) > Thanks for the review! I uploaded the modified patch. I think it's worthwhile > to keep the name-check since nautilus-sendto only supports and recognizes these > 4 email clients (any others are marked as unknown). And even though it would > still try to launch an email client even if it's unknown, this usually leads to > a crash (since different email clients need different parameters in order to be > launched from command line). Thanks for the updated patch! I don't want to keep those special cases in the extension code - nautilus-sendto should be made more robust not to crash when spawning an unknown mailer if that's problematic for any reason.
Review of attachment 246133 [details] [review]: ::: nautilus-sendto-extension/nautilus-nste.c @@ +65,3 @@ + if (app_info) { + char *mailer_name = g_app_info_get_name (app_info); + GAppInfo *app_info; As for previous comment, please remove these checks, nautilus-sendto should possibly deal better with unknown mailers instead of crashing if that's problematic. @@ +75,3 @@ + else + return FALSE; + if (g_strcmp0 (mailer_name, "Evolution") == 0) This line of code will never be reached, as you always return before getting here in the function.
Created attachment 246238 [details] [review] Updated patch with removed string checks
(In reply to comment #6) > Review of attachment 246133 [details] [review]: > > ::: nautilus-sendto-extension/nautilus-nste.c > @@ +65,3 @@ > + if (app_info) { > + char *mailer_name = g_app_info_get_name (app_info); > + GAppInfo *app_info; > > As for previous comment, please remove these checks, nautilus-sendto should > possibly deal better with unknown mailers instead of crashing if that's > problematic. > > @@ +75,3 @@ > + else > + return FALSE; > + if (g_strcmp0 (mailer_name, "Evolution") == 0) > > This line of code will never be reached, as you always return before getting > here in the function. I've removed the string checks in the new patch. Thanks, Pam
Review of attachment 246238 [details] [review]: Looks good, thanks! Do you have a GNOME git account?
Sorry, no. Would you mind applying the patch?
Pushed to master for you, thanks.