jwiegley / emacs-async

Simple library for asynchronous processing in Emacs
GNU General Public License v3.0
829 stars 68 forks source link

Suggestion/question about `#` in async in `async-when-done` function #145

Closed dalanicolai closed 2 years ago

dalanicolai commented 2 years ago

I would like to notify about a simple problem to a minor issue. I encountered the issue while answering to a question on stack-exchange (https://emacs.stackexchange.com/questions/68123/async-org-agenda/68136#68136). That question and answer quite clearly state the problem and the solution.

So when trying to produce the org-agenda asynchronously, I encountered the error async-handle-result: Invalid read syntax: "#". This was because I was sending text-properties. However, I could simply replace the # in the output buffer and via a mapping function reapply the text-properties.

I guess similar replace 'tricks' could also solve some of the cases where problems arise when trying to read buffers. So, I thought it might be an option to add a flag/argument for it (to provide some replace function with async-start).

Such trick might help to solve issues like #127...

thierryvolpiatto commented 2 years ago

Thanks to catch this, it is a very nice trick, however it would be simpler to just quote the "#" then there would be no effort on the user side i.e. not need to reapply properties with map-text-properties though this function is very nice. Here a patch for async.el:

diff --git a/async.el b/async.el
index 2537681a4e..2e640a51d5 100644
--- a/async.el
+++ b/async.el
@@ -164,8 +164,11 @@ It is intended to be used as follows:
                         (kill-buffer (current-buffer))))
                   (set (make-local-variable 'async-callback-value) proc)
                   (set (make-local-variable 'async-callback-value-set) t))
+              (goto-char (point-min))
+              (while (re-search-forward "#" nil t)
+                (replace-match "\\#" t t))
               (goto-char (point-max))
-              (backward-sexp)
+              (backward-sexp) (skip-chars-backward "#" 1)
               (async-handle-result async-callback (read (current-buffer))
                                    (current-buffer)))
           (set (make-local-variable 'async-callback-value)

and here the user code to use instead:

(async-start
 ;; What to do in the child process
 (lambda ()
   (setq org-agenda-files '("~/tmp/test.org"))
   (org-agenda-list)
   (buffer-string))

 ;; What to do when it finishes
 (lambda (result)
   (switch-to-buffer-other-window "*Org Agenda*")
   (insert result)
   (org-agenda-mode)))

Is this working for you ?

dalanicolai commented 2 years ago

Thanks, that is great! Yes, I already expected there would be a simpler way somehow (I mentioned it in the answer), but it is only simpler when you already know it ;). So, thanks for sharing this trick. It looks simple and logical. I will look a bit into how read works.

Anyway, it works perfectly!

I guess it is fine if I point users to this better answer on stack-exchange.

thierryvolpiatto commented 2 years ago

UPDATE: After looking more carefully at this, it looks it is failing with more complex strings :-(. Same with your solution, we can't get back special object as property value (e.g. org-marker), and embedded strings with props are not restored properly (they are substituted by nil instead), as a result with the org example hitting RET in the agenda buffer have no effect. So actually there is no safe solution to this problem i.e. returning (buffer-string) as value as long as read doesn't support such objects.

dalanicolai commented 2 years ago

I noticed this also, very soon after I commented back. But I was looking into it and trying to understand and fix it, so I only see your comment now. However, I do not understand what you mean with "embedded string with props are not restored properly but they are substituted with nil". I mean, although I notice that they do not get restored properly, when I print the buffer-string received from the async-process, then I do see the embedded string with the properties but I see the hashes "#" have been replaced by "\# " compared with the original buffer string (from org-agenda-list). I am using Emacs 28 b.t.w.

Well, thanks again anyway, of course. And my original solution is a nice workaround, then. It does restore the "embedded string with props" properly and the links work (at least on my system, but not that of the OP, however he did not yet respond to my proposed solution for him in the comments). But it requires the dirty version of async-when-done as given in the se-answer.

I will 'revert' my edit in the stack-exchange answer for now then...

Also, I could report a bug/feature request for this (or post in the emacs-develop mailing list). If you did not do it already? Then let me know.

dalanicolai commented 2 years ago

Okay, I think I do see now what you meant about the face props

thierryvolpiatto commented 2 years ago

Daniel Nicolai @.***> writes:

I noticed this also, very soon after I commented back. But I was looking into it and trying to understand and fix it, so I only see your comment now. However, I do not understand what you mean with "embedded string with props are not restored properly but they are substituted with nil".

For example this:

(todo-state

("TODO" 0 4

is restored with:

(todo-state (nil 0 4

Well, thanks again anyway, of course. And my original solution is a nice workaround, then. It does restore the "embedded string with props" properly and the links work (at least on my system, but not that of the OP, however he did not yet respond to my proposed solution for him in the comments).

Some objects contained in props may not be restored properly e.g. markers are corrupted:

Instead of #<marker[...] you have <marker[...].

Maybe trying to replace marker objects by a sexp like (set-marker (make-marker)...), perhaps desktop.el have such code to save markers.

-- Thierry

map7 commented 2 years ago

@thierryvolpiatto The patch and code you posted before didn't work for me under Emacs 27.1.

thierryvolpiatto commented 2 years ago

UPDATE: Here is something that may restore markers properly and fix issues mentioned previously:

diff --git a/async.el b/async.el
index 2537681a4e..7eeb87eeab 100644
--- a/async.el
+++ b/async.el
@@ -164,6 +164,16 @@ It is intended to be used as follows:
                         (kill-buffer (current-buffer))))
                   (set (make-local-variable 'async-callback-value) proc)
                   (set (make-local-variable 'async-callback-value-set) t))
+              (goto-char (point-min))
+              (save-excursion
+                ;; Transform markers in list like
+                ;; (marker (moves after insertion) at 2338 in
+                ;; test\.org) so that remap text properties function
+                ;; can parse to restitute marker.
+                (while (re-search-forward "#<\\([^>]*\\)>" nil t)
+                  (replace-match (concat "(" (match-string 1) ")") t t)))
+              (while (re-search-forward "#" nil t)
+                (replace-match "" t t))
               (goto-char (point-max))
               (backward-sexp)
               (async-handle-result async-callback (read (current-buffer))

(async-start
 ;; What to do in the child process
 (lambda ()
   (setq org-agenda-files '("~/tmp/test.org"))
   (org-agenda-list)
   (buffer-string))

 ;; What to do when it finishes
 (lambda (result)
   (switch-to-buffer-other-window "*Org Agenda*")
   ;; In this example, we have only one file in org-agenda-files, but
   ;; we can assume user will want to use the same value of
   ;; org-agenda-files both in parent and child so we can loop in
   ;; org-agenda-files to open all org buffer (org is doing this when
   ;; calling org-agenda).
   (find-file-noselect "~/tmp/test.org")
   (insert (car result))
   (map-text-properties (cdr result))
   (org-agenda-mode)))

(defun map-text-properties (props)
  (let ((plist (caddr props)))
    (while plist
      (put-text-property (1+ (nth 0 props))
                         (1+ (nth 1 props))
                         (car plist)
                         (let ((value (cadr plist)))
                           (cond ((and (consp value)
                                       (stringp (car value)))
                                  (with-temp-buffer
                                    (insert (car value))
                                    (map-text-properties (cdr value))
                                    (buffer-string)))
                                 ((and (consp value) (memq 'marker value))
                                  (set-marker (make-marker)
                                              (cl-loop for i in value
                                                       thereis (and (numberp i) i))
                                              (get-buffer (mapconcat 'symbol-name (last value) ""))))
                                 (t value))))
      (setq plist (cddr plist)))
    (when props
      (map-text-properties (nthcdr 3 props)))))
thierryvolpiatto commented 2 years ago

Also when restoring markers we want to restore as well marker insertion types:

(defun map-text-properties (props)
  (let ((plist (caddr props)))
    (while plist
      (put-text-property (1+ (nth 0 props))
                         (1+ (nth 1 props))
                         (car plist)
                         (let ((value (cadr plist)))
                           (cond ((and (consp value)
                                       (stringp (car value)))
                                  (with-temp-buffer
                                    (insert (car value))
                                    (map-text-properties (cdr value))
                                    (buffer-string)))
                                 ((and (consp value) (memq 'marker value))
                                  (let ((marker (set-marker
                                                 (make-marker)
                                                 (cl-loop for i in value
                                                          thereis (and (numberp i) i))
                                                 (get-buffer (mapconcat 'symbol-name (last value) "")))))
                                    (set-marker-insertion-type marker t)
                                    marker))
                                 (t value))))
      (setq plist (cddr plist)))
    (when props
      (map-text-properties (nthcdr 3 props)))))
dalanicolai commented 2 years ago

Great! Indeed, the links work now (and this time I tested it properly, so no false claims:). Very nice! I will add the reference to here again then, in the stack-exchange answer.

Thanks a lot!

thierryvolpiatto commented 2 years ago

Daniel Nicolai @.***> writes:

Great! Indeed, the links work now (and this time I tested it properly, so no false claims:). Very nice! I will add the reference to here again then, in the stack-exchange answer.

I will commit the changes on the async side soon, I am still unsure if I make it optional or not and how.

Thanks a lot!

Thanks also for your help on this!

-- Thierry

thierryvolpiatto commented 2 years ago

Merged the changes in master, closing now. Thanks.

map7 commented 2 years ago

Hello. I tried the following code in Emacs 27.1, 27.2 and master branch and I get an error

Code tested;

(defun map-text-properties (props)
  (let ((plist (caddr props)))
    (while plist
      (put-text-property (1+ (nth 0 props))
                         (1+ (nth 1 props))
                         (car plist)
                         (let ((value (cadr plist)))
                           (cond ((and (consp value)
                                       (stringp (car value)))
                                  (with-temp-buffer
                                    (insert (car value))
                                    (map-text-properties (cdr value))
                                    (buffer-string)))
                                 ((and (consp value) (memq 'marker value))
                                  (let ((marker (set-marker
                                                 (make-marker)
                                                 (cl-loop for i in value
                                                          thereis (and (numberp i) i))
                                                 (get-buffer (mapconcat 'symbol-name (last value) "")))))
                                    (set-marker-insertion-type marker t)
                                    marker))
                                 (t value))))
      (setq plist (cddr plist)))
    (when props
      (map-text-properties (nthcdr 3 props)))))

(defun async-org-agenda-list4()
  (interactive)

  (async-start
    ;; What to do in the child process
    (lambda ()
      (setq org-agenda-files '("~/tmp/test.org"))
      (org-agenda-list)
      (buffer-string))

    ;; What to do when it finishes
    (lambda (result)
      (switch-to-buffer-other-window "*Org Agenda ASYNC*")
      ;; In this example, we have only one file in org-agenda-files, but
      ;; we can assume user will want to use the same value of
      ;; org-agenda-files both in parent and child so we can loop in
      ;; org-agenda-files to open all org buffer (org is doing this when
      ;; calling org-agenda).
      (find-file-noselect "~/tmp/test.org")
      (insert (car result))
      (map-text-properties (cdr result))
      (org-agenda-mode))))

~/tmp/test.org file

* TODO [#B] test
  SCHEDULED: <2021-08-23 Mon>
  :PROPERTIES:
  :CREATED:  [2021-08-23 Mon 09:36]
  :END:

Error

 Args out of range: 1, 15

Even though I get this error in Messages it still presents the Org Agenda ASYNC buffer with some highlighting and if I hover the mouse it looks like it will jump to the ~/tmp/test.org file but when I click it, emacs doesn't open that file.

I have updated async package this morning before running all these tests.

thierryvolpiatto commented 2 years ago

Michael Pope @.***> writes:

Hello. I tried the following code in Emacs 27.1, 27.2 and master branch and I get an error ~/tmp/test.org file

• TODO [#B] test ^^^^ because of this, if you remove it it works without errors.

Perhaps we should strip out the # only at start and in second part of the string?

Thanks for the test file.

Note: to quote your code use three quotes and and close it with three quotes again or at least indent it with 4 spaces.

-- Thierry

thierryvolpiatto commented 2 years ago

This should work:

diff --git a/async.el b/async.el
index eb9da7b685..a8455b6959 100644
--- a/async.el
+++ b/async.el
@@ -175,8 +175,8 @@ It is intended to be used as follows:
                 ;; can parse it to restitute marker.
                 (while (re-search-forward "#<\\([^>]*\\)>" nil t)
                   (replace-match (concat "(" (match-string 1) ")") t t)))
-              (while (re-search-forward "#" nil t)
-                (replace-match "" t t))
+              (while (re-search-forward "#(" nil t)
+                (replace-match "(" t t))
               (goto-char (point-max))
               (backward-sexp)
               (async-handle-result async-callback (read (current-buffer))
map7 commented 2 years ago

@thierryvolpiatto With that latest change you just posted I no longer get the error, the colour is correct. The only thing which doesn't work is clicking on the TODO task doesn't open up the test.org file. If I hover my mouse over the TODO item in the Org Agenda ASYNC buffer then it says 'mouse-2 or RET jump to Org file ~/tmp/test.org' but when I do either of those on the entry test.org file is not loaded. This is in emacs 27.1 & 27.2

thierryvolpiatto commented 2 years ago

Michael Pope @.***> writes:

@thierryvolpiatto With that latest change you just posted I no longer get the error, the colour is correct. The only thing which doesn't work is clicking on the TODO task doesn't open up the test.org file. If I hover my mouse over the TODO item in the Org Agenda ASYNC buffer then it says 'mouse-2 or RET jump to Org file ~/tmp/test.org' but when I do either of those on the entry test.org file is not loaded. This is in emacs 27.1 & 27.2

This is now a bug in map-text-properties which add duplicate props, I will look at this later.

Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.*

-- Thierry

thierryvolpiatto commented 2 years ago

Thierry Volpiatto @.***> writes:

Michael Pope @.***> writes:

@thierryvolpiatto With that latest change you just posted I no longer get the error, the colour is correct. The only thing which doesn't work is clicking on the TODO task doesn't open up the test.org file. If I hover my mouse over the TODO item in the Org Agenda ASYNC buffer then it says 'mouse-2 or RET jump to Org file ~/tmp/test.org' but when I do either of those on the entry test.org file is not loaded. This is in emacs 27.1 & 27.2

This is now a bug in map-text-properties which add duplicate props, I will look at this later.

So no, it is working fine here, I was just using another version of map-text-properties which is not working properly.

-- Thierry

thierryvolpiatto commented 2 years ago

Ensure you erase the org agenda buffer before insertion, also ensure some other emacs/org extensions are not also modifying your org buffer.

map7 commented 2 years ago

On all my tests I close the Org Agenda ASYNC buffer.

I just tested again using Emacs 27.2 with the following fresh config file

;;; Setup package.el
(require 'package)
(setq package-enable-at-startup nil)
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/"))
(unless package--initialized (package-initialize))

;;; Setup use-package
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))

(eval-when-compile
  (require 'use-package))

(setq use-package-always-ensure t)

(defun map-text-properties (props)
  (let ((plist (caddr props)))
    (while plist
      (put-text-property (1+ (nth 0 props))
                         (1+ (nth 1 props))
                         (car plist)
                         (let ((value (cadr plist)))
                           (cond ((and (consp value)
                                       (stringp (car value)))
                                  (with-temp-buffer
                                    (insert (car value))
                                    (map-text-properties (cdr value))
                                    (buffer-string)))
                                 ((and (consp value) (memq 'marker value))
                                  (let ((marker (set-marker
                                                 (make-marker)
                                                 (cl-loop for i in value
                                                          thereis (and (numberp i) i))
                                                 (get-buffer (mapconcat 'symbol-name (last value) "")))))
                                    (set-marker-insertion-type marker t)
                                    marker))
                                 (t value))))
      (setq plist (cddr plist)))
    (when props
      (map-text-properties (nthcdr 3 props)))))

(defun async-org-agenda-list4()
  (interactive)

  (async-start
    ;; What to do in the child process
    (lambda ()
      (setq org-agenda-files '("~/tmp/test.org"))
      (org-agenda-list)
      (buffer-string))

    ;; What to do when it finishes
    (lambda (result)
      (switch-to-buffer-other-window "*Org Agenda ASYNC*")
      ;; In this example, we have only one file in org-agenda-files, but
      ;; we can assume user will want to use the same value of
      ;; org-agenda-files both in parent and child so we can loop in
      ;; org-agenda-files to open all org buffer (org is doing this when
      ;; calling org-agenda).
      (find-file-noselect "~/tmp/test.org")
      (insert (car result))
      (map-text-properties (cdr result))
      (org-agenda-mode))))

When running async-org-agenda-list4 it displays the buffer with colours in the right place, but still clicking on the TODO item doesn't load the test.org file and when hitting enter on that TODO item it splits the line.

The error which is displayed in the mode line just before the Org Agenda ASYNC buffer is displayed is the following;

error in process sentinel: Symbol's function definition is void: org-agenda-mode
thierryvolpiatto commented 2 years ago

Michael Pope @.***> writes:

The error which is displayed in the mode line just before the Org Agenda ASYNC buffer is displayed is the following;

error in process sentinel: Symbol's function definition is void: org-agenda-mode

Require org-agenda both in parent and child.

-- Thierry

map7 commented 2 years ago

It all works now on my test.org, but not on my large collection of TODO jobs.

When I introduce all my todo jobs I had to increase the maximum lisp eval depth as it was hitting that and clicking/hitting return on a TODO item doesn't open up the org file. Instead it opens up Org Agenda ASYNC in another buffer and moves the cursor. So something funny is happening here.

 (setq max-lisp-eval-depth 10000)
(defun async-org-agenda-list4()
  (interactive)

  (async-start
   (lambda ()
     (require 'org-agenda)
     (setq max-lisp-eval-depth 10000)
     (setq org-agenda-files '("~/tmp/test.org"))
     (org-agenda-list)
     (buffer-string))

   (lambda (result)
     (require 'org-agenda)
     (setq max-lisp-eval-depth 10000)
     (switch-to-buffer-other-window "*Org Agenda ASYNC*")
     (find-file-noselect "~/tmp/test.org")
     (insert (car result))
     (map-text-properties (cdr result))
     (org-agenda-mode))))
map7 commented 2 years ago

Found another bug. When adding two org files with two tasks scheduled on the same day I can only click on the first item, the second item doesn't work.

(defun async-org-agenda-list()
  (interactive)

  (async-start
   ;; What to do in the child process
   (lambda ()
     (require 'org-agenda)
     ;; (setq org-agenda-files '("~/org/" "~/org/business/michael" "~/org/projects"))
     ;; (setq org-agenda-files '("~/tmp/test.org"))
     (setq org-agenda-files '("~/tmp"))
     (org-agenda-list)
     (buffer-string))

   ;; What to do when it finishes
   (lambda (result)
     (require 'org-agenda)
     (switch-to-buffer-other-window "*Org Agenda ASYNC*")
     (find-file-noselect "~/tmp/test.org")
     (insert (car result))
     (map-text-properties (cdr result))
     (org-agenda-mode))))

To fix the problem I had to also add

 (find-file-noselect "~/tmp/test2.org")

Does this mean I have to use find-file-noselect on all my org files I add to the agenda?

thierryvolpiatto commented 2 years ago

Michael Pope @.***> writes:

Found another bug. When adding two org files with two tasks scheduled on the same day I can only click on the first item, the second item doesn't work.

(defun async-org-agenda-list() (interactive)

(async-start ;; What to do in the child process (lambda () (require 'org-agenda) ;; (setq org-agenda-files '("~/org/" "~/org/business/michael" "~/org/projects")) ;; (setq org-agenda-files '("~/tmp/test.org")) (setq org-agenda-files '("~/tmp")) (org-agenda-list) (buffer-string))

;; What to do when it finishes (lambda (result) (require 'org-agenda) (switch-to-buffer-other-window "Org Agenda ASYNC") (find-file-noselect "~/tmp/test.org") (insert (car result)) (map-text-properties (cdr result)) (org-agenda-mode))))

To fix the problem I had to also add

(find-file-noselect "~/tmp/test2.org")

Does this mean I have to use find-file-noselect on all my org files I add to the agenda?

Yes. See https://github.com/thierryvolpiatto/emacs-config/blob/main/tv-utils.el#L1604

-- Thierry

thierryvolpiatto commented 2 years ago

Michael Pope @.***> writes:

With the following I no longer get the org-agenda-mode error but I still cannot click on the TODO task. I can hit enter and it will open the TODO task, but not click. So I think it's still missing some properties relating to mouse clicks.

Don't know, here both are working.

-- Thierry