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

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

[Mohammed Sadiq]

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.

Tested with c-mode and css-mode.

In GNU Emacs 28.0.60 (build 2, x86_64-pc-linux-gnu, GTK+ Version 
3.24.30, cairo version 1.16.0)
  of 2021-11-25 built on purism
Repository revision: 0854453ec2343cbfac3ac8e233cdc7bd2c8554a9
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 
11.0.12011000
System Description: Debian GNU/Linux bookworm/sid

Configured using:
  'configure --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
   value of $LANG: en_IN
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: iso-latin-1-unix

Major mode: C/*l

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t
   abbrev-mode: t
   hs-minor-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source eieio eieio-core eieio-loaddefs
password-cache json map text-property-search time-date mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail comp comp-cstr warnings subr-x rx cl-seq cl-macs cl-extra
help-mode rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
hideshow vc-git diff-mode easy-mmode vc-dispatcher seq byte-opt gv
bytecomp byte-compile cconv cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 125023 9196)
  (symbols 48 10111 1)
  (strings 32 30461 3484)
  (string-bytes 1 1135663)
  (vectors 16 20448)
  (vector-slots 8 388478 20146)
  (floats 8 31 60)
  (intervals 56 377 0)
  (buffers 992 13))

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

[Eli Zaretskii]

> Date: Thu, 25 Nov 2021 13:10:51 +0530
> From: Mohammed Sadiq <sadiq@sadiqpk.org>
> 
> 
> 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.
> 
> Tested with c-mode and css-mode.

It seems that unhiding works only if you are at the beginning of the
hidden block.  In C Mode, that means cursor is on the opening brace of
the block.

The problem seems to be inside hs-already-hidden-p:

  (defun hs-already-hidden-p ()
    "Return non-nil if point is in an already-hidden block, otherwise nil."
    (save-excursion
      (let ((c-reg (hs-inside-comment-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)

That call to end-of-line moves point outside of the braces _unless_
point is on the left brace that starts the hidden block.  That
movement then makes it appear as if we are in the outer block, not in
the one that you've just hidden.

I don't know enough about hideshow.el and don't use it enough to
propose a robust enough solution for this which won't break some use
case out there.

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

[kobarity]

[-- 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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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.

Hmm... indeed it's a trivial oversight in my patch, sorry.
The patch below should fix it.

> (posn-set-point (event-end e)) was added to `hs-toggle-hiding' by this
> commit.  This is the main cause of bug#52092.

Indeed, thanks.

I had no idea that (posn-point (posn-at-point POS)) doesn't return POS
if POS is within an invisible chunk of text and instead returns the next
visible position.

I'll have to think about how best to fix the consequence.


        Stefan


diff --git a/lisp/progmodes/hideshow.el b/lisp/progmodes/hideshow.el
index c0796fc2eeb..8dd8c7e182c 100644
--- a/lisp/progmodes/hideshow.el
+++ b/lisp/progmodes/hideshow.el
@@ -948,7 +948,7 @@ 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."
-  (interactive)
+  (interactive (list last-nonmenu-event))
   (hs-life-goes-on
    (posn-set-point (event-end e))
    (if (hs-already-hidden-p)

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

[kobarity]

Stefan Monnier wrote:
> The patch below should fix it.

Thanks, I didn't know `last-nonmenu-event'.  I confirmed that the
patch enables the mouse to hide the clicked block.

> I had no idea that (posn-point (posn-at-point POS)) doesn't return POS
> if POS is within an invisible chunk of text and instead returns the next
> visible position.
> 
> I'll have to think about how best to fix the consequence.

Thank you for consideration.

Regards,

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[ Added Thien-Thi and Dan to the Cc since they're listed as maintainers
  of `hideshow.el`.  ]

kobarity [2022-09-24 14:59:56] wrote:
> Stefan Monnier wrote:
>> The patch below should fix it.
> Thanks, I didn't know `last-nonmenu-event'.  I confirmed that the
> patch enables the mouse to hide the clicked block.

I pushed it to the `emacs-28` branch, because it's fixing a stupid regression.

>> I had no idea that (posn-point (posn-at-point POS)) doesn't return POS
>> if POS is within an invisible chunk of text and instead returns the next
>> visible position.
>> 
>> I'll have to think about how best to fix the consequence.
>
> Thank you for consideration.

I pushed to `master` a patch which makes `event-end` return a position
that's always at `point` for keyboard events, even if `point` is hidden.
That should bring back the previous behavior (it's not safe enough for
the `emacs-28` branch, sadly).

But I think your patch is still an improvement since it makes
`hs-toggle-hiding` work even when point is right after the hidden block.
I also like the additional tests you provide.
So I adjusted it to the new code and pushed it to `master`.

Thank you,


        Stefan

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

[kobarity]

Stefan Monnier wrote:
> I pushed to `master` a patch which makes `event-end` return a position
> that's always at `point` for keyboard events, even if `point` is hidden.
> That should bring back the previous behavior (it's not safe enough for
> the `emacs-28` branch, sadly).
> 
> But I think your patch is still an improvement since it makes
> `hs-toggle-hiding` work even when point is right after the hidden block.
> I also like the additional tests you provide.
> So I adjusted it to the new code and pushed it to `master`.

I confirmed that everything works as expected.  Thank you for fixing
the issue and improving my patch.

Regards,

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Mohammed Sadiq <sadiq@sadiqpk.org>,
>   52092@debbugs.gnu.org
> Date: Fri, 23 Sep 2022 18:01:05 -0400
> 
> I had no idea that (posn-point (posn-at-point POS)) doesn't return POS
> if POS is within an invisible chunk of text and instead returns the next
> visible position.

posn-at-point uses display routines, so it cannot report on invisible
POS, because the display code skips invisible text.  The doc string
kinda hints on that:

  Return nil if POS is not visible in WINDOW.  Otherwise,
  the return value is similar to that returned by ‘event-start’ for
  a mouse click at the upper left corner of the glyph corresponding
  to POS:

You cannot click a mouse on a glyph corresponding to invisible text,
can you?

We could make this more clear in the documentation.  I think this
usually isn't a problem because this function is not called for POS
calculated in some way, as opposed to positions actually displayed, or
nil.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> posn-at-point uses display routines, so it cannot report on invisible
> POS, because the display code skips invisible text.  The doc string
> kinda hints on that:

Yes, it makes a lot of sense.  In the case of `event-end` and
`event-start` that's not what we want, tho, so I tweaked those
functions accordingly.


        Stefan