Created attachment 246088 [details] [review]
My proposed fix for this issue

Review of attachment 246088 [details] [review]:

It is not an evince task to check whether there is an email client or not.  Email is one way to send documents, but nautilus-sendto provides other means to send a document, for instance, sharing the folder where the documents is stored, via bluetooth, etc.

Therefore, the fix should be in nautilus-sendto.

::: shell/ev-window.c
@@ +409,3 @@
+			return TRUE;
+		else if (g_strcmp0 (mailer_name, "Thunderbird") == 0)
+			return TRUE;

Any list is not going to be comprehensive.  There are many mail clients out there.

What is the application that crashes? nautilus-sendto or evince?  If the later, this bug belongs there.

Nevertheless, without a stack trace from the crash it's very hard to determine what caused it.  Can you get us a stack trace? Please see http://live.gnome.org/GettingTraces for more information on how to do so. Thanks in advance!

(In reply to comment #3)
> What is the application that crashes? nautilus-sendto or evince?  If the later,
> this bug belongs there.
> 
> Nevertheless, without a stack trace from the crash it's very hard to determine
> what caused it.  Can you get us a stack trace? Please see
> http://live.gnome.org/GettingTraces for more information on how to do so.
> Thanks in advance!

Currently nautilus-sendto only supports sharing via email (all other API like bluetooth, chat client etc has been deprecated). Also it only supports the email clients listed in my patch (using others is possible, but 80% of the time leads to a crash). Maybe this should be taken into account and the "Send to" option should be enabled/disabled depending on the availability of a compatible email client on the system. Otherwise the availability of an option that does nothing might confuse the users and might be interpreted as feature failure. 

Here is the requested backtrace:
(nautilus-sendto:4376): GLib-CRITICAL **: g_strrstr: assertion 'haystack != NULL' failed

(nautilus-sendto:4376): GLib-CRITICAL **: g_strrstr: assertion 'haystack != NULL' failed

(nautilus-sendto:4376): GLib-CRITICAL **: g_spawn_command_line_async: assertion 'command_line != NULL' failed

Thank you for your time

-> nautilus-sendto

This is definitely not a nautilus-sendto issue. If there is no email client nautilus-sendto simply exits (even though it leaves the backtrace I provided), which make sense, but in this case I think Evince shouldn't try to launch it. A reasonable way to avoid calling nautilus-sendto when there is no email client available would be to simply disable the Send To option.

-> nautilus-sendto.  Please specify which version of it you have.

The critical warnings clearly come from nautilus-sendto. 

As for the issue with disabling the button if nautilus-sendto has no ways to send, IMHO it's not a bug that we don't do this.

(In reply to comment #7)
> [...]
> As for the issue with disabling the button if nautilus-sendto has no ways to
> send, IMHO it's not a bug that we don't do this.

I think they meant to say that evince should disable the menu "Send to" is there is no email client installed.

In such case, evince should not duplicate nautilus-sendto code (as the patch proposed).  It would be better if we could query if nautilus-sendto has any target available.  Yet again, not something that is in evince.

Review of attachment 246088 [details] [review]:

::: shell/ev-window.c
@@ +399,3 @@
+	GAppInfo *app_info;
+
+  	app_info = g_app_info_get_default_for_uri_scheme ("mailto");

app_info != NULL is enough.
You're also leaking GAppInfo.

I've pushed a fix for nautilus-sendto not to generate warnings when there's no mailer installed.

Given that nautilus-sendto only sends files via email now, it might be easier to just check for a mailto handler as the original patch did (though there's too much code in it).

*** Bug 701357 has been marked as a duplicate of this bug. ***

Created attachment 263812 [details] [review]
Don't allow "Send To" when no mail client available

I've simplified the patch a little. It just checks the availability of handler of "mailto" scheme as advised by Bastien.

Review of attachment 263812 [details] [review]:

That looks correct to me.

Isn't it possible to use nautilus sendto for other things than email? Why do we disable sendto feature only because it's not possible to send an email? or is an email client a requirement for nautilus sendto to work? why?

(In reply to comment #14)
> Isn't it possible to use nautilus sendto for other things than email?

No.

> Why do we
> disable sendto feature only because it's not possible to send an email? or is
> an email client a requirement for nautilus sendto to work? why?

Because that's the only thing it does.

I think we should rename the action to something like send as email or something like that then.

Review of attachment 263812 [details] [review]:

::: shell/ev-window.c
@@ +463,3 @@
 	}
 
+	app_info = g_app_info_get_default_for_uri_scheme ("mailto");

My main concern is that this method is called quite often, and I guess g_app_info_get_default_for_uri_scheme() iterates all the desktop files every time. Maybe we could cache the value, even if it's true that new desktop files might be installed while evince is running.

Created attachment 264051 [details] [review]
Don't allow "Send To" when there is no mail client available

(In reply to comment #17)
> Review of attachment 263812 [details] [review]:
> 
> ::: shell/ev-window.c
> @@ +463,3 @@
>      }
> 
> +    app_info = g_app_info_get_default_for_uri_scheme ("mailto");
> 
> My main concern is that this method is called quite often, and I guess
> g_app_info_get_default_for_uri_scheme() iterates all the desktop files every
> time. Maybe we could cache the value, even if it's true that new desktop files
> might be installed while evince is running.

There is some kind of caching but it still performs quite a lot of steps to get the GAppInfo (see get_all_desktop_entries_for_mime_type() and mime_info_cache_init()).

Attached is a patch which checks the availability of a "mailto" handler during initialization of EvWindow.

Review of attachment 264051 [details] [review]:

Thanks! Please consider my suggestions below before pushing the patch.

::: shell/ev-window.c
@@ +7363,3 @@
 
+	app_info = g_app_info_get_default_for_uri_scheme ("mailto");
+	ev_window->priv->has_mailto_handler = app_info != NULL;

Since this is already false and we need an if to unref the app_info in any case, we could do something like:

if (app_info) {
        ev_window->priv->has_mailto_handler = TRUE;
        g_object_unref (app_info);
}

Or even better:

ev_window->priv->has_mailto_handler = app_info != NULL;
g_clear_object (&app_info);

Comment on attachment 264051 [details] [review]
Don't allow "Send To" when there is no mail client available

Thank you for the review. I've pushed the version with g_clear_object() to master. Do you want me to push it also to a stable branch?

Regards

Marek

Thanks. I don't think there are more stable releases planned this cycle, so not sure it's worth. I wouldn't harm in any case, though.

(In reply to comment #21)
> Thanks. I don't think there are more stable releases planned this cycle, so not
> sure it's worth. I wouldn't harm in any case, though.

OK, I won't push it there then.

Thank you

Marek

(In reply to comment #22)
> (In reply to comment #21)
> > Thanks. I don't think there are more stable releases planned this cycle, so not
> > sure it's worth. I wouldn't harm in any case, though.
> 
> OK, I won't push it there then.

Hi, me again.

Bastien asked me to push the patch to 3.8 and 3.10. So I'm thinking a little bit more about this and since it can not introduce a bug into the code I will push it there. Maybe it will help some downstream maintainers.

Regards

Marek