bug#52092: 28.0.60; hs-toggle-hiding does not toggle once folded

[kobarity <kobarity@gmail.com>]

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


Mohammed Sadiq wrote:
> With hs-minor-mode enabled, I could hide the visibility of a block with
> hs-toggle-hiding, but using it again doesn't unfold the region.

There is another problem related to this issue.  `hs-toggle-hiding' is
also bound to shift mouse-2, but it toggles the block where the
current point is located instead of the block where mouse is clicked.
The problem is described below:

1. emacs -Q
2. Open the following C mode file.

#+begin_src C
int
main() {
  sub();
}

int
sub() {
  printf("sub\n");
}
#+end_src

3. M-x hs-minor-mode
4. Move the point to the "printf" line in the "sub" function.
5. Shift mouse-2 in the function "main".
   The function body of "sub" is hidden instead of "main".  This is
   not I expected.

This behavior was introduced by the following commit.  Is this
intended behavior?

commit d0e9113de9783094b4da7f6aeee131194653c325
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Sun May 19 09:36:22 2019 -0400

    * lisp/progmodes/hideshow.el: Simplify mouse binding; Use lexical-binding

    (hs-set-up-overlay, hs-adjust-block-beginning): Use non-nil default for
    function variables, so `add-function` can be used on them.
    (hs-toggle-hiding): Make it work for mouse bindings as well.
    (hs-minor-mode-map): Use it for the mouse binding.
    (hs-grok-mode-type): Use bound-and-true-p.
    (hs-life-goes-on): Use `declare` for the debug spec.
    (hs-mouse-toggle-hiding): Make it an obsolete alias.

(posn-set-point (event-end e)) was added to `hs-toggle-hiding' by this
commit.  This is the main cause of bug#52092.  When the point is
inside the hidden block, (posn-set-point (event-end e)) moves the
point to the first shown character (e.g. closing "}"), resulting in
the failure to show the block.  This is not needed for keyboard
operations.  Although this is necessary for mouse operations, current
code does not handle mouse events because "e" is not specified in
`interactive'.  So I think `hs-mouse-toggle-hiding' should be revived
to separate `posn-set-point' from `hs-toggle-hiding' if the above
behavior is not intentional.

As for the `hs-already-hidden-p', the current implementation returns
nil when the point is at closing "}".  Although this is not a big
problem for keyboard operations, this is harmful for mouse to show the
hidden block because `posn-set-point' sets the point at closing "}" as
described above.  Current implementation only checks if the end of the
line is inside a block.  I suggest to add a check if the beginning of
the line is inside a block to be able detect a hidden block when the
point is at the line of closing "}".

Attached is a patch to fix bug#52092 and the above behavior.  I'm not
sure if it is okay to cancel `define-obsolete-function-alias'.

Regards,

[-- Attachment #2: 0001-Fix-hs-toggle-hiding-behaviors.patch --]
[-- Type: application/octet-stream, Size: 8351 bytes --]

From 902fd35281c394173a9688c4b149f3b909f491c8 Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Mon, 19 Sep 2022 13:43:33 +0900
Subject: [PATCH] Fix hs-toggle-hiding behaviors

* lisp/progmodes/hideshow.el (hs-minor-mode-map): Change shift
mouse-2 binding to `hs-mouse-toggle-hiding'.
(hs-find-block-beginning-match): New function to be used in
`hs-already-hidden-p'.
(hs-already-hidden-p): Add check if beginning of line is inside a
block.
(hs-toggle-hiding): Move mouse specific processing to
`hs-mouse-toggle-hiding'.
(hs-mouse-toggle-hiding): Revived function for mouse binding.

* test/lisp/progmodes/hideshow-tests.el
(hideshow-tests-with-temp-buffer-selected): New helper macro.
(hideshow-tests-make-event-at): New helper function.
(hideshow-already-hidden-p-1): New test.
(hideshow-toggle-hiding-1): New test.
(hideshow-mouse-toggle-hiding-1): New test (bug#52092).
---
 lisp/progmodes/hideshow.el            |  39 ++++++----
 test/lisp/progmodes/hideshow-tests.el | 106 ++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 13 deletions(-)

diff --git a/lisp/progmodes/hideshow.el b/lisp/progmodes/hideshow.el
index c0796fc2ee..1cdb93138e 100644
--- a/lisp/progmodes/hideshow.el
+++ b/lisp/progmodes/hideshow.el
@@ -37,7 +37,7 @@
 ;;   hs-show-all                        C-c @ C-M-s
 ;;   hs-hide-level                      C-c @ C-l
 ;;   hs-toggle-hiding                   C-c @ C-c
-;;   hs-toggle-hiding                   [(shift mouse-2)]
+;;   hs-mouse-toggle-hiding             [(shift mouse-2)]
 ;;   hs-hide-initial-comment-block
 ;;
 ;; Blocks are defined per mode.  In c-mode, c++-mode and java-mode, they
@@ -361,7 +361,7 @@ hs-minor-mode-map
     (define-key map "\C-c@\C-t"       'hs-hide-all)
     (define-key map "\C-c@\C-d"       'hs-hide-block)
     (define-key map "\C-c@\C-e"       'hs-toggle-hiding)
-    (define-key map [(shift mouse-2)] 'hs-toggle-hiding)
+    (define-key map [(shift mouse-2)] 'hs-mouse-toggle-hiding)
     map)
   "Keymap for hideshow minor mode.")
 
@@ -786,6 +786,14 @@ hs-life-goes-on
            (case-fold-search t))
        ,@body)))
 
+(defun hs-find-block-beginning-match ()
+  "Reposition point at the end of match of the block-start regexp.
+Return point, or nil if original point was not in a block."
+  (when (and (funcall hs-find-block-beginning-func)
+	     (funcall hs-looking-at-block-start-p-func))
+    ;; point is inside a block
+    (goto-char (match-end 0))))
+
 (defun hs-overlay-at (position)
   "Return hideshow overlay at POSITION, or nil if none to be found."
   (let ((overlays (overlays-at position))
@@ -802,12 +810,11 @@ hs-already-hidden-p
       (if (and c-reg (nth 0 c-reg))
           ;; point is inside a comment, and that comment is hideable
           (goto-char (nth 0 c-reg))
-        (end-of-line)
-        (when (and (not c-reg)
-                   (funcall hs-find-block-beginning-func)
-		   (funcall hs-looking-at-block-start-p-func))
-          ;; point is inside a block
-          (goto-char (match-end 0)))))
+        (when (not c-reg)
+          (end-of-line)
+          (when (not (hs-find-block-beginning-match))
+            (beginning-of-line)
+            (hs-find-block-beginning-match)))))
     (end-of-line)
     (hs-overlay-at (point))))
 
@@ -944,18 +951,24 @@ hs-hide-level
      (message "Hiding blocks ... done"))
    (run-hooks 'hs-hide-hook)))
 
-(defun hs-toggle-hiding (&optional e)
+(defun hs-toggle-hiding ()
   "Toggle hiding/showing of a block.
-See `hs-hide-block' and `hs-show-block'.
-Argument E should be the event that triggered this action."
+See `hs-hide-block' and `hs-show-block'."
   (interactive)
   (hs-life-goes-on
-   (posn-set-point (event-end e))
    (if (hs-already-hidden-p)
        (hs-show-block)
      (hs-hide-block))))
 
-(define-obsolete-function-alias 'hs-mouse-toggle-hiding #'hs-toggle-hiding "27.1")
+(defun hs-mouse-toggle-hiding (e)
+  "Toggle hiding/showing of a block.
+This command should be bound to a mouse key.
+Argument E is a mouse event used by `event-end'.
+See `hs-hide-block' and `hs-show-block'."
+  (interactive "@e")
+  (hs-life-goes-on
+   (posn-set-point (event-end e))
+   (hs-toggle-hiding)))
 
 (defun hs-hide-initial-comment-block ()
   "Hide the first block of comments in a file.
diff --git a/test/lisp/progmodes/hideshow-tests.el b/test/lisp/progmodes/hideshow-tests.el
index ee2a0c7c4c..d4bcd3c164 100644
--- a/test/lisp/progmodes/hideshow-tests.el
+++ b/test/lisp/progmodes/hideshow-tests.el
@@ -41,6 +41,18 @@ hideshow-tests-with-temp-buffer
      (goto-char (point-min))
      ,@body))
 
+(defmacro hideshow-tests-with-temp-buffer-selected (mode contents &rest body)
+  "Create and switch to a `hs-minor-mode' enabled MODE temp buffer with CONTENTS.
+BODY is code to be executed within the temp buffer.  Point is
+always located at the beginning of buffer."
+  (declare (indent 1) (debug t))
+  `(ert-with-test-buffer-selected ()
+     (,mode)
+     (hs-minor-mode 1)
+     (insert ,contents)
+     (goto-char (point-min))
+     ,@body))
+
 (defun hideshow-tests-look-at (string &optional num restore-point)
   "Move point at beginning of STRING in the current buffer.
 Optional argument NUM defaults to 1 and is an integer indicating
@@ -96,6 +108,39 @@ hideshow-tests-visible-string
                            (overlay-end overlay))))
       (buffer-substring-no-properties (point-min) (point-max)))))
 
+(defun hideshow-tests-make-event-at (string)
+  "Make dummy mouse event at beginning of STRING."
+  (save-excursion
+    (let ((pos (hideshow-tests-look-at string)))
+      (vector
+       `('S-mouse-2
+         (,(get-buffer-window) ,pos (1 . 1) 0 nil ,pos (1 . 1)
+          nil (1 . 1) (1 . 1)))))))
+
+(ert-deftest hideshow-already-hidden-p-1 ()
+  (let ((contents "
+int
+main()
+{
+  printf(\"Hello\\n\");
+}
+"))
+    (hideshow-tests-with-temp-buffer
+     c-mode
+     contents
+     (hideshow-tests-look-at "printf")
+     (should (not (hs-already-hidden-p)))
+     (hs-hide-block)
+     (goto-char (point-min))
+     (hideshow-tests-look-at "{")
+     (should (hs-already-hidden-p))
+     (forward-line -1)
+     (should (not (hs-already-hidden-p)))
+     (hideshow-tests-look-at "}")
+     (should (hs-already-hidden-p))
+     (forward-line)
+     (should (not (hs-already-hidden-p))))))
+
 (ert-deftest hideshow-hide-block-1 ()
   "Should hide current block."
   (let ((contents "
@@ -263,6 +308,67 @@ hideshow-hide-level-2
 }
 "))))
 
+(ert-deftest hideshow-toggle-hiding-1 ()
+  "Should toggle hiding/showing of a block."
+  (let ((contents "
+int
+main()
+{
+  printf(\"Hello\\n\");
+}
+"))
+    (hideshow-tests-with-temp-buffer
+     c-mode
+     contents
+     (hideshow-tests-look-at "printf")
+     (hs-toggle-hiding)
+     (should (string=
+              (hideshow-tests-visible-string)
+              "
+int
+main()
+{}
+"))
+     (hs-toggle-hiding)
+     (should (string= (hideshow-tests-visible-string) contents)))))
+
+(ert-deftest hideshow-mouse-toggle-hiding-1 ()
+  "Should toggle hiding/showing of a block by mouse events."
+  (let ((contents "
+int
+main()
+{
+  printf(\"Hello\\n\");
+}
+")
+        (hidden "
+int
+main()
+{}
+"))
+    (hideshow-tests-with-temp-buffer-selected
+     c-mode
+     contents
+     ;; Should not hide the block when clicked outside of the block.
+     (call-interactively
+      #'hs-mouse-toggle-hiding nil (hideshow-tests-make-event-at "int"))
+     (should (string= (hideshow-tests-visible-string) contents))
+     ;; Should hide the block when clicked inside of the block.
+     (goto-char (point-min))
+     (call-interactively
+      #'hs-mouse-toggle-hiding nil (hideshow-tests-make-event-at "printf"))
+     (should (string= (hideshow-tests-visible-string) hidden))
+     ;; Should not show the block when clicked outside of the block.
+     (goto-char (point-min))
+     (call-interactively
+      #'hs-mouse-toggle-hiding nil (hideshow-tests-make-event-at "int"))
+     (should (string= (hideshow-tests-visible-string) hidden))
+     ;; Should show the block when clicked inside of the block.
+     (goto-char (point-min))
+     (call-interactively
+      #'hs-mouse-toggle-hiding nil (hideshow-tests-make-event-at "}"))
+     (should (string= (hideshow-tests-visible-string) contents)))))
+
 (provide 'hideshow-tests)
 
 ;;; hideshow-tests.el ends here
-- 
2.34.1