bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

A user on #emacs brought it to my attention that the xdg-open detection
for browse-url is more complex than it should be and fails detecting
their Linux desktop.  Looking at the definition of
`browse-url-can-use-xdg-open' reveals that it replicates the desktop
environment check as done by `xdg-open` (which is done to detect the
DE-specific default browser setting, not to prevent people not using a
popular desktop environment from using xdg-open).  As enumerating all
possible kinds of Linux desktop is a futile endeavor, I've completely
eliminated this check from it so that the only ones left are whether
we're in a graphical session (by checking $DISPLAY) and whether
`xdg-open` has been found on $PATH.

[-- Attachment #2: 0001-Drastically-simplify-check-for-xdg-open.patch --]
[-- Type: text/x-patch, Size: 2714 bytes --]

From 6c99e7ace3110b73a5b212203acc728165be092b Mon Sep 17 00:00:00 2001
From: Vasilij Schneidermann <v.schneidermann@gmail.com>
Date: Fri, 17 Feb 2017 19:08:54 +0100
Subject: [PATCH] Drastically simplify check for xdg-open

* browse-url.el (browse-url-can-use-xdg-open): Simplify xdg-open check
---
 lisp/net/browse-url.el | 34 +++-------------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index a7c879cbfb..bf7561306c 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -45,7 +45,7 @@
 ;; browse-url-generic                 arbitrary
 ;; browse-url-default-windows-browser MS-Windows browser
 ;; browse-url-default-macosx-browser  macOS browser
-;; browse-url-xdg-open                Free Desktop xdg-open on Gnome, KDE, Xfce4, LXDE
+;; browse-url-xdg-open                Free Desktop xdg-open
 ;; browse-url-kde                     KDE konqueror (kfm)
 ;; browse-url-elinks                  Elinks      Don't know (tried with 0.12.GIT)
 
@@ -944,36 +944,8 @@ instead of `browse-url-new-window-flag'."
 
 (defun browse-url-can-use-xdg-open ()
   "Return non-nil if the \"xdg-open\" program can be used.
-xdg-open is a desktop utility that calls your preferred web browser.
-This requires you to be running either Gnome, KDE, Xfce4 or LXDE."
-  (and (getenv "DISPLAY")
-       (executable-find "xdg-open")
-       ;; xdg-open may call gnome-open and that does not wait for its child
-       ;; to finish.  This child may then be killed when the parent dies.
-       ;; Use nohup to work around.  See bug#7166, bug#8917, bug#9779 and
-       ;; http://lists.gnu.org/archive/html/emacs-devel/2009-07/msg00279.html
-       (executable-find "nohup")
-       (or (getenv "GNOME_DESKTOP_SESSION_ID")
-	   ;; GNOME_DESKTOP_SESSION_ID is deprecated, check on Dbus also.
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "dbus-send" nil nil nil
-				  "--dest=org.gnome.SessionManager"
-				  "--print-reply"
-				  "/org/gnome/SessionManager"
-				  "org.gnome.SessionManager.CanShutdown"))
-	     (error nil))
-	   (equal (getenv "KDE_FULL_SESSION") "true")
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "/bin/sh" nil nil nil
-		      "-c"
-		      ;; FIXME use string-match rather than grep.
-		      "xprop -root _DT_SAVE_MODE|grep xfce4"))
-	     (error nil))
-	   (member (getenv "DESKTOP_SESSION") '("LXDE" "Lubuntu"))
-	   (equal (getenv "XDG_CURRENT_DESKTOP") "LXDE"))))
-
+xdg-open is a desktop utility that calls your preferred web browser."
+  (and (getenv "DISPLAY") (executable-find "xdg-open")))
 
 ;;;###autoload
 (defun browse-url-xdg-open (url &optional ignored)
-- 
2.11.0

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Eli Zaretskii]

> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Date: Fri, 17 Feb 2017 19:16:48 +0100
> 
> A user on #emacs brought it to my attention that the xdg-open detection
> for browse-url is more complex than it should be and fails detecting
> their Linux desktop.  Looking at the definition of
> `browse-url-can-use-xdg-open' reveals that it replicates the desktop
> environment check as done by `xdg-open` (which is done to detect the
> DE-specific default browser setting, not to prevent people not using a
> popular desktop environment from using xdg-open).  As enumerating all
> possible kinds of Linux desktop is a futile endeavor, I've completely
> eliminated this check from it so that the only ones left are whether
> we're in a graphical session (by checking $DISPLAY) and whether
> `xdg-open` has been found on $PATH.

First, checking $DISPLAY doesn't yet mean you are in a GUI frame.  We
have display-graphic-p for that.

And second, I don't understand what will happen with all the bugs
mentioned in the comments if we remove the code that was supposed to
avoid hitting them.  Maybe I'm missing something obvious.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Vasilij Schneidermann]

> First, checking $DISPLAY doesn't yet mean you are in a GUI frame.  We
> have display-graphic-p for that.

The assumption that you need to be in a GUI frame is incorrect.  If I
run `emacs -nw` in a graphical terminal emulator, why would that prevent
me from opening a graphical browser?  The only way I see to detect
whether a graphical browser can be opened is by checking for X with
$DISPLAY.

> And second, I don't understand what will happen with all the bugs
> mentioned in the comments if we remove the code that was supposed to
> avoid hitting them.  Maybe I'm missing something obvious.

OK, I'll go through them all for completeness' sake:

- bug#7166: This one arises from using /bin/sh in combination with
  `nohup` for launching xdg-open, resulting in a lack of shell quoting.
  As `xdg-open` is launched directly, it's no longer relevant and only
  mentioned because of `nohup` (which is no longer used either).
- bug#8917: The old usage of `call-process` has made `nohup` obsolete.
  Irrelevant as `nohup` isn't used any longer.
- bug#9779: Extension of the now removed xdg-open DE detection logic,
  together with some discussion whether nohup is still necessary.  Some
  voices in favor of not doing any unnecessary detection.
- The linked emacs-devel thread: Sighting of a possible bug in
  gnome-open.  As `nohup` is no longer used, there's no way this could
  have been worked around with its usage

Summary: All linked bugs are related to `nohup` which is only checked
for, but not actually used.  Therefore they can be disregarded.

Do you have any actual objections based on a situation where one would
*not* want to use xdg-open, even though it's installed and the user in a
X11 session?

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Glenn Morris]

The area seems a mess, but:

1) nohup was removed from browse-url-xdg-open in 1c0f1a199ca, with no
explanation and as part of a change that seems unrelated.

2) In https://debbugs.gnu.org/9779#20, the author of that change said

    I note that since 22-Jun-2011 browse-url-xdg-open no longer uses
    nohup.  This is either a mistake on my part (I didn't read the
    comment in browse-url-can-use-xdg-open) or it is indeed
    unnecessary and therefore isn't a requirement for xdg-open.

3) In reply, Jan said in https://debbugs.gnu.org/9779#29

    See discussion at
    http://lists.gnu.org/archive/html/emacs-devel/2009-07/msg00279.html.
    nohup is still needed for xdg-open.

And AFAICS that was the last comment about nohup. No-one ever seems to
have followed up Jan's comment.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Glenn Morris]

PS Maybe you are just saying that since no-one (?) complained since
then, nohup can't have been needed after all. It would be better to test
it, though. It seems difficult to test all the possibilities for
xdg-open.

Ref https://debbugs.gnu.org/25234 

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Vasilij Schneidermann]

> PS Maybe you are just saying that since no-one (?) complained since
> then, nohup can't have been needed after all.

Pretty much.  Note that `gnome-open` belongs to GNOME 2 which last
release has been in 2010.  Only Debian Squeeze is offering it in its
package archives and as that one has been EOL'd, I don't see any reason
to pursue it any further.  If `gvfs-open` exhibits the same behavior,
maybe, but I still consider it upstream's fault if they have weird open
behavior, not an area of Emacs to clumsily work around.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Glenn Morris]

Vasilij Schneidermann wrote:

> If `gvfs-open` exhibits the same behavior,

It does. https://bugzilla.gnome.org/show_bug.cgi?id=652262
And https://debbugs.gnu.org/25234, as previously mentioned.

> maybe, but I still consider it upstream's fault

As you can see from that above report, they don't show any signs of
fixing it.

> if they have weird open behavior, not an area of Emacs to clumsily
> work around.

The people who implemented support for xdg-open in Emacs disagreed.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Vasilij Schneidermann]

> As you can see from that above report, they don't show any signs of
> fixing it.

Well, fine.  I'm sorry for not noticing a five year old piece of code
that got mistakenly removed without anyone complaining.  What I don't
understand is why my patch (removing IMHO unnecessary DE detection) is
related to using nohup to work around a problem with xdg-open and
gnome-open/gvfs-open.  If you consider fixing the nohup problem a
prerequisite before this patch can be applied, OK, do that then, I can
wait until that's done and recommence discussion on the other issue.

That aside, I cannot reproduce any weird behavior, be it with or without
nohup and gvfs-open.  Both `(call-process "gvfs-open" nil 0 nil
"foo.pdf")` and `(call-process "nohup" nil 0 nil "gvfs-open"
"foo.pdf")` spawn my PDF reader successfully and don't do anything weird
after closing it.  Same if I pass "http://ix.de/" as last argument.  I
therefore consider the nohup issue a case of cargo-culting and would
like to drop its discussion entirely as it does not help with the
current issue.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Eli Zaretskii]

> Date: Fri, 17 Feb 2017 19:55:14 +0100
> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Cc: 25778@debbugs.gnu.org
> 
> > First, checking $DISPLAY doesn't yet mean you are in a GUI frame.  We
> > have display-graphic-p for that.
> 
> The assumption that you need to be in a GUI frame is incorrect.  If I
> run `emacs -nw` in a graphical terminal emulator, why would that prevent
> me from opening a graphical browser?  The only way I see to detect
> whether a graphical browser can be opened is by checking for X with
> $DISPLAY.

In that case, maybe we shouldn't test $DISPLAY at all?  Why restrict
this to X if xdg-open is already tested for availability?

> Do you have any actual objections based on a situation where one would
> *not* want to use xdg-open, even though it's installed and the user in a
> X11 session?

See Glenn's response about that.  I think we should be more careful
with these issues, and at least do some more research of them before
we can be sure they are no longer relevant.  If you could follow up on
that, it would be great.

Thanks.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Vasilij Schneidermann]

> In that case, maybe we shouldn't test $DISPLAY at all?  Why restrict
> this to X if xdg-open is already tested for availability?

To quote its man page: "xdg-open is for use inside a desktop session
only.".  If you attempt using it outside one (like, on the Linux
console), M-x browse-url fails silently.

bug#25778: 25.1; [PATCH] Drastically simplify xdg-open check

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

Thanks for the bug report. I installed the attached somewhat-more-ambitious 
patch that works along the line that you suggested. Although every heuristic in 
this area will be wrong sometimes, I think nowadays we're probably better off 
simply using xdg-open if it's available and if there's a display.

It turns out that nohup is ineffective on xdg-open, as xdg-open's child does a 
'signal (SIGHUP, SIG_DFL)' before it execs (!). This may help to explain why 
nobody noticed when Emacs stopped using nohup. Although nohup may have been 
needed for ancient GNOME versions, I don't think we need to worry about these 
old GNOME versions in future Emacs releases.

I'm closing the bug report as I think the bug is fixed. We can reopen it if I'm 
wrong.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Simplify-checks-for-xdg-open-and-xdg-email.patch --]
[-- Type: text/x-diff; name="0001-Simplify-checks-for-xdg-open-and-xdg-email.patch", Size: 5679 bytes --]

From 095e7d126ee1fc56cb1ec0f9ab1a9cdd6417a232 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 9 Mar 2017 19:01:19 -0800
Subject: [PATCH] Simplify checks for xdg-open and xdg-email

browse-url's xdg-open detection was too picky on some GNU/Linux
desktops; see Bug#25778.  Simplify the code by assuming xdg-open works
if it is executable, as nowadays this is more likely to be correct than
trying to use heuristics from a few years ago.  Don't test for nohup: it
is ineffective nowadays, as xdg-open's child uses the default action for
SIGHUP even if xdg-open's invoker ignores SIGHUP.  While we're at it,
allow for Wayland here, as "emacs -nw" might be running in a non-X
Wayland terminal.
* lisp/mail/emacsbug.el (report-emacs-bug-can-use-xdg-email):
* lisp/net/browse-url.el (browse-url-can-use-xdg-open):
Simplify to a test for DISPLAY and whether the helper program is
executable.  Allow WAYLAND_DISPLAY as an option.
---
 lisp/mail/emacsbug.el  | 29 ++++-------------------------
 lisp/net/browse-url.el | 40 +++++++++-------------------------------
 2 files changed, 13 insertions(+), 56 deletions(-)

diff --git a/lisp/mail/emacsbug.el b/lisp/mail/emacsbug.el
index ecb7db6..c1aec69 100644
--- a/lisp/mail/emacsbug.el
+++ b/lisp/mail/emacsbug.el
@@ -77,33 +77,12 @@ report-emacs-bug-can-use-osx-open
        (equal (executable-find "open") "/usr/bin/open")
        (memq system-type '(darwin))))
 
-;; FIXME this duplicates much of the logic from browse-url-can-use-xdg-open.
 (defun report-emacs-bug-can-use-xdg-email ()
   "Return non-nil if the \"xdg-email\" command can be used.
-xdg-email is a desktop utility that calls your preferred mail client.
-This requires you to be running either Gnome, KDE, or Xfce4."
-  (and (getenv "DISPLAY")
-       (executable-find "xdg-email")
-       (or (getenv "GNOME_DESKTOP_SESSION_ID")
-	   ;; GNOME_DESKTOP_SESSION_ID is deprecated, check on Dbus also.
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "dbus-send" nil nil nil
-				  "--dest=org.gnome.SessionManager"
-				  "--print-reply"
-				  "/org/gnome/SessionManager"
-				  "org.gnome.SessionManager.CanShutdown"))
-	     (error nil))
-	   (equal (getenv "KDE_FULL_SESSION") "true")
-	   ;; FIXME? browse-url-can-use-xdg-open also accepts LXDE.
-	   ;; Is that no good here, or just overlooked?
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "/bin/sh" nil nil nil
-		      "-c"
-		      ;; FIXME use string-match rather than grep.
-		      "xprop -root _DT_SAVE_MODE|grep xfce4"))
-	     (error nil)))))
+xdg-email is a desktop utility that calls your preferred mail client."
+  (and ;; See browse-url-can-use-xdg-open.
+       (or (getenv "DISPLAY") (getenv "WAYLAND_DISPLAY"))
+       (executable-find "xdg-email")))
 
 (defun report-emacs-bug-insert-to-mailer ()
   "Send the message to your preferred mail client.
diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index 04b49c4..20ae072 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -45,7 +45,7 @@
 ;; browse-url-generic                 arbitrary
 ;; browse-url-default-windows-browser MS-Windows browser
 ;; browse-url-default-macosx-browser  macOS browser
-;; browse-url-xdg-open                Free Desktop xdg-open on Gnome, KDE, Xfce4, LXDE
+;; browse-url-xdg-open                freedesktop.org xdg-open
 ;; browse-url-kde                     KDE konqueror (kfm)
 ;; browse-url-elinks                  Elinks      Don't know (tried with 0.12.GIT)
 
@@ -944,36 +944,14 @@ browse-url-default-browser
 
 (defun browse-url-can-use-xdg-open ()
   "Return non-nil if the \"xdg-open\" program can be used.
-xdg-open is a desktop utility that calls your preferred web browser.
-This requires you to be running either Gnome, KDE, Xfce4 or LXDE."
-  (and (getenv "DISPLAY")
-       (executable-find "xdg-open")
-       ;; xdg-open may call gnome-open and that does not wait for its child
-       ;; to finish.  This child may then be killed when the parent dies.
-       ;; Use nohup to work around.  See bug#7166, bug#8917, bug#9779 and
-       ;; http://lists.gnu.org/archive/html/emacs-devel/2009-07/msg00279.html
-       (executable-find "nohup")
-       (or (getenv "GNOME_DESKTOP_SESSION_ID")
-	   ;; GNOME_DESKTOP_SESSION_ID is deprecated, check on Dbus also.
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "dbus-send" nil nil nil
-				  "--dest=org.gnome.SessionManager"
-				  "--print-reply"
-				  "/org/gnome/SessionManager"
-				  "org.gnome.SessionManager.CanShutdown"))
-	     (error nil))
-	   (equal (getenv "KDE_FULL_SESSION") "true")
-	   (condition-case nil
-	       (eq 0 (call-process
-		      "/bin/sh" nil nil nil
-		      "-c"
-		      ;; FIXME use string-match rather than grep.
-		      "xprop -root _DT_SAVE_MODE|grep xfce4"))
-	     (error nil))
-	   (member (getenv "DESKTOP_SESSION") '("LXDE" "Lubuntu"))
-	   (equal (getenv "XDG_CURRENT_DESKTOP") "LXDE"))))
-
+xdg-open is a desktop utility that calls your preferred web browser."
+  ;; The exact set of situations where xdg-open works is complicated,
+  ;; and it would be a pain to duplicate xdg-open's situation-specific
+  ;; code here, as the code is a moving target.  So assume that
+  ;; xdg-open will work if there is a graphical display; this should
+  ;; be good enough for platforms Emacs is likely to be running on.
+  (and (or (getenv "DISPLAY") (getenv "WAYLAND_DISPLAY"))
+       (executable-find "xdg-open")))
 
 ;;;###autoload
 (defun browse-url-xdg-open (url &optional ignored)
-- 
2.7.4