Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

[Marvin J.]

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

If you globally rebind M-q, then `org-fill-paragraph' will not work

properly anymore.

The reason is that `org-fill-element' has the line

(src-block (org-babel-do-key-sequence-in-edit-buffer (kbd "M-q")))

While of course M-q is the default binding for `fill-paragraph', org

should respect that the user might want to change keybindings.


Emacs : GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.30, cairo version 1.17.4)

of 2021-08-13

Package: Org mode version 9.4.6 (9.4.6-12-gdcc3a8-elpa @
/home/mwj/.emacs.d/elpa/org-20210809/)

[-- Attachment #2: Type: text/html, Size: 1314 bytes --]

Re: Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

[Tim Cross]

This seems not only wrong, but also dangerous. I don't think org should
make any assumptions about what is bound to a key, especially when
talking about a key binding which is not an org binding. Consider what
might occur if the user had rebound that key to something destructive. 

"Marvin J." <silbersplitter@googlemail.com> writes:

> If you globally rebind M-q, then `org-fill-paragraph' will not work
>
> properly anymore.
>
> The reason is that `org-fill-element' has the line
>
> (src-block (org-babel-do-key-sequence-in-edit-buffer (kbd "M-q")))
>
> While of course M-q is the default binding for `fill-paragraph', org
>
> should respect that the user might want to change keybindings.
>
> Emacs : GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.17.4)
>
> of 2021-08-13
>
> Package: Org mode version 9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)

Re: Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

[Timothy]

"Marvin J." <silbersplitter@googlemail.com> writes:

> If you globally rebind M-q, then `org-fill-paragraph' will not work
> properly anymore.
> The reason is that `org-fill-element' has the line
> (src-block (org-babel-do-key-sequence-in-edit-buffer (kbd "M-q")))
> While of course M-q is the default binding for `fill-paragraph', org
> should respect that the user might want to change keybindings.

This looks as if it would only affect src blocks, is this consistent
with your experience? With regards to triggering M-q in the edit buffer,
considering that different modes can have different fill commands, might
you have any suggestions on what command this could be replaced by?

--
Timothy

[PATCH] Re: Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

[Ihor Radchenko]

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

Timothy <tecosaur@gmail.com> writes:

> With regards to triggering M-q in the edit buffer,
> considering that different modes can have different fill commands, might
> you have any suggestions on what command this could be replaced by?

I think simply calling `fill-paragraph' (bound to "M-q" by default)
should do. From it's docstring, it should take care about mode-specific
fill functions:

>> If fill-paragraph-function is non-nil, we call it (passing our
>> argument to it), and if it returns non-nil, we simply return its value.

Also, the docstring says that fill-paragraph-function is expected to
return non-nil when the filling is done there. For some reason, it is
not done in org-fill-paragraph.

The attached is patch solving the "M-q" problem and making sure the
org-fill-paragraph returns non-nil.

Beware that I am not very familiar with fill.el. It would be great if
someone more knowledgeable take a look at the patch.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-rely-on-M-q-binding-for-filling-src-block.patch --]
[-- Type: text/x-diff, Size: 1713 bytes --]

From 2ff317ece235890da5e1f8246dba08d585b0fbaa Mon Sep 17 00:00:00 2001
Message-Id: <2ff317ece235890da5e1f8246dba08d585b0fbaa.1630500712.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Wed, 1 Sep 2021 20:42:05 +0800
Subject: [PATCH] Do not rely on M-q binding for filling src-block

* lisp/org.el (org-fill-element): Use `fill-paragraph' instead of
simulating "M-q" binding.
(org-fill-paragraph): Return t as described in `fill-paragraph'
docstring.

Fixes https://orgmode.org/list/CAH7LOt0PO3js6_+Cbinm6EYNx0KFvpfiSS7Dwc1EXsfHfHkHVA@mail.gmail.com/
---
 lisp/org.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ce68f4692..601526e84 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19486,7 +19486,9 @@ (defun org-fill-element (&optional justify)
       ;; the buffer.  In that case, ignore filling.
       (cl-case (org-element-type element)
 	;; Use major mode filling function is source blocks.
-	(src-block (org-babel-do-key-sequence-in-edit-buffer (kbd "M-q")))
+	(src-block (org-babel-do-in-edit-buffer
+                    (mark-whole-buffer)
+                    (funcall-interactively #'fill-paragraph justify 'region)))
 	;; Align Org tables, leave table.el tables as-is.
 	(table-row (org-table-align) t)
 	(table
@@ -19621,7 +19623,9 @@ (defun org-fill-paragraph (&optional justify region)
     ;; previously unmodified), then flip the modification status back
     ;; to "unchanged".
     (when (and hash (equal hash (org-buffer-hash)))
-      (set-buffer-modified-p nil))))
+      (set-buffer-modified-p nil))
+    ;; Return non-nil.
+    t))
 
 (defun org-auto-fill-function ()
   "Auto-fill function."
-- 
2.31.1

Re: [PATCH] Re: Bug: `org-fill-paragraph' relies on M-q being bound to `fill-paragraph' [9.4.6 (9.4.6-12-gdcc3a8-elpa @ /home/mwj/.emacs.d/elpa/org-20210809/)]

[Bastien]

Ihor Radchenko <yantar92@gmail.com> writes:

> The attached is patch solving the "M-q" problem and making sure the
> org-fill-paragraph returns non-nil.

Applied, thanks!

-- 
 Bastien