GNU bug report logs - #20774
auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

Previous Next

Full log

View this message in rfc822 format

From: Samuel Freilich <sfreilich <at> google.com>
To: npostavs <at> users.sourceforge.net
Cc: 20774 <at> debbugs.gnu.org
Subject: bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode
Date: Thu, 24 Aug 2017 11:11:12 -0400

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

You're right about string-width, since I'm counting number of columns.
Fixed that. (Which required me to deal with the fact that fill-prefix can
be nil.)

Your understanding is correct. My fix avoids do-auto-fill breaking a line
only to create a subsequent line that's as long or longer.

The previous version avoided that in most cases by refusing to break a line
during (or immediately after) the fill-prefix. But that failed to avoid
that problem when:
* It's breaking the first line of a paragraph
* The prefix of that first line doesn't match the fill-prefix for the rest
of the paragraph

I've attached the fixed version of the patch with a hopefully-improved
commit message.



On Wed, Aug 23, 2017 at 10:16 PM, <npostavs <at> users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich <at> google.com> writes:
>
> > tab-width = 4 seems consistent with how the rest of the file is
> formatted.
> > I didn't want to change the spacing in lines not touched by my diff.
>
> Hmm, somehow the code that landed in my buffer looked wrongly indented.
> Something got messed up somewhere (possibly by the email system,
> possibly during patch application, I've been experimenting with various
> forms of automation for that), but since the indentation in your next
> patch looks fine there's not really much point in worrying about it.
>
> > I added tests and a change description, and formatted the patch according
> > to the current instructions.
>
> Thanks!
>
> > I made the patch a bit more minimal. I pruned the part that dealt with
> > adaptive-fill-first-line-regexp didn't work as well as expected, since
> > that didn't work as well as expected (it didn't deal with all the
> > complexity possible with adaptive-fill-function). The updated version
> > at least handles cases where the fill-prefix isn't shorter than the
> > first-line prefix. That allowed me to simplify the code quite a bit,
> > since that makes the previous logic for skipping the exact fill-prefix
> > redundant, fill-move-to-break-point already handles the logic of
> > trying to skip at least one word after the start position passed to
> > it. Please take another look.
>
> Your commit message is a bit too focused on the problems of what was
> there previously rather than how your new code is correct.  This
> description in your email is better, but I'm still struggling a bit (I'm
> realizing I was probably a bit too superficial previously, there are a
> lot of interacting options which makes things tricky).
>
> If I understand correctly, the previous code would take the fill-prefix
> as non-breakable if the current line started with the fill-prefix.  Your
> new code takes the first N characters of the line as non-breakable
> (where N is the length of fill-prefix, should this be based on
> string-width instead?).  Is that correct (for both adaptive-fill-mode
> and otherwise)?  If so, please add an explanation of why it's correct to
> the commit message.  If not, I hope we can elaborate the commit message
> until even I can understand what's happening ;)
>
> (Minor commit message format nitpick: the part explaining the changes to
> the function should start with "* lisp/simple.el (do-auto-fill):".)
>

[Message part 2 (text/html, inline)]

[0002-Do-not-split-line-before-width-of-fill-prefix.patch (text/x-patch, attachment)]

Previous Next

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