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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20774 in the body.
You can then email your comments to 20774 AT debbugs.gnu.org in the normal way.

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

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

From: Samuel Freilich <sfreilich <at> google.com>
To: bug-gnu-emacs <at> gnu.org
Subject: auto-fill doesn't work properly when first-line prefix differs in
 adaptive-fill-mode
Date: Mon, 8 Jun 2015 19:39:03 -0400

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

The function do-auto-fill doesn't consider that in adaptive-fill-mode, the
first line's prefix might not match the fill-prefix for subsequent lines.

For example, markdown-mode configures adaptive-fill-mode so that when
filling list item paragraphs, there's a hanging indent:

*   List item...
    next line...

Currently, there's a bug where auto-fill-mode will break lines at the start
of a list item, even though the resulting line is just as long, e.g.:

*   [link](http://a-really-long-url.example.com/..
<http://a-really-long-url.example.com/>.)

When you hit enter at the end of the line in markdown-mode with autofill on
and the fill-column set to less than the length of that line, it becomes:

*
   [link](http://a-really-long-url.example.com/..
<http://a-really-long-url.example.com/>.)

That's incorrect behavior because the prefix added after breaking the line
is just as long as the line up to the point before the break.

The attached patch fixes this by ensuring that a line will be broken by
auto-fill only if the position is further along in the line than the length
of the fill prefix. It also ensures that the first-line fill prefix is
skipped when finding a point to break (that is, the line should not be
broken in the middle of the prefix), even if the first-line prefix differs
from the fill-prefix for subsequent lines.


*Detailed Reproduction Steps:*

Get markdown-mode from:
http://jblevins.org/projects/markdown-mode/markdown-mode.el

$ emacs -Q -l markdown-mode.el
M-x markdown-mode RET
M-x auto-fill-mode RET
M-x set-fill-column RET 5 RET

Type the following into the buffer and hit RET:
* Item

Actual result:
*
  Item
  [cursor]

Expected result:
* Item
  [cursor]

Peace,
-Sam

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

[emacs-auto-fill-different-first-line.patch (text/x-patch, attachment)]

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

From: Samuel Freilich <sfreilich <at> google.com>
To: 20774 <at> debbugs.gnu.org
Subject: Typo in Patch
Date: Fri, 26 Jun 2015 19:40:09 -0400

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

There was a typo in that previous patch. Should have been regexp-quote, not
regex-quote. Fixed version attached.

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

[emacs-auto-fill-different-first-line-fixed.patch (text/x-patch, attachment)]

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

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

Samuel Freilich <sfreilich <at> google.com> writes:

> $ emacs -Q -l markdown-mode.el
> M-x markdown-mode RET
> M-x auto-fill-mode RET
> M-x set-fill-column RET 5 RET
>
> Type the following into the buffer and hit RET:
> * Item
>
> Actual result:
> *
>   Item
>   [cursor]
>
> Expected result:
> * Item
>   [cursor]

With the patch I get

* Item
[cursor]

i.e, the cursor lands in column 0.  Is it correct?

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

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

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

Yes, that's correct. Sorry for getting that wrong in my initial description
of the bug. That's the same as the indentation behavior with auto-fill-mode
disabled. A hanging indent is not assumed with only one line.

On Tue, Aug 22, 2017 at 8:49 AM, <npostavs <at> users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich <at> google.com> writes:
>
> > $ emacs -Q -l markdown-mode.el
> > M-x markdown-mode RET
> > M-x auto-fill-mode RET
> > M-x set-fill-column RET 5 RET
> >
> > Type the following into the buffer and hit RET:
> > * Item
> >
> > Actual result:
> > *
> >   Item
> >   [cursor]
> >
> > Expected result:
> > * Item
> >   [cursor]
>
> With the patch I get
>
> * Item
> [cursor]
>
> i.e, the cursor lands in column 0.  Is it correct?
>

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

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

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

Samuel Freilich <sfreilich <at> google.com> writes:

> Yes, that's correct. Sorry for getting that wrong in my initial
> description of the bug. That's the same as the indentation behavior
> with auto-fill-mode disabled. A hanging indent is not assumed with
> only one line.

Ah okay.  The patch looks fine (apart from the indentation being off, I
assume because you had tab-width = 4).  Would you mind adding a commit
message?  (See CONTRIBUTE for the format.)

If you're feeling up to it, a test would be nice too.

Since you're sending from a google.com address, I understand the
copyright assignment is covered under Google's blanket agreement, is
that right?

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

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

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

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.

I added tests and a change description, and formatted the patch according
to the current instructions. 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.

You're correct about copyright. I'm doing this for work, copyright is
covered by whatever agreements Google has made.



On Tue, Aug 22, 2017 at 11:56 PM, <npostavs <at> users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich <at> google.com> writes:
>
> > Yes, that's correct. Sorry for getting that wrong in my initial
> > description of the bug. That's the same as the indentation behavior
> > with auto-fill-mode disabled. A hanging indent is not assumed with
> > only one line.
>
> Ah okay.  The patch looks fine (apart from the indentation being off, I
> assume because you had tab-width = 4).  Would you mind adding a commit
> message?  (See CONTRIBUTE for the format.)
>
> If you're feeling up to it, a test would be nice too.
>
> Since you're sending from a google.com address, I understand the
> copyright assignment is covered under Google's blanket agreement, is
> that right?
>

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

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

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

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

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 #28 received at 20774 <at> debbugs.gnu.org (full text, mbox):

From: Samuel Freilich <sfreilich <at> google.com>
To: npostavs <at> users.sourceforge.net
Cc: 20774 <at> debbugs.gnu.org
Subject: Re: 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)]

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

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

Samuel Freilich <sfreilich <at> google.com> writes:

> 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.)

Actually, it's now occuring to me that adding a position to a
string-width isn't quite correct either, since it doesn't take into
account wide characters on the current line.  I guess it should actually
be something like

    (save-excursion
      (if line-prefix
          (move-to-column (string-width line-prefix))
        (beginning-of-line))
      (point))

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

That is much better, thanks.  (One more minor formatting nitpick, there
should be 2 spaces after the period.)

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

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

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

[forwarding to bug list]

[Message part 2 (message/rfc822, inline)]

From: Samuel Freilich <sfreilich <at> google.com>
To: npostavs <at> users.sourceforge.net
Subject: Re: bug#20774: auto-fill doesn't work properly when first-line prefix
 differs in adaptive-fill-mode
Date: Mon, 28 Aug 2017 22:06:23 -0400

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

<npostavs <at> users.sourceforge.net> wrote:

> Actually, it's now occuring to me that adding a position to a
> string-width isn't quite correct either

Ah right, (point) is in characters, which doesn't correspond directly to
columns at all. I think I can avoid the extra save-excursion and make it a
little cleaner as a result.

> there should be 2 spaces after the period

Heresy! :-P

But done. That does seem to be the most common style in the ChangeLog.

Please take another look. Hopefully this patch can be merged.

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

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

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

> From: Samuel Freilich <sfreilich <at> google.com>
>
> <npostavs <at> users.sourceforge.net> wrote:
>
>> Actually, it's now occuring to me that adding a position to a
>> string-width isn't quite correct either
>
> Ah right, (point) is in characters, which doesn't correspond directly to
> columns at all. I think I can avoid the extra save-excursion and make it a
> little cleaner as a result.

Looks good.  I'll wait a couple of days before pushing in case someone
finds we missed something.

>> there should be 2 spaces after the period
>
> Heresy! :-P
>
> But done. That does seem to be the most common style in the ChangeLog.

I think you'll find it's not just most common, it's official policy[1].
However, the Church of Emacs will forgive your impertinence, as long as
you don't bring up the subject of quotation marks ;) [2]

[1]: http://www.gnu.org/prep/standards/html_node/Comments.html
[2]: https://mobile.twitter.com/AMalabarba/status/641598687467163648

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

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

tags 20774 fixed
close 20774 26.1
quit

npostavs <at> users.sourceforge.net writes:

> Looks good.  I'll wait a couple of days before pushing in case someone
> finds we missed something.

I've pushed to master.

[1: cda26e6462]: 2017-08-30 20:10:36 -0400
  Do not split line before width of fill-prefix
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=cda26e64621d71c6a797f694418d844a621998be

Previous Next

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