Quit and Error in org-export--dispatch-action

Quit and Error in org-export--dispatch-action

[Takaaki Ishikawa]

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

Dear Nicolas and all,

The org-export provides a quitting option for user by typing `q`.
This is nice feature but it is implemented with an error function.
For me, it is not actually an error, it is one of the user actions,
and when `debug-on-error` is `t`, the Backtrace buffer will be
popped up every time. It is annoying.

Please find a patch to replace error function with a simple message.
What do you think?

Best regards,
Takaaki

--
Takaaki ISHIKAWA <takaxp@ieee.org>

[-- Attachment #2: ox.patch --]
[-- Type: application/octet-stream, Size: 701 bytes --]

diff --git a/lisp/ox.el b/lisp/ox.el
index 5b4134ecc..98846540d 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -6929,8 +6929,8 @@ options as CDR."
       (org-export--dispatch-ui options first-key expertp))
      ;; q key at first level aborts export.  At second level, cancel
      ;; first key instead.
-     ((eq key ?q) (if (not first-key) (error "Export aborted")
-		    (org-export--dispatch-ui options nil expertp)))
+     ((eq key ?q) (if first-key (org-export--dispatch-ui options nil expertp)
+		    (message "Export aborted") '(ignore)))
      ;; Help key: Switch back to standard interface if expert UI was
      ;; active.
      ((eq key ??) (org-export--dispatch-ui options first-key nil))

Re: Quit and Error in org-export--dispatch-action

[Kyle Meyer]

Hi Takaaki,

Takaaki Ishikawa <takaxp@ieee.org> writes:

> The org-export provides a quitting option for user by typing `q`.
> This is nice feature but it is implemented with an error function.
> For me, it is not actually an error, it is one of the user actions,
> and when `debug-on-error` is `t`, the Backtrace buffer will be
> popped up every time. It is annoying.

True, that shouldn't be treated as a plain error.

> Please find a patch to replace error function with a simple message.
> What do you think?
>
>[...]
>
> --- a/lisp/ox.el
> +++ b/lisp/ox.el
> @@ -6929,8 +6929,8 @@ options as CDR."
>        (org-export--dispatch-ui options first-key expertp))
>       ;; q key at first level aborts export.  At second level, cancel
>       ;; first key instead.
> -     ((eq key ?q) (if (not first-key) (error "Export aborted")
> -		    (org-export--dispatch-ui options nil expertp)))
> +     ((eq key ?q) (if first-key (org-export--dispatch-ui options nil expertp)
> +		    (message "Export aborted") '(ignore)))

Hmm, what about instead replacing the call to `error' with a call to
`user-error'?  If that works for you, could you send an updated patch
with a commit message?  (Org's commit message conventions are described
at <https://orgmode.org/worg/org-contribute.html#commit-messages>.)

Re: Quit and Error in org-export--dispatch-action

[Takaaki Ishikawa]

Dear Kyle and all,

Using user-error is another way, but it does not work for me
because user-error stops the org-export-dispatch.
I would like to keep the session to do an action after
the completing org-export-dispatch something like this:

  (defun my-org-export-dispatch (f ARG)
    (interactive "P")
    (if (< (frame-width) 160)
        (apply f ARG)
      (split-window-right)
      (apply f ARG)
      (delete-window)))
  (advice-add 'org-export-dispatch :around #'my-org-export-dispatch)

So I still prefer to replace the error function with a simple message function.
If you agree with this idea, I'll produce an appropriate patch for this
as you kindly instructed.

Best,
Takaaki


--
Takaaki ISHIKAWA <takaxp@ieee.org>

2019年12月5日(木) 19:27 Kyle Meyer <kyle@kyleam.com>:
>
> Hi Takaaki,
>
> Takaaki Ishikawa <takaxp@ieee.org> writes:
>
> > The org-export provides a quitting option for user by typing `q`.
> > This is nice feature but it is implemented with an error function.
> > For me, it is not actually an error, it is one of the user actions,
> > and when `debug-on-error` is `t`, the Backtrace buffer will be
> > popped up every time. It is annoying.
>
> True, that shouldn't be treated as a plain error.
>
> > Please find a patch to replace error function with a simple message.
> > What do you think?
> >
> >[...]
> >
> > --- a/lisp/ox.el
> > +++ b/lisp/ox.el
> > @@ -6929,8 +6929,8 @@ options as CDR."
> >        (org-export--dispatch-ui options first-key expertp))
> >       ;; q key at first level aborts export.  At second level, cancel
> >       ;; first key instead.
> > -     ((eq key ?q) (if (not first-key) (error "Export aborted")
> > -                 (org-export--dispatch-ui options nil expertp)))
> > +     ((eq key ?q) (if first-key (org-export--dispatch-ui options nil expertp)
> > +                 (message "Export aborted") '(ignore)))
>
> Hmm, what about instead replacing the call to `error' with a call to
> `user-error'?  If that works for you, could you send an updated patch
> with a commit message?  (Org's commit message conventions are described
> at <https://orgmode.org/worg/org-contribute.html#commit-messages>.)

Re: Quit and Error in org-export--dispatch-action

[Kyle Meyer]

Takaaki Ishikawa <takaxp@ieee.org> writes:

> Dear Kyle and all,
>
> Using user-error is another way, but it does not work for me
> because user-error stops the org-export-dispatch.
> I would like to keep the session to do an action after
> the completing org-export-dispatch something like this:
>
>   (defun my-org-export-dispatch (f ARG)
>     (interactive "P")
>     (if (< (frame-width) 160)
>         (apply f ARG)
>       (split-window-right)
>       (apply f ARG)
>       (delete-window)))
>   (advice-add 'org-export-dispatch :around #'my-org-export-dispatch)
>
> So I still prefer to replace the error function with a simple message function.

Thanks for providing more context.  So if I'm understanding correctly,
the point here is that for your use case/setup you'd like to call
delete-window even when you select 'q' within the org-export-dispatch
call.  Signaling a user-error doesn't make this much more difficult: you
can wrap the `(apply f ...)' call within a condition-case.

I see two related advantages of sticking to signaling an error here:

 * It stays close to what the current code does.  Continuing to signal
   an error but replacing `error' with `user-error' reduces the risk of
   bugs or unintended changes in behavior while avoiding showing a
   backtrace when the caller aborts and debug-on-error is true (the
   initial issue reported in this thread).

 * 'q' is described as aborting, so I think it's confusing to make 'q'
   instead execute a no-op function and continue with the remaining code
   in the outer functions, which at the moment boils down to code in
   org-export-dispatch.  For example, with your suggested change,
   issuing 'q' will result in `ignore' being saved as the last export
   action, and it's hard to imagine that's an action the user would
   expect or want to repeat when calling org-export-dispatch with a
   prefix argument.

Re: Quit and Error in org-export--dispatch-action

[Takaaki Ishikawa]

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

Dear Kyle and all,

Thank you for your kind feedback.

> Thanks for providing more context.  So if I'm understanding correctly,
> the point here is that for your use case/setup you'd like to call
> delete-window even when you select 'q' within the org-export-dispatch
> call. Signaling a user-error doesn't make this much more difficult: you
> can wrap the `(apply f ...)' call within a condition-case.

Yes. But I found out `C-g' also exits `org-export-dispatch’ and
it is difficult for me to catch `C-g' signal in the original advice function.

So I’ve tried again and produced the following code to support calling any 
additional functions before and after `org-export-dispatch’ even if user types `q` or `C-g`.
In this case, using user-error is OK for me :)

#+begin_src emacs-lisp
(with-eval-after-load "ox"
  (defvar my-org-export-before-hook nil)
  (add-hook 'my-org-export-before-hook #'split-window-horizontally)

  (defvar my-org-export-after-hook nil)
  (add-hook 'my-org-export-after-hook #'delete-window)

  (defun my-org-export--post-processing ()
    (when (eq this-command 'org-export-dispatch)
      (run-hooks 'my-org-export-after-hook))
    (remove-hook 'post-command-hook #'my-org-export--post-processing))

  (defun my-org-export-dispatch (f ARG)
    (cond (org-export-dispatch-use-expert-ui
           (apply f ARG))
          ((> (frame-width) 160)
           (when my-org-export-after-hook
             (add-hook 'post-command-hook #'my-org-export--post-processing))
           (run-hooks 'my-org-export-before-hook)
           (apply f ARG))
          (t
           (apply f ARG))))
  (advice-add 'org-export-dispatch :around #'my-org-export-dispatch))
#+end_src

I created a patch for this issue. Please find an attached patch.
It is a really tiny patch. But I have already signed the copyright
assignment with FSF.


> I see two related advantages of sticking to signaling an error here:
> 
> * It stays close to what the current code does.  Continuing to signal
>   an error but replacing `error' with `user-error' reduces the risk of
>   bugs or unintended changes in behavior while avoiding showing a
>   backtrace when the caller aborts and debug-on-error is true (the
>   initial issue reported in this thread).
> 
> * 'q' is described as aborting, so I think it's confusing to make 'q'
>   instead execute a no-op function and continue with the remaining code
>   in the outer functions, which at the moment boils down to code in
>   org-export-dispatch.  For example, with your suggested change,
>   issuing 'q' will result in `ignore' being saved as the last export
>   action, and it's hard to imagine that's an action the user would
>   expect or want to repeat when calling org-export-dispatch with a
>   prefix argument.

I partially agree with you because the dispatcher shows it
as “[q] Exit”. This is actually different from “[q] Aborting”.
Typing `q` is completely intended by user to exit the procedure.
If something is happened accidentally in the procedure,
then, IMO, we should say it as “aborting” and update the code appropriately.

Anyway, replacing with `user-error` is OK.

Best,
Takaaki



[-- Attachment #2: 0001-ox.el-Replace-error-with-user-error-to-exit-org-expo.patch --]
[-- Type: application/octet-stream, Size: 1220 bytes --]

From b22ed4854e7776effa62f01af47691800a76a9ff Mon Sep 17 00:00:00 2001
From: Takaaki ISHIKAWA <takaxp@ieee.org>
Date: Mon, 9 Dec 2019 13:08:56 +0900
Subject: [PATCH] ox.el: Replace error with user-error to exit
 org-export-dispatch

* lisp/ox.el (org-export--dispatch-action): Replace error with user-error
(org-export--dispatch-action): Replace an error function with a user-error so that user can quit `org-export-dispatch' without entering debugging mode.

Modified from a patch proposal by Takaaki Ishikawa.
---
 lisp/ox.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ox.el b/lisp/ox.el
index 5b4134ecc..10286e18f 100644
--- a/lisp/ox.el
+++ b/lisp/ox.el
@@ -6929,7 +6929,7 @@ options as CDR."
       (org-export--dispatch-ui options first-key expertp))
      ;; q key at first level aborts export.  At second level, cancel
      ;; first key instead.
-     ((eq key ?q) (if (not first-key) (error "Export aborted")
+     ((eq key ?q) (if (not first-key) (user-error "Export aborted")
 		    (org-export--dispatch-ui options nil expertp)))
      ;; Help key: Switch back to standard interface if expert UI was
      ;; active.
-- 
2.21.0 (Apple Git-122.2)


[-- Attachment #3: Type: text/plain, Size: 4 bytes --]

Re: Quit and Error in org-export--dispatch-action

[Kyle Meyer]

Takaaki Ishikawa <takaxp@ieee.org> writes:

[...]

> I created a patch for this issue. Please find an attached patch.

Thanks!  Applied in c7ad3f884 (with minor cosmetic tweaks to the commit
message).

> It is a really tiny patch. But I have already signed the copyright
> assignment with FSF.

Great, I've added you to the list at
https://orgmode.org/worg/org-contribute.html

Re: Quit and Error in org-export--dispatch-action

[Takaaki Ishikawa]

Dear Kyle,

Confirmed. Thanks!

Best,
Takaaki

2019年12月9日(月) 19:39 Kyle Meyer <kyle@kyleam.com>:

>
> Takaaki Ishikawa <takaxp@ieee.org> writes:
>
> [...]
>
> > I created a patch for this issue. Please find an attached patch.
>
> Thanks!  Applied in c7ad3f884 (with minor cosmetic tweaks to the commit
> message).
>
> > It is a really tiny patch. But I have already signed the copyright
> > assignment with FSF.
>
> Great, I've added you to the list at
> https://orgmode.org/worg/org-contribute.html