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

[Maxim Nikulin]

On 19/03/2021 10:50, Kyle Meyer wrote:
> Maxim Nikulin writes:
> A few comments in addition to Eli's advice to drop the
> (eq system-type 'gnu/linux) condition...

Feel free to commit the change suggested in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44824#82
instead of this patch.

> > +(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.

My impression is that org-open-file function is already too long and 
complex. Another reason to use standalone function is that I am unsure 
if elisp compiler and interpreter are smart enough to reuse single 
instance of lambda. I was afraid that every opened file caused creation 
of new sentinel possibly with a closure containing chain of stack 
frames. On the other hand even in worst case memory footprint is 
negligible in comparison to any GUI viewer.

> > +  (unless (string-match "finished" event)
>
> There's no need for substring matching, right?  So it could be
>
>    (equal event "finished\n")

I was surprised by final "\n" that is not always suitable and I was in 
doubts concerning its stability. I would prefer something like

    (starts-with event "finished")

Certainly match-data is not necessary, so even match-string-p is better.

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

Thank you, I was too lazy to implement such kind of check myself. 
Certainly this variant is better.

I hope, I have addressed other your comments in the updated patch.

org-open-file-make-process-v2.patch
Description: Text Data