Re: [PATCH 2-v2] New: auto display inline images under subtree when `org-cycle'.

["Christopher M. Miles" <numbchild@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 619 bytes --]


Ihor Radchenko <yantar92@gmail.com> writes:

> "Christopher M. Miles" <numbchild@gmail.com> writes:
>
>> Ihor Radchenko <yantar92@gmail.com> writes:
>>
>> I got a new problem in patch, the (point-max) in function
>> ~org-cycle-display-inline-images~ SOMETIMES return EOF error instead of
>> correct max point value when org-cycle `state' is ~'folded~. I can't
>> find out what caused this even using Edebug. Do you have any clue?
>
> Maybe because the 'folded branch in that function is not wrapped into
> `save-restriction'?
>

Ah, Indeed, I forgot to wrap it. Now I fixed this error.

I attached new version patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-org.el-Support-auto-display-inline-images-when-cycli.patch --]
[-- Type: text/x-patch, Size: 8922 bytes --]

From 570551a39038cf4190cd7cb453a8d059326b9dcb Mon Sep 17 00:00:00 2001
From: stardiviner <numbchild@gmail.com>
Date: Wed, 28 Sep 2022 20:46:52 +0800
Subject: [PATCH] org.el: Support auto display inline images when cycling

* lisp/org.el (org-toggle-inline-images): Support region.
(org-display-inline-images): Fix refresh argument logic.
(org-remove-inline-images): Support region.

* lisp/org-keys.el (org-toggle-inline-images): Update arguments.

* lisp/org-cycle.el (org-cycle-inline-images-display): Add new option to
control whether auto display inline images when cycling.
(org-cycle-display-inline-images): Add new hook function to auto display
inline images when cycling.
(org-cycle-hook): Add `org-cycle-display-inline-images' into cycling
hook by default.

* testing/lisp/test-org-fold.el (test-org-fold/org-fold-display-inline-images):
Add test for inline image displaying when cycling.
---
 lisp/org-cycle.el             | 41 ++++++++++++++++++++++++++++--
 lisp/org-keys.el              |  2 +-
 lisp/org.el                   | 21 +++++++++------
 testing/lisp/test-org-fold.el | 48 +++++++++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/lisp/org-cycle.el b/lisp/org-cycle.el
index 656ca83f2..6204b8a4f 100644
--- a/lisp/org-cycle.el
+++ b/lisp/org-cycle.el
@@ -208,8 +208,9 @@ the values `folded', `children', or `subtree'."
   :type 'hook)
 
 (defcustom org-cycle-hook '(org-cycle-hide-archived-subtrees
-		   org-cycle-show-empty-lines
-		   org-cycle-optimize-window-after-visibility-change)
+                            org-cycle-show-empty-lines
+                            org-cycle-optimize-window-after-visibility-change
+                            org-cycle-display-inline-images)
   "Hook that is run after `org-cycle' has changed the buffer visibility.
 The function(s) in this hook must accept a single argument which indicates
 the new state that was set by the most recent `org-cycle' command.  The
@@ -229,6 +230,12 @@ normal outline commands like `show-all', but not with the cycling commands."
   :group 'org-cycle
   :type 'boolean)
 
+(defcustom org-cycle-inline-images-display nil
+  "Non-nil means auto display inline images under subtree when cycling."
+  :group 'org-startup
+  :group 'org-cycle
+  :type 'boolean)
+
 (defvar org-cycle-tab-first-hook nil
   "Hook for functions to attach themselves to TAB.
 See `org-ctrl-c-ctrl-c-hook' for more information.
@@ -776,6 +783,36 @@ STATE should be one of the symbols listed in the docstring of
 		       "Subtree is archived and stays closed.  Use \
 `\\[org-cycle-force-archived]' to cycle it anyway."))))))
 
+(defun org-cycle-display-inline-images (state)
+  "Auto display inline images under subtree when cycling.
+It works when `org-cycle-inline-images-display' is non-nil."
+  (when org-cycle-inline-images-display
+    (pcase state
+      ('children
+       (save-excursion
+         (save-restriction
+           (org-narrow-to-subtree)
+           ;; If has nested headlines, beg,end only from parent
+           ;; headline to first child headline which reference to
+           ;; upper let-binding `org-next-visible-heading'.
+           (org-display-inline-images
+            nil nil
+            (point-min) (progn (org-next-visible-heading 1) (point))))))
+      ('subtree
+       (save-excursion
+         (save-restriction
+           (org-narrow-to-subtree)
+           ;; If has nested headlines, also inline display images under all sub-headlines.
+           (org-display-inline-images nil nil (point-min) (point-max)))))
+      ('folded
+       (save-excursion
+         (save-restriction
+           (org-narrow-to-subtree)
+           ;; FIXME: sometimes `point-max' is EOF.
+           (if (numberp (point-max))
+               (org-remove-inline-images (point-min) (point-max))
+             (ignore))))))))
+
 (provide 'org-cycle)
 
 ;;; org-cycle.el ends here
diff --git a/lisp/org-keys.el b/lisp/org-keys.el
index d65379a72..79e34cbd1 100644
--- a/lisp/org-keys.el
+++ b/lisp/org-keys.el
@@ -204,7 +204,7 @@
 (declare-function org-toggle-radio-button "org" (&optional arg))
 (declare-function org-toggle-comment "org" ())
 (declare-function org-toggle-fixed-width "org" ())
-(declare-function org-toggle-inline-images "org" (&optional include-linked))
+(declare-function org-toggle-inline-images "org" (&optional include-linked beg end))
 (declare-function org-latex-preview "org" (&optional arg))
 (declare-function org-toggle-narrow-to-subtree "org" ())
 (declare-function org-toggle-ordered-property "org" ())
diff --git a/lisp/org.el b/lisp/org.el
index 036384a04..36db62f0c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16071,16 +16071,16 @@ SNIPPETS-P indicates if this is run to create snippet images for HTML."
 
 (defvar-local org-inline-image-overlays nil)
 
-(defun org-toggle-inline-images (&optional include-linked)
+(defun org-toggle-inline-images (&optional include-linked beg end)
   "Toggle the display of inline images.
 INCLUDE-LINKED is passed to `org-display-inline-images'."
   (interactive "P")
   (if org-inline-image-overlays
       (progn
-	(org-remove-inline-images)
+	(org-remove-inline-images beg end)
 	(when (called-interactively-p 'interactive)
 	  (message "Inline image display turned off")))
-    (org-display-inline-images include-linked)
+    (org-display-inline-images include-linked nil beg end)
     (when (called-interactively-p 'interactive)
       (message (if org-inline-image-overlays
 		   (format "%d images displayed inline"
@@ -16170,8 +16170,8 @@ BEG and END define the considered part.  They default to the
 buffer boundaries with possible narrowing."
   (interactive "P")
   (when (display-graphic-p)
-    (unless refresh
-      (org-remove-inline-images)
+    (when refresh
+      (org-remove-inline-images beg end)
       (when (fboundp 'clear-image-cache) (clear-image-cache)))
     (let ((end (or end (point-max))))
       (org-with-point-at (or beg (point-min))
@@ -16322,11 +16322,16 @@ buffer boundaries with possible narrowing."
       (delete ov org-inline-image-overlays)
       (delete-overlay ov))))
 
-(defun org-remove-inline-images ()
+(defun org-remove-inline-images (&optional beg end)
   "Remove inline display of images."
   (interactive)
-  (mapc #'delete-overlay org-inline-image-overlays)
-  (setq org-inline-image-overlays nil))
+  (let* ((beg (or beg (point-min)))
+         (end (or end (point-max)))
+         (overlays (overlays-in beg end)))
+    (dolist (ov overlays)
+      (when (memq ov org-inline-image-overlays)
+        (setq org-inline-image-overlays (delq ov org-inline-image-overlays))
+        (delete-overlay ov)))))
 
 (defvar org-self-insert-command-undo-counter 0)
 (defvar org-speed-command nil)
diff --git a/testing/lisp/test-org-fold.el b/testing/lisp/test-org-fold.el
index 0fa800dee..5c47cafb4 100644
--- a/testing/lisp/test-org-fold.el
+++ b/testing/lisp/test-org-fold.el
@@ -604,6 +604,54 @@ Unfolded Paragraph.
             (org-fold-check-before-invisible-edit kind)
             (should (org-invisible-p (1- (point))))))))))
 
+;;; TODO: reference `test-org-fold/org-fold-reveal-broken-structure'
+(ert-deftest test-org-fold/org-fold-display-inline-images ()
+  "Test inline images displaying when cycling."
+  (let ((org-fold-core-style 'text-properties)
+        (org-cycle-inline-images-display t))
+    ;; `org-cycle' -(state)-> `'children' display child inline images.
+    (org-test-with-temp-text
+     "<point>* Heading 1
+[[file:image.png]]"
+     (org-overview)
+     (org-cycle)
+     (org-next-link)
+     (should (overlays-at (point)))
+     (org-toggle-inline-images)
+     (should-not (overlays-at (point))))
+    ;; `org-cycle' -(state)-> `'subtree' display subtrees inline images.
+    (org-test-with-temp-text
+     "<point>* Heading 1
+[[file:image.png]]
+** Subheading 1
+[[file:image2.png]]
+** Subheading 2
+[[file:image3.png]]"
+     (org-overview)
+     (org-cycle)
+     (org-cycle)
+     (org-next-link)
+     (org-next-link)
+     (should (overlays-at (point)))
+     (org-toggle-inline-images)
+     (should-not (overlays-at (point))))
+    ;; `org-cycle' -(state)-> `'folded' remove inline image overlays.
+    (org-test-with-temp-text
+     "<point>* Heading 1
+[[file:image.png]]
+** Subheading 1
+[[file:image2.png]]
+** Subheading 2
+[[file:image3.png]]"
+     (org-overview)
+     (org-cycle)
+     (org-cycle)
+     (org-cycle)
+     ;; TODO: how to detect inline image overlays are deleted after folded?
+     (should (overlays-at (point)))
+     (org-toggle-inline-images)
+     (should-not (overlays-at (point))))))
+
 (provide 'test-org-fold)
 
 ;;; test-org-fold.el ends here
-- 
2.37.2


[-- Attachment #1.3: Type: text/plain, Size: 1005 bytes --]


>>> You are providing both hook and a customization. It is redundant. Users
>>> can already remove the hook when desired.
>>
>> I mock this style from a very similar existing hook function
>> ~org-cycle-hide-archived-subtrees~ and custom variable
>> ~org-cycle-open-archived-trees~. I think removing a hook function from
>> ~org-cycle-hook~ is a way, but not as convenient as defcustom option.
>> (P.S: I think users prefer this defcustom customization style.)
>
> I see your point.
> I agree that having a variable is better and also leaves an option to
> introduce in-buffer setting for this.
>
> Let's leave the hook part for now - if we want to alter it, we may need
> to re-consider all the default hooks.

Thanks

-- 

[ stardiviner ]
I try to make every word tell the meaning that I want to express without misunderstanding.

Blog: https://stardiviner.github.io/
IRC(libera.chat, freenode): stardiviner, Matrix: stardiviner
GPG: F09F650D7D674819892591401B5DF1C95AE89AC3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]