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

Previous Next

Full log

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.

Previous Next

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