Subject: Re: bug#69555: 30.0.50; shr - Preserve indentation when shr-fill-text is nil

From: Kévin Le Gouguec <[email-masked]>
Date: Tue, 05 Mar 2024 20:22:52 +0100

Eli Zaretskii <[email-masked]> writes:

>> Cc: Rahguzar <[email-masked]>
>> From: Kévin Le Gouguec <[email-masked]>
>> Date: Mon, 04 Mar 2024 23:08:20 +0100
>> 
>> Kévin Le Gouguec <[email-masked]> writes:
>> 
>> >                                 Will post an updated patch once I have a
>> > bug number to replace the "bug#TODO" placeholders.
>> 
>> See attached.
>
> The change in behavior is unconditional, AFAIU.  Should we have a
> separate knob for that?  Perhaps someone out there doesn't want text
> to be indented when shr-fill-text is nil?

Perhaps; I could look into adding that knob.  FWIW nothing in the
original feature request (bug#66676) gives me the impression that this
new option should affect indentation; the stated motivation was:

  > 1) Using `visual-line-mode` for line wrapping. I think this is more
  > natural for html and makes resizing windows work more nicely.

That's why I interpret the indentation change as a side-effect; curious
to hear Rahguzar's thoughts though.

Another reason I'm not overly fond of adding another knob is figuring
out what should happen if a user keeps shr-fill-text set to t, but sets
that hypothetical indentation option to nil.  Should we support that?
Or should shr-fill-text become a tri-state?  (nil, 'indent-only, t)

I'm open to biting the bullet, but I'd also like to make sure we don't
sophisticate shr.el further than we want to.

> Also, please time the code on some substantially large body of text,
> with and without shr-fill-text, and compare that with the current
> version.  I think performance is an important aspect of any change in
> this area.

Can do; would "(elisp) Profiling" be the starting point?  (For all my
list lurking, I confess my eyes have always glossed over discussions
involving benchmarks)

(Also wondering if we have any "standard" or preferred HTML documents or
websites to throw at shr.el for benchmarking purposes; if not, I guess
I'll peruse <https://en.wikipedia.org/wiki/Special:LongPages> 🤔)

Thanks for the review!