[PATCH] ol-man.el (org-man-open): Set window point not buffer point

[PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Tom Gillespie]

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

Here's a patch to fix the follow behavior for ol-man links so
that the ::SEARCH functionality will actually work. Best!
Tom

[-- Attachment #2: 0001-lisp-ol-man.el-org-man-open-Set-window-point-not-buf.patch --]
[-- Type: text/x-patch, Size: 1914 bytes --]

From 2c3e3b994fd7b47a6e91d147d2b1f08cd97a1908 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Thu, 28 Jul 2022 23:33:22 -0700
Subject: [PATCH] * lisp/ol-man.el (org-man-open): Set window point not buffer
 point

When passed man:path::SEARCH org-man-open tries to use search-forward
to jump to the location of e.g. a heading. Prior to this fix it only
used search-forward, which will not change the point of the cursor in
the window, meaning that even if there is a match it will not appear.

Use sleep-for as a horrible hack to work around the fact that the man
command runs in the background with no way to synchronize back.
---
 lisp/ol-man.el | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/lisp/ol-man.el b/lisp/ol-man.el
index aa22964c5..5843cd5f6 100644
--- a/lisp/ol-man.el
+++ b/lisp/ol-man.el
@@ -43,12 +43,20 @@ If PATH contains extra ::STRING which will use `occur' to search
 matched strings in man buffer."
   (string-match "\\(.*?\\)\\(?:::\\(.*\\)\\)?$" path)
   (let* ((command (match-string 1 path))
-	 (search (match-string 2 path)))
-    (funcall org-man-command command)
+         (search (match-string 2 path))
+         (buffer (funcall org-man-command command)))
     (when search
-      (with-current-buffer (concat "*Man " command "*")
-	(goto-char (point-min))
-	(search-forward search)))))
+      (with-current-buffer buffer
+        (goto-char (point-min))
+        (unless (search-forward search nil t)
+          (sleep-for 0.75) ; async, can't block, no callback
+          (goto-char (point-min))
+          (search-forward search))
+        (previous-line)
+        (let ((point (point)))
+          (let ((window (get-buffer-window buffer)))
+            (set-window-point window point)
+            (set-window-start window point)))))))
 
 (defun org-man-store-link ()
   "Store a link to a README file."
-- 
2.35.1

Re: [PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Here's a patch to fix the follow behavior for ol-man links so
> that the ::SEARCH functionality will actually work. Best!
> Tom
> From 2c3e3b994fd7b47a6e91d147d2b1f08cd97a1908 Mon Sep 17 00:00:00 2001
> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Thu, 28 Jul 2022 23:33:22 -0700
> Subject: [PATCH] * lisp/ol-man.el (org-man-open): Set window point not buffer
>  point

Thanks for the contribution!

> When passed man:path::SEARCH org-man-open tries to use search-forward
> to jump to the location of e.g. a heading. Prior to this fix it only
> used search-forward, which will not change the point of the cursor in
> the window, meaning that even if there is a match it will not appear.

Please check out
https://orgmode.org/worg/org-contribute.html#commit-messages for our
conventions about commit messages.

> Use sleep-for as a horrible hack to work around the fact that the man
> command runs in the background with no way to synchronize back.

This is indeed a horrible hack. What about `accept-process-output'?

Best,
Ihor

Re: [PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Tom Gillespie]

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

Hi Ihor,
   Here is an updated patch. We can't use accept-process-output
because it doesn't seem to block in the way we need, or it blocks
exactly long enough for the process to finish but then continues
immediately to search instead of allowing the function that fills
the buffer to complete. Instead I use sleep-for a shorter time and
process-live-p which gives better results. I think I got the commit
message formats right this time. Best!
Tom

[-- Attachment #2: 0001-ol-man-Set-window-point-not-buffer-point-and-wait-be.patch --]
[-- Type: text/x-patch, Size: 2067 bytes --]

From 2db2ce6d83b27fcf6366183cbd8b5fa79fcbc4a7 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Thu, 28 Jul 2022 23:33:22 -0700
Subject: [PATCH] ol-man: Set window point not buffer point and wait before
 search

* lisp/ol-man.el (org-man-open): Set window point not buffer point
When passed man:path::SEARCH org-man-open tries to use search-forward
to jump to the location of e.g. a heading. Prior to this fix it only
used search-forward, which will not change the point of the cursor in
the window, meaning that even if there is a match it will not appear.
Uses process-live-p and sleep-for to wait until the manpage finishes
rendering before searching the buffer so that there will be something
to find.
---
 lisp/ol-man.el | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/lisp/ol-man.el b/lisp/ol-man.el
index aa22964c5..8633fe5cb 100644
--- a/lisp/ol-man.el
+++ b/lisp/ol-man.el
@@ -43,12 +43,22 @@ If PATH contains extra ::STRING which will use `occur' to search
 matched strings in man buffer."
   (string-match "\\(.*?\\)\\(?:::\\(.*\\)\\)?$" path)
   (let* ((command (match-string 1 path))
-	 (search (match-string 2 path)))
-    (funcall org-man-command command)
+         (search (match-string 2 path))
+         (buffer (funcall org-man-command command)))
     (when search
-      (with-current-buffer (concat "*Man " command "*")
-	(goto-char (point-min))
-	(search-forward search)))))
+      (with-current-buffer buffer
+        (goto-char (point-min))
+        (unless (search-forward search nil t)
+          (let ((process (get-buffer-process buffer)))
+            (while (process-live-p process)
+              (sleep-for 0.01)))
+          (goto-char (point-min))
+          (search-forward search))
+        (previous-line)
+        (let ((point (point)))
+          (let ((window (get-buffer-window buffer)))
+            (set-window-point window point)
+            (set-window-start window point)))))))
 
 (defun org-man-store-link ()
   "Store a link to a README file."
-- 
2.35.1

Re: [PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

> Hi Ihor,
>    Here is an updated patch. We can't use accept-process-output
> because it doesn't seem to block in the way we need, or it blocks
> exactly long enough for the process to finish but then continues
> immediately to search instead of allowing the function that fills
> the buffer to complete. Instead I use sleep-for a shorter time and
> process-live-p which gives better results. I think I got the commit
> message formats right this time. Best!

Why not 

(while (process-live-p process)
  (accept-process-output process)))

then?

sleep-for is using similar machinery under the hood, but
accept-process-output does not require magic constants and in addition
handles various edge cases.

Also, compiling the patch yields

In org-man-open:
ol-man.el:54:16: Warning: ‘previous-line’ is for interactive use only; use
    ‘forward-line’ with negative argument instead.

> * lisp/ol-man.el (org-man-open): Set window point not buffer point
> When passed man:path::SEARCH org-man-open tries to use search-forward
> to jump to the location of e.g. a heading. Prior to this fix it only
> used search-forward, which will not change the point of the cursor in
> the window, meaning that even if there is a match it will not appear.
> Uses process-live-p and sleep-for to wait until the manpage finishes
> rendering before searching the buffer so that there will be something
> to find.

Please use double space "  " between sentences and quote `org-man-open'
and similar Elisp symbols.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92

Re: [PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Tom Gillespie]

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

> (while (process-live-p process)
>   (accept-process-output process)))

When I tried this before it didn't work, but now it does, I
must have missed something. Patch updated accordingly.

The order in which the man.el code does things is supremely
confusing, but I think when accept-process-output returns that
means the process sentinel has finished its final run and the
man buffer is fully populated so it is safe to search.

> Also, compiling the patch yields

No byte compiler errors now, and I think I got all the formatting issues.

[-- Attachment #2: 0001-ol-man-Set-window-point-not-buffer-point-and-wait-be.patch --]
[-- Type: text/x-patch, Size: 2104 bytes --]

From 848d6fc9bd395d7d45f14af71c4df8ea44ed7b4c Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Thu, 28 Jul 2022 23:33:22 -0700
Subject: [PATCH] ol-man: Set window point not buffer point and wait before
 search

* lisp/ol-man.el (org-man-open): Set window point not buffer point and
wait before search.  When passed man:path::SEARCH `org-man-open' uses
`search-forward' to jump to the location of e.g. a heading.  Prior to
this fix it only used `search-forward', which will not change the
point of the cursor in the window, meaning that even if there is a
match it will not appear.  Use `accept-process-output' to block until
the manpage finishes rendering before searching the buffer so that
there will be something to find.
---
 lisp/ol-man.el | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/lisp/ol-man.el b/lisp/ol-man.el
index aa22964c5..24e896f30 100644
--- a/lisp/ol-man.el
+++ b/lisp/ol-man.el
@@ -43,12 +43,22 @@ If PATH contains extra ::STRING which will use `occur' to search
 matched strings in man buffer."
   (string-match "\\(.*?\\)\\(?:::\\(.*\\)\\)?$" path)
   (let* ((command (match-string 1 path))
-	 (search (match-string 2 path)))
-    (funcall org-man-command command)
+         (search (match-string 2 path))
+         (buffer (funcall org-man-command command)))
     (when search
-      (with-current-buffer (concat "*Man " command "*")
-	(goto-char (point-min))
-	(search-forward search)))))
+      (with-current-buffer buffer
+        (goto-char (point-min))
+        (unless (search-forward search nil t)
+          (let ((process (get-buffer-process buffer)))
+            (while (process-live-p process)
+              (accept-process-output process)))
+          (goto-char (point-min))
+          (search-forward search))
+        (forward-line -1)
+        (let ((point (point)))
+          (let ((window (get-buffer-window buffer)))
+            (set-window-point window point)
+            (set-window-start window point)))))))
 
 (defun org-man-store-link ()
   "Store a link to a README file."
-- 
2.35.1

Re: [PATCH] ol-man.el (org-man-open): Set window point not buffer point

[Ihor Radchenko]

Tom Gillespie <tgbugs@gmail.com> writes:

>> (while (process-live-p process)
>>   (accept-process-output process)))
>
> When I tried this before it didn't work, but now it does, I
> must have missed something. Patch updated accordingly.
>
> The order in which the man.el code does things is supremely
> confusing, but I think when accept-process-output returns that
> means the process sentinel has finished its final run and the
> man buffer is fully populated so it is safe to search.

Thanks!
Applied onto main via 76643256f.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=76643256f2bddb79908a41c264c43de3e06a70dd


-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92