bug#44824: [PATCH] org.el: Avoid xdg-open silent failure

[Kyle Meyer <kyle@kyleam.com>]

Maxim Nikulin writes:

>     org.el: Avoid xdg-open silent failure
>     
>     * lisp/org.el (org-open-file): Use 'pipe :connection-type instead of
>     'pty to prevent killing of background process on handler exit.
>     
>     Problem happens only in some desktop environments where configured
>     through `org-file-apps' or mailcap handlers launches actual viewer
>     (as defined in .desktop files and obtained from mimeapps.list)
>     in background.  E.g. xdg-open invokes "gio open" or kde-open5 for Gnome
>     or KDE accordingly and these handlers launches e.g. eog or okular in
>     background.  As soon as main process exits, temporary terminal session
>     created by `start-process-shell-command' is terminated.  As a result
>     background processes receive SIGHUP.
>     
>     Previously command were executed with no buffer, so the change
>     does not affect "needsterminal" and "copiousoutput" mailcap features,
>     they are not supported as earlier.
>     
>     If handler main process fails then show a message with exit reason.
>     Output (including error messages) is ignored as before.
>     Gtk application tends to report significant amount of failed asserts
>     hardly informative for majority of users.

Thanks for the detailed commit message.

A few comments in addition to Eli's advice to drop the
(eq system-type 'gnu/linux) condition...

> diff --git a/lisp/org.el b/lisp/org.el
> index 7d8733448..a199a65c9 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -8645,6 +8645,15 @@ opened in Emacs."
>     (when add-auto-mode
>       (mapcar (lambda (x) (cons (car x) 'emacs)) auto-mode-alist))))
>  
> +(defun org--error-process-sentinel (proc event)
> +  "Show a message if process failed (exited with non-zero code
> +or killed by a signal.  Pass the function as :SENTINEL argument

Please rework the first sentence so that it fits on the first line,
though I'd be in favor dropping the function and using a lambda in the
make-process call.

> +of `make-process'."
> +  (unless (string-match "finished" event)

There's no need for substring matching, right?  So it could be

  (equal event "finished\n")

Or perhaps

  (when (and (memq (process-status proc) '(exit signal))
             (/= (process-exit-status proc) 0))
    ...)

> +    (message "Command %s: %s."
> +             (mapconcat 'identity (process-command proc) " ")

s/'identity/#'identity/

> +             (substring event 0 -1))))
> +
>  ;;;###autoload
>  (defun org-open-file (path &optional in-emacs line search)
>    "Open the file at PATH.
> @@ -8766,7 +8775,17 @@ If the file does not exist, throw an error."
>  
>        (save-window-excursion
>  	(message "Running %s...done" cmd)
> -	(start-process-shell-command cmd nil cmd)
> +	(if (eq system-type 'gnu/linux)
> +	  ;; Handlers as "gio open" and kde-open5 start viewer in background

s/as/such as/ ?

> +	  ;; and exit immediately. Avoid start-process since it assumes

                                  ^ missing space

> +	  ;; :connection-type 'pty and kills children processes with SIGHUP
> +	  ;; when temporary terminal session is finished.
> +	  (make-process
> +	    :name "org-open-file" :connection-type 'pipe :noquery 't

s/'t/t/

> +	    :buffer nil ; use "*Messages*" for debugging
> +	    :sentinel 'org--error-process-sentinel
> +	    :command (list shell-file-name shell-command-switch cmd))
> +	  (start-process-shell-command cmd nil cmd))
>  	(and (boundp 'org-wait) (numberp org-wait) (sit-for org-wait))))
>       ((or (stringp cmd)
>  	  (eq cmd 'emacs))

Thanks.