Package: emacs;
Reported by: Elijah Gabe Pérez <eg642616 <at> gmail.com>
Date: Sat, 23 Aug 2025 04:57:02 UTC
Severity: normal
Tags: patch
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?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.