GNU bug report logs - #79294
[PATCH] Add hideable indicators for hideshow.

Previous Next

To reply to this bug, email your comments to 79294 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add hideable indicators for hideshow.
Date: Fri, 22 Aug 2025 22:56:33 -0600

[Message part 1 (text/plain, inline)]

This patch adds indicators to toggle source block visibility (as shown
in the emacs-devel thread).

Currently the implementation makes scrolling a bit slow, due to the
computation required to get the hideable blocks.

I'm not sure if there is a way to add the indicators lazily or a better
solution.

[0001-Add-hideable-indicators-for-hideshow.-Bug.patch (text/patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
- E.G via Gnus and Org.

Message #8 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sat, 23 Aug 2025 11:00:37 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Date: Fri, 22 Aug 2025 22:56:33 -0600
> 
> This patch adds indicators to toggle source block visibility (as shown
> in the emacs-devel thread).
> 
> Currently the implementation makes scrolling a bit slow, due to the
> computation required to get the hideable blocks.

How slow?  Can you post some measurements, comparing the scroll times
with and without the indicators?

> +@vindex hs-indicators-type

I think this variable should be named "hs-indicator-type", in
singular.

> +@item hs-show-indicators
> +This variable specifies if hideshow should display indicators to toggle
> +the block hiding.

This sentence reads awkward ("indicators to toggle") because you
conflated two separate features together.  I would leave them
separate:

  This variable controls whether Hideshow mode should display
  indicators of hidden and shown blocks.  The indicators also allow
  toggling the hide/show state of each block.

>                     If this is set to non-@code{nil}, the indicators are
> +enabled except in comments, otherwise, if set to
> +@code{include-comments}, the indicators will be displayed also in
> +comments.

It is better to design this the other way around:

  If the value is @code{t}, the indicators are enabled.  The special
  value @code{exclude-comments} enables the indicators in all blocks
  except comment blocks.  The default is @code{nil}, which disables
  the indicators.

Note that this also documents the default value.

> +@item hs-indicactors-type
> +This variable specifies which indicator type to use.  Its value should
> +be either @code{left-fringe} (display the indicators in the left
> +fringe), @code{right-fringe} (display the indicators in the right
> +fringe), @code{left-margin} (display the indicators in the left margin),
> +@code{right-margin} (display the indicators in the right margin), or
> +@code{nil} (display a string as indicator after the block beginning).
> +If @code{left-fringe} or @code{right-fringe} are selected and current
> +Emacs session does not support fringes, margins will be used instead.

This is too detailed for such a minor feature.  I would simply say

  This variable controls where to show the indicators, if they are
  enabled.  You can show them on the fringe (@pxref{Windows Fringes})
  or in the window's margin.  The default is to use the fringe if it's
  available, otherwise to use the margin.

The other details are clear from the variable's doc string, and need
not be repeated here.

Note that I deliberately omitted the left/right part, because I think
your current design is wrong in this area, see below.

> ++++
>  *** New user option 'hs-display-lines-hidden'.
>  If this option is non-nil, Hideshow displays the number of hidden
>  lines next to the ellipsis.

Please tell the default value.

> ++++
> +*** New user option 'hs-show-indicators'.
> +This user option determines if hideshow should display indicators to
> +toggle the block hiding. If non-nil, the indicators are enabled except
> +in comments.           ^^

Two spaces between sentences.  In addition "in comments" is
problematic here, as the indicators are not part of text.  Did you
mean "for comment blocks" or something?

> +The new icons 'hs-indicator-show' and 'hs-indicator-hide', can be used
> +for customize the indicators appearance, only if 'hs-indicators-type' is
   ^^^^^^^^^^^^^
Either "to customize" or "for customizing".

> +set to 'left-margin', 'right-margin' or nil.
> +
>  ** C-ts mode
>  
>  +++
> diff --git a/lisp/progmodes/hideshow.el b/lisp/progmodes/hideshow.el
> index 445bdeeb7a7..4407cb78b94 100644
> --- a/lisp/progmodes/hideshow.el
> +++ b/lisp/progmodes/hideshow.el
> @@ -220,6 +220,9 @@
>  
>  ;;; Code:
>  (require 'mule-util) ; For `truncate-string-ellipsis'
> +;; For indicators
> +(require 'icons)
> +(require 'fringe)
>  
>  ;;---------------------------------------------------------------------------
>  ;; user-configurable variables
> @@ -236,6 +239,16 @@ hs-ellipsis
>  use that face for the ellipsis instead."
>    :version "31.1")
>  
> +(defface hs-indicator-hide
> +  '((t :inherit (shadow default)))
> +  "Face used in hideshow indicator to hide current block."

Not "to show", but "to indicate a shown block".

> +  :version "31.1")
> +
> +(defface hs-indicator-show
> +  '((t :inherit hs-indicator-hide :weight bold))
> +  "Face used in hideshow indicator to show current block."

Same here.

> +(defcustom hs-show-indicators nil
> +  "Whether hideshow should display block indicators.

I'd say

  Whether hideshow should display block hide/show indicators.

> +If non-nil, hideshow will display indicators for toggle the
> +visibility of a block.                       ^^^^^^^^^^

"for toggling"

> +If set to `include-comments', the indicators are displayed also in
> +comments.

See the comment above about "in comments".  Also, I would design the
values the other way around: the value t shows indicators everywhere,
the value exclude-comments excludes comment blocks.

> +(defcustom hs-indicators-type 'left-fringe
> +  "Indicate which indicator type to use for the block indicators.
> +
> +The possible values can be:
> +
> + - `left-fringe', display the indicators in the left fringe.
> + - `right-fringe', display the indicators in the right fringe.
> +
> + - `left-margin', display the indicators in the left margin.
> + - `right-margin', display the indicators in the right margin.
> +
> + - nil, display a string as indicator after the block beginning.
> +
> +If `left-fringe' or `right-fringe' are selected and fringes are not
> +supported, margins will be used instead.
> +
> +This only have effect if `hs-show-indicators' is non-nil."
> +  :type '(choice
> +          (const :tag "Left fringe" left-fringe)
> +          (const :tag "Right fringe" right-fringe)
> +          (const :tag "Left margin" left-margin)
> +          (const :tag "Right margin" right-margin)
> +          (const :tag "String after block beginning." nil))
> +  :version "31.1")

I think this is sub-optimal, if not fundamentally wrong.  In a
left-to-right text (which is an overwhelmingly the usual case), I
don't expect users to want to show the indicators on the right.  OTOH,
when the text is right-to-left, the indicators should be on the right.
Also, when showing the "show" indicator on the right, the direction of
the arrow should be reversed: it should point to the left, not to the
right.

So I think we should allow just two values: 'fringe' and 'margin', and
determine the left/right part from the actual text directionality.  In
the remote possibility that we would like to allow the indicators "on
the opposite part" of the display (but why? it sounds like unnecessary
feature to me), we could have values like 'opposite-fringe' etc.  The
main point is to avoid "left" and "right" when we choose which one to
use dynamically.

> +(define-icon hs-indicator-hide nil
> +  `((image "outline-open.svg" "outline-open.pbm"
> +           :face hs-indicator-hide
> +           :height (0.6 . em)
> +           :ascent center)
> +    (symbol "🞃" "▶")
> +    (text "-"))
> +  "Icon used for hide block at point.
> +This is only used if `hs-indicators-type' is set to `left-margin',
> +`right-margin' or nil."
> +  :version "31.1")
> +
> +(define-icon hs-indicator-show nil
> +  `((image "outline-close.svg" "outline-close.pbm"
> +           :face hs-indicator-show
> +           :height (0.6 . em)
> +           :ascent center)
> +    (symbol "▸" "▶")
> +    (text "+"))
> +  "Icon used for show block at point.

The image and symbol parts of the "show" indicators should support the
right margin with an arrow pointing to the left, so we'd need
additional images for that.

Also, the "▶" symbol is the same for both hide and show indicators?
Is this intentional?

> +(defvar hs--indicators-rx
> +  `((hs-discard-indicator-overlays)
> +    (,(lambda (bound)
> +        (funcall hs-find-next-block-func
> +                 hs-block-start-regexp
> +                 bound
> +                 (unless (eq hs-show-indicators 'exclude-comments)
> +                   t)))
> +     0 (hs--add-indicators)))
> +  "Hideshow indicator regexp for `font-lock-keywords'.")

Why do we need the value in a defvar, instead of invoking the function
directly?

> +              p q)
> +          ;; `p' is the point at the end of the block beginning, which
> +          ;; may need to be adjusted
> +          (save-excursion
> +            (goto-char (funcall (or hs-adjust-block-beginning #'identity)
> +                                header-end))
> +            (setq p (line-end-position)))
> +          ;; `q' is the point at the end of the block

Can you name these variables by more meaningful names, like
pt-at-block-beg and pt-at-block-end?

> +        (when hs-show-indicators
> +          (when-let* ((type
> +                       (if (not (display-graphic-p))
> +                           'left-margin hs-indicators-type)))
> +            (setq-local
> +             hs-indicators-type
> +             (cond
> +              ((not (eq (current-bidi-paragraph-direction) 'right-to-left)) type)
> +              ((eq hs-indicators-type 'left-fringe) 'right-fringe)
> +              ((eq hs-indicators-type 'left-margin) 'right-margin)
> +              (t hs-indicators-type))))
> +
> +          (font-lock-add-keywords nil hs--indicators-rx)

I think the fact that you modify overlays inside font-lock is the main
reason for this being slow: it requires an immediate second redisplay
cycle after a new window-full was displayed.  It's the same problem as
the one which made linenum-mode so slow.  Did you try using
pre-redisplay-function(s) for this purpose instead?

Message #11 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sat, 23 Aug 2025 23:15:00 -0600

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
>> Date: Fri, 22 Aug 2025 22:56:33 -0600
>> 
>> This patch adds indicators to toggle source block visibility (as shown
>> in the emacs-devel thread).
>> 
>> Currently the implementation makes scrolling a bit slow, due to the
>> computation required to get the hideable blocks.
>
> How slow?  Can you post some measurements, comparing the scroll times
> with and without the indicators?

Well, since i have fast-but-imprecise-scrolling enabled, i've noticed a
few chunks of text unfontified when scrolling faster, and it gets a
little stuck, this doesn't happen if i disable the feature.

> [...]

>> ++++
>> +*** New user option 'hs-show-indicators'.
>> +This user option determines if hideshow should display indicators to
>> +toggle the block hiding. If non-nil, the indicators are enabled except
>> +in comments.           ^^
>
> Two spaces between sentences.  In addition "in comments" is
> problematic here, as the indicators are not part of text.  Did you
> mean "for comment blocks" or something?

Yes, comment blocks.

[...]

>> +(defcustom hs-indicators-type 'left-fringe
>> +  "Indicate which indicator type to use for the block indicators.
>> +
>> +The possible values can be:
>> +
>> + - `left-fringe', display the indicators in the left fringe.
>> + - `right-fringe', display the indicators in the right fringe.
>> +
>> + - `left-margin', display the indicators in the left margin.
>> + - `right-margin', display the indicators in the right margin.
>> +
>> + - nil, display a string as indicator after the block beginning.
>> +
>> +If `left-fringe' or `right-fringe' are selected and fringes are not
>> +supported, margins will be used instead.
>> +
>> +This only have effect if `hs-show-indicators' is non-nil."
>> +  :type '(choice
>> +          (const :tag "Left fringe" left-fringe)
>> +          (const :tag "Right fringe" right-fringe)
>> +          (const :tag "Left margin" left-margin)
>> +          (const :tag "Right margin" right-margin)
>> +          (const :tag "String after block beginning." nil))
>> +  :version "31.1")
>
> I think this is sub-optimal, if not fundamentally wrong.  In a
> left-to-right text (which is an overwhelmingly the usual case), I
> don't expect users to want to show the indicators on the right.  OTOH,
> when the text is right-to-left, the indicators should be on the right.
> Also, when showing the "show" indicator on the right, the direction of
> the arrow should be reversed: it should point to the left, not to the
> right.
>
> So I think we should allow just two values: 'fringe' and 'margin', and
> determine the left/right part from the actual text directionality.  In
> the remote possibility that we would like to allow the indicators "on
> the opposite part" of the display (but why? it sounds like unnecessary
> feature to me), we could have values like 'opposite-fringe' etc.  The
> main point is to avoid "left" and "right" when we choose which one to
> use dynamically.

I agree, I have attempted to follow the same behavior as in flymake, but
I think it is better to hardcode this.

Furthermore, since hideshow is intended to be used in programming
language modes, I don't think it's necessary to change the orientation
of the indicators for bidirectional text.  There are very few cases (or
non-existent) in which a programming language uses (or supports)
right-to-left text, (maybe for code comments?, I don't know).  Compared
to outline, which is intended to be used anywhere, so the orientation it
handles makes more sense in outline than in hideshow.

>> +(define-icon hs-indicator-hide nil
>> +  `((image "outline-open.svg" "outline-open.pbm"
>> +           :face hs-indicator-hide
>> +           :height (0.6 . em)
>> +           :ascent center)
>> +    (symbol "🞃" "▶")
>> +    (text "-"))
>> +  "Icon used for hide block at point.
>> +This is only used if `hs-indicators-type' is set to `left-margin',
>> +`right-margin' or nil."
>> +  :version "31.1")
>> +
>> +(define-icon hs-indicator-show nil
>> +  `((image "outline-close.svg" "outline-close.pbm"
>> +           :face hs-indicator-show
>> +           :height (0.6 . em)
>> +           :ascent center)
>> +    (symbol "▸" "▶")
>> +    (text "+"))
>> +  "Icon used for show block at point.
>
> The image and symbol parts of the "show" indicators should support the
> right margin with an arrow pointing to the left, so we'd need
> additional images for that.

Sure.

> Also, the "▶" symbol is the same for both hide and show indicators?
> Is this intentional?

No, I forgot to change it.

>> +(defvar hs--indicators-rx
>> +  `((hs-discard-indicator-overlays)
>> +    (,(lambda (bound)
>> +        (funcall hs-find-next-block-func
>> +                 hs-block-start-regexp
>> +                 bound
>> +                 (unless (eq hs-show-indicators 'exclude-comments)
>> +                   t)))
>> +     0 (hs--add-indicators)))
>> +  "Hideshow indicator regexp for `font-lock-keywords'.")
>
> Why do we need the value in a defvar, instead of invoking the function
> directly?

I don't understand this, is not this how the keywords should be appended
to font-lock-keywords?

>> +              p q)
>> +          ;; `p' is the point at the end of the block beginning, which
>> +          ;; may need to be adjusted
>> +          (save-excursion
>> +            (goto-char (funcall (or hs-adjust-block-beginning #'identity)
>> +                                header-end))
>> +            (setq p (line-end-position)))
>> +          ;; `q' is the point at the end of the block
>
> Can you name these variables by more meaningful names, like
> pt-at-block-beg and pt-at-block-end?

Done.

>> +        (when hs-show-indicators
>> +          (when-let* ((type
>> +                       (if (not (display-graphic-p))
>> +                           'left-margin hs-indicators-type)))
>> +            (setq-local
>> +             hs-indicators-type
>> +             (cond
>> +              ((not (eq (current-bidi-paragraph-direction) 'right-to-left)) type)
>> +              ((eq hs-indicators-type 'left-fringe) 'right-fringe)
>> +              ((eq hs-indicators-type 'left-margin) 'right-margin)
>> +              (t hs-indicators-type))))
>> +
>> +          (font-lock-add-keywords nil hs--indicators-rx)
>
> I think the fact that you modify overlays inside font-lock is the main
> reason for this being slow: it requires an immediate second redisplay
> cycle after a new window-full was displayed.  It's the same problem as
> the one which made linenum-mode so slow.  Did you try using
> pre-redisplay-function(s) for this purpose instead?

Thanks for the explanation, i will try to use `pre-redisplay-functions`
instead, but i'm not sure how to add the indicators to only the visible
part of the buffer (that is why I choose to use font-lock for this, so
the performance would be somewhat normal).

I don't know if using jit-lock-functions instead would be better or
would have the same result.

-- 
- E.G via Gnus and Org.

Message #14 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sun, 24 Aug 2025 09:47:06 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Cc: 79294 <at> debbugs.gnu.org
> Date: Sat, 23 Aug 2025 23:15:00 -0600
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > So I think we should allow just two values: 'fringe' and 'margin', and
> > determine the left/right part from the actual text directionality.  In
> > the remote possibility that we would like to allow the indicators "on
> > the opposite part" of the display (but why? it sounds like unnecessary
> > feature to me), we could have values like 'opposite-fringe' etc.  The
> > main point is to avoid "left" and "right" when we choose which one to
> > use dynamically.
> 
> I agree, I have attempted to follow the same behavior as in flymake, but
> I think it is better to hardcode this.
> 
> Furthermore, since hideshow is intended to be used in programming
> language modes, I don't think it's necessary to change the orientation
> of the indicators for bidirectional text.

If Hideshow supports only modes for programming languages, then yes,
we only need the indicators on the left.

> >> +(defvar hs--indicators-rx
> >> +  `((hs-discard-indicator-overlays)
> >> +    (,(lambda (bound)
> >> +        (funcall hs-find-next-block-func
> >> +                 hs-block-start-regexp
> >> +                 bound
> >> +                 (unless (eq hs-show-indicators 'exclude-comments)
> >> +                   t)))
> >> +     0 (hs--add-indicators)))
> >> +  "Hideshow indicator regexp for `font-lock-keywords'.")
> >
> > Why do we need the value in a defvar, instead of invoking the function
> > directly?
> 
> I don't understand this, is not this how the keywords should be appended
> to font-lock-keywords?

font-lock-keywords supports elements that call functions, see the
ELisp manual where font-lock-keywords is described.

> > I think the fact that you modify overlays inside font-lock is the main
> > reason for this being slow: it requires an immediate second redisplay
> > cycle after a new window-full was displayed.  It's the same problem as
> > the one which made linenum-mode so slow.  Did you try using
> > pre-redisplay-function(s) for this purpose instead?
> 
> Thanks for the explanation, i will try to use `pre-redisplay-functions`
> instead, but i'm not sure how to add the indicators to only the visible
> part of the buffer (that is why I choose to use font-lock for this, so
> the performance would be somewhat normal).

You can use window-start and add the indicators to some reasonable
vicinity of it.  There's no need to do that _only_ in the visible part
(and using the font-lock machinery will not limit the indicators only
to the visible part, either).

Message #17 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: eg642616 <at> gmail.com
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sat, 06 Sep 2025 11:05:49 +0300

Ping!  How can we make some progress in this matter?

> Cc: 79294 <at> debbugs.gnu.org
> Date: Sun, 24 Aug 2025 09:47:06 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> > Cc: 79294 <at> debbugs.gnu.org
> > Date: Sat, 23 Aug 2025 23:15:00 -0600
> > 
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> > 
> > > So I think we should allow just two values: 'fringe' and 'margin', and
> > > determine the left/right part from the actual text directionality.  In
> > > the remote possibility that we would like to allow the indicators "on
> > > the opposite part" of the display (but why? it sounds like unnecessary
> > > feature to me), we could have values like 'opposite-fringe' etc.  The
> > > main point is to avoid "left" and "right" when we choose which one to
> > > use dynamically.
> > 
> > I agree, I have attempted to follow the same behavior as in flymake, but
> > I think it is better to hardcode this.
> > 
> > Furthermore, since hideshow is intended to be used in programming
> > language modes, I don't think it's necessary to change the orientation
> > of the indicators for bidirectional text.
> 
> If Hideshow supports only modes for programming languages, then yes,
> we only need the indicators on the left.
> 
> > >> +(defvar hs--indicators-rx
> > >> +  `((hs-discard-indicator-overlays)
> > >> +    (,(lambda (bound)
> > >> +        (funcall hs-find-next-block-func
> > >> +                 hs-block-start-regexp
> > >> +                 bound
> > >> +                 (unless (eq hs-show-indicators 'exclude-comments)
> > >> +                   t)))
> > >> +     0 (hs--add-indicators)))
> > >> +  "Hideshow indicator regexp for `font-lock-keywords'.")
> > >
> > > Why do we need the value in a defvar, instead of invoking the function
> > > directly?
> > 
> > I don't understand this, is not this how the keywords should be appended
> > to font-lock-keywords?
> 
> font-lock-keywords supports elements that call functions, see the
> ELisp manual where font-lock-keywords is described.
> 
> > > I think the fact that you modify overlays inside font-lock is the main
> > > reason for this being slow: it requires an immediate second redisplay
> > > cycle after a new window-full was displayed.  It's the same problem as
> > > the one which made linenum-mode so slow.  Did you try using
> > > pre-redisplay-function(s) for this purpose instead?
> > 
> > Thanks for the explanation, i will try to use `pre-redisplay-functions`
> > instead, but i'm not sure how to add the indicators to only the visible
> > part of the buffer (that is why I choose to use font-lock for this, so
> > the performance would be somewhat normal).
> 
> You can use window-start and add the indicators to some reasonable
> vicinity of it.  There's no need to do that _only_ in the visible part
> (and using the font-lock machinery will not limit the indicators only
> to the visible part, either).
> 
> 
> 
> 

Message #20 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Thu, 11 Sep 2025 21:54:30 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

> Ping!  How can we make some progress in this matter?

Sorry for the delay, I have been very busy and have not had time to
respond.

I tried using `pre-redisplay-functions`, but performance was worse.

I was able to mitigate the poor performance by removing the indicators
for comment blocks (that implementation was broken and very slow) and
adding a limit for the indicators.

I also tried using the new feature in larger files for a while to test
its performance (since there were no volunteers), and from what I've
tested, it is stable.

At the moment, the implementation can now be installed.

[0001-Add-hideable-indicators-for-hideshow.-Bug-79294.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
- E.G via Gnus and Org.

Message #23 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sat, 13 Sep 2025 13:30:02 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Cc: 79294 <at> debbugs.gnu.org
> Date: Thu, 11 Sep 2025 21:54:30 -0600
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Ping!  How can we make some progress in this matter?
> 
> Sorry for the delay, I have been very busy and have not had time to
> respond.
> 
> I tried using `pre-redisplay-functions`, but performance was worse.
> 
> I was able to mitigate the poor performance by removing the indicators
> for comment blocks (that implementation was broken and very slow) and
> adding a limit for the indicators.
> 
> I also tried using the new feature in larger files for a while to test
> its performance (since there were no volunteers), and from what I've
> tested, it is stable.
> 
> At the moment, the implementation can now be installed.

Thanks, I have a few minor comments, and then we can install this.

> +@item hs-show-indicators
> +This variable controls whether Hideshow mode should display indicators
> +of hidden and shown blocks.  The indicators also allow toggling the
> +hide/show state of each block. If the value is non-@code{nil}, enables
> +the indicators in all blocks except comment blocks.  The default is
> +@code{nil}, which disables the indicators.

You don't mention the value include-comments here.  It's okay not to
mention it for brevity, but t6hen we should not say "all blocks except
comment blocks", just that non-nil shows them and nil doesn't.

> +@item hs-indicator-maximum-buffer-size
> +This variable determines if the indicators should be enabled if the
> +current buffer size is not larger than this variable value.  If set to
> +@code{nil}, the indicators will be activated regardless of the buffer
> +size.  The default value is 2000000 bytes (2 megabytes).

I suggest to rephrase as follows:

  This variable limits the display of hideshow indicators to buffers
  that are not too large.  (Larger buffers might adversely affect
  redisplay performance.)  By default, buffers larger than 2MB have
  the indicators disabled; the value of nil will activate the
  indicators regardless of the buffer size.

> +*** New user option 'hs-indicator-maximum-buffer-size'.
> +This variable determines whether the indicators should be enabled if the
> +current buffer size is not larger than this variable value.  If set to
> +nil, the indicators will be activated regardless of the buffer size.
> +The default value is 2000000 bytes (2 megabytes).

Suggest the same rephrase here.

> +*** New user option 'hs-indicators-type'.
> +This user option determine which indicator type should be used for the
> +block indicators.
> +
> +The possible values can be: 'left-fringe', display the indicators in the
> +left fringe (the default); 'right-fringe', display the indicators in the
> +right fringe; 'left-margin', display the indicators in the left margin;
> +'right-margin', display the indicators in the right margin; nil, display
> +a string as indicator after the block beginning.

This needs to be updated, since the values are now different.

> +(defun hs-block-positions (&optional beg)
> +  "Return block positions.
> +If BEG is defined, start from BEG."
   ^^^^^^^^^^^^^^^^^
"If BEG is non-nil" is our style.

And the doc string should also explain:

  . what do you mean by "block positions" -- is it a list of positions?
  . what is the significance and the role of the starting position
  . where does the function start if BEG is nil or omitted

> +(defun hs--make-indicators-overlays (beg end)
> +  "Helper function for make the indicators overlays."
                      ^^^^^^^^
Either "to make" or "for making".

> +  (let* ((o (make-overlay
> +             ;; FIXME: The mouse-1 event doesn't work well for fringes,
> +             ;; a workaround for this is set the overlay at the
> +             ;; beginning of the line only for fringe indicators
> +             (if (eq hs-indicator-type 'fringe) (pos-bol) beg)
> +             (1+ beg)))

What doesn't work well in mouse-1 clicks on the fringes?

Message #26 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Sun, 14 Sep 2025 19:50:02 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> +@item hs-show-indicators
>> +This variable controls whether Hideshow mode should display indicators
>> +of hidden and shown blocks.  The indicators also allow toggling the
>> +hide/show state of each block. If the value is non-@code{nil}, enables
>> +the indicators in all blocks except comment blocks.  The default is
>> +@code{nil}, which disables the indicators.
>
> You don't mention the value include-comments here.  It's okay not to
> mention it for brevity, but t6hen we should not say "all blocks except
> comment blocks", just that non-nil shows them and nil doesn't.

I removed the include-comments value from the code, so this mean that
the indicators will not be enabled in comment blocks, that's why I had
to specify it.

I've already updated that part.

[...]


>> +*** New user option 'hs-indicators-type'.
>> +This user option determine which indicator type should be used for the
>> +block indicators.
>> +
>> +The possible values can be: 'left-fringe', display the indicators in the
>> +left fringe (the default); 'right-fringe', display the indicators in the
>> +right fringe; 'left-margin', display the indicators in the left margin;
>> +'right-margin', display the indicators in the right margin; nil, display
>> +a string as indicator after the block beginning.
>
> This needs to be updated, since the values are now different.

Thanks i forgot to update it.

> And the doc string should also explain:
>
>   . what do you mean by "block positions" -- is it a list of positions?
>   . what is the significance and the role of the starting position
>   . where does the function start if BEG is nil or omitted

Thanks, I've updated it.

The BEG argument was intended for internal use only, and I forgot to
remove it.

>> +  (let* ((o (make-overlay
>> +             ;; FIXME: The mouse-1 event doesn't work well for fringes,
>> +             ;; a workaround for this is set the overlay at the
>> +             ;; beginning of the line only for fringe indicators
>> +             (if (eq hs-indicator-type 'fringe) (pos-bol) beg)
>> +             (1+ beg)))
>
> What doesn't work well in mouse-1 clicks on the fringes?

Apparently the fringe keymap does not work if the overlay is not created
from BOL:

e.g.

The mouse-1 clicks works for this since it is created at BOL:

#+begin_src elisp
|+| (defvar-local var nil
| |  "Docstring.")
#+end_src

But for code blocks that are not at BOL this will not work:

#+begin_src elisp
|+| (defvar-local var ; <- This works
|+|   (when foo ; <- This does not
| |     (bar))
| |  "Docstring.")
#+end_src

I have added a new alternative in the patch, and for the moment, it
should work well for fringes.

[0001-Add-hideable-indicators-for-hideshow.-Bug-79294.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

--
- E.G via Gnus and Org.

Message #29 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Mon, 15 Sep 2025 15:19:51 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Cc: 79294 <at> debbugs.gnu.org
> Date: Sun, 14 Sep 2025 19:50:02 -0600
> 
> >> +  (let* ((o (make-overlay
> >> +             ;; FIXME: The mouse-1 event doesn't work well for fringes,
> >> +             ;; a workaround for this is set the overlay at the
> >> +             ;; beginning of the line only for fringe indicators
> >> +             (if (eq hs-indicator-type 'fringe) (pos-bol) beg)
> >> +             (1+ beg)))
> >
> > What doesn't work well in mouse-1 clicks on the fringes?
> 
> Apparently the fringe keymap does not work if the overlay is not created
> from BOL:
> 
> e.g.
> 
> The mouse-1 clicks works for this since it is created at BOL:
> 
> #+begin_src elisp
> |+| (defvar-local var nil
> | |  "Docstring.")
> #+end_src
> 
> But for code blocks that are not at BOL this will not work:
> 
> #+begin_src elisp
> |+| (defvar-local var ; <- This works
> |+|   (when foo ; <- This does not
> | |     (bar))
> | |  "Docstring.")
> #+end_src

Maybe it's a bug?  Can you show a simple Lisp program that I could
evaluate in *scratch* and use to look into this issue?

Message #32 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Mon, 15 Sep 2025 14:21:35 -0600

[Message part 1 (text/plain, inline)]

>> > What doesn't work well in mouse-1 clicks on the fringes?
>> 
>> Apparently the fringe keymap does not work if the overlay is not created
>> from BOL:
>> 
>> e.g.
>> 
>> The mouse-1 clicks works for this since it is created at BOL:
>> 
>> #+begin_src elisp
>> |+| (defvar-local var nil
>> | |  "Docstring.")
>> #+end_src
>> 
>> But for code blocks that are not at BOL this will not work:
>> 
>> #+begin_src elisp
>> |+| (defvar-local var ; <- This works
>> |+|   (when foo ; <- This does not
>> | |     (bar))
>> | |  "Docstring.")
>> #+end_src
>
> Maybe it's a bug?  Can you show a simple Lisp program that I could
> evaluate in *scratch* and use to look into this issue?

Sure, here is a recipe:

1. M-x fundamental-mode

2. M-:
   (insert
    "  "
    (propertize
     "x"
     'display '(left-fringe
                left-arrow
                error)
     'keymap (let ((map (make-sparse-keymap)))
               (define-key map (kbd "<left-fringe> <mouse-1>")
                           (lambda ()
                             (interactive)
                             (print "message!")))
               map)))

3. Click on the fringe icon, it should not work an throw the
"<left-fringe> <mouse-1> is undefined" message.

4. M-: ; (Same code but without the spaces at BOL)
   (insert
    (propertize
     "x"
     'display '(left-fringe
                left-arrow
                error)
     'keymap (let ((map (make-sparse-keymap)))
               (define-key map (kbd "<left-fringe> <mouse-1>")
                           (lambda ()
                             (interactive)
                             (print "message!")))
               map)))

5. Click on the fringe icon, it should display the message "message!".

I'm not sure if this is a known bug, but I've seen packages that locally
(in the buffer local map) bind mouse-1 to a command that scans for the
overlay in some region of the buffer, probably the fringe keymap doesn't
support this yet.

For the patch, I have already solved this problem, although I am not
happy with the solution, it is possibly the only way to solve this:

[0001-Add-hideable-indicators-for-hideshow.-Bug-79294.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
- E.G via Gnus and Org.

Message #35 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Tue, 16 Sep 2025 14:30:16 +0300

> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
> Cc: 79294 <at> debbugs.gnu.org
> Date: Mon, 15 Sep 2025 14:21:35 -0600
> 
> > Maybe it's a bug?  Can you show a simple Lisp program that I could
> > evaluate in *scratch* and use to look into this issue?
> 
> Sure, here is a recipe:
> 
> 1. M-x fundamental-mode
> 
> 2. M-:
>    (insert
>     "  "
>     (propertize
>      "x"
>      'display '(left-fringe
>                 left-arrow
>                 error)
>      'keymap (let ((map (make-sparse-keymap)))
>                (define-key map (kbd "<left-fringe> <mouse-1>")
>                            (lambda ()
>                              (interactive)
>                              (print "message!")))
>                map)))
> 
> 3. Click on the fringe icon, it should not work an throw the
> "<left-fringe> <mouse-1> is undefined" message.

I don't understand: you set the 'keymap' property on a character 'x',
and expect a click on the fringe to somehow magically use that keymap?
That's not how this works: the 'keymap' property defines the keymap to
use when clicking on the character with the property.

> 4. M-: ; (Same code but without the spaces at BOL)
>    (insert
>     (propertize
>      "x"
>      'display '(left-fringe
>                 left-arrow
>                 error)
>      'keymap (let ((map (make-sparse-keymap)))
>                (define-key map (kbd "<left-fringe> <mouse-1>")
>                            (lambda ()
>                              (interactive)
>                              (print "message!")))
>                map)))
> 
> 5. Click on the fringe icon, it should display the message "message!".

This is just a side effect of the implementation, arguably a bug.

> I'm not sure if this is a known bug, but I've seen packages that locally
> (in the buffer local map) bind mouse-1 to a command that scans for the
> overlay in some region of the buffer, probably the fringe keymap doesn't
> support this yet.

That's exactly what you should do.  See, for example,
gdb-mouse-set-clear-breakpoint in gdb-mi.el.

> For the patch, I have already solved this problem, although I am not
> happy with the solution, it is possibly the only way to solve this:

That is the right solution.

Message #38 received at 79294 <at> debbugs.gnu.org (full text, mbox):

From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79294 <at> debbugs.gnu.org
Subject: Re: bug#79294: [PATCH] Add hideable indicators for hideshow.
Date: Tue, 16 Sep 2025 13:21:17 -0600

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Elijah Gabe Pérez <eg642616 <at> gmail.com>
>> Cc: 79294 <at> debbugs.gnu.org
>> Date: Mon, 15 Sep 2025 14:21:35 -0600
>> 
>> > Maybe it's a bug?  Can you show a simple Lisp program that I could
>> > evaluate in *scratch* and use to look into this issue?
>> 
>> Sure, here is a recipe:
>> 
>> 1. M-x fundamental-mode
>> 
>> 2. M-:
>>    (insert
>>     "  "
>>     (propertize
>>      "x"
>>      'display '(left-fringe
>>                 left-arrow
>>                 error)
>>      'keymap (let ((map (make-sparse-keymap)))
>>                (define-key map (kbd "<left-fringe> <mouse-1>")
>>                            (lambda ()
>>                              (interactive)
>>                              (print "message!")))
>>                map)))
>> 
>> 3. Click on the fringe icon, it should not work an throw the
>> "<left-fringe> <mouse-1> is undefined" message.
>
> I don't understand: you set the 'keymap' property on a character 'x',
> and expect a click on the fringe to somehow magically use that keymap?
> That's not how this works: the 'keymap' property defines the keymap to
> use when clicking on the character with the property.

I see, thanks for the explanation.

>> I'm not sure if this is a known bug, but I've seen packages that locally
>> (in the buffer local map) bind mouse-1 to a command that scans for the
>> overlay in some region of the buffer, probably the fringe keymap doesn't
>> support this yet.
>
> That's exactly what you should do.  See, for example,
> gdb-mouse-set-clear-breakpoint in gdb-mi.el.
>
>> For the patch, I have already solved this problem, although I am not
>> happy with the solution, it is possibly the only way to solve this:
>
> That is the right solution.

Fine, then i think the patch is now ready to merge (if there are no
objections), I'm resending it since i found a bug.

[0001-Add-hideable-indicators-for-hideshow.-Bug-79294.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
- E.G via Gnus and Org.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.