From: Samuel Freilich <sfreilich@google.com>
To: npostavs@users.sourceforge.net
Cc: 20774@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 [thread overview]
Message-ID: <CACQEUgM-bfjUAwstyvLyAp8FN3qk6dk=w+GdPOntDpK3xHz53A@mail.gmail.com> (raw)
In-Reply-To: <87r2w1aew4.fsf@users.sourceforge.net>
[-- Attachment #1.1: Type: text/plain, Size: 3191 bytes --]
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@users.sourceforge.net> wrote:
> Samuel Freilich <sfreilich@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):".)
>
[-- Attachment #1.2: Type: text/html, Size: 3997 bytes --]
[-- Attachment #2: 0002-Do-not-split-line-before-width-of-fill-prefix.patch --]
[-- Type: text/x-patch, Size: 3529 bytes --]
From 42da1d80367b7519b0047328a37d845e78aaa157 Mon Sep 17 00:00:00 2001
From: Samuel Freilich <sfreilich@google.com>
Date: Wed, 23 Aug 2017 13:40:45 -0400
Subject: [PATCH] Do not split line before width of fill-prefix
When auto-filling a paragraph, don't split a line before the width of the
fill-prefix, creating a subsequent line that is as long or longer (Bug#20774).
* lisp/simple.el (do-auto-fill): Only consider break-points that are later in
the line than the width of the fill-prefix. This is a more general solution
than the previous logic, which only skipped over the exact fill-prefix. The
fill-prefix doesn't necessarily match the prefix of the first line of a
paragraph in adaptive-fill-mode.
---
lisp/simple.el | 29 ++++++++++++++---------------
test/lisp/simple-tests.el | 11 +++++++++++
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/lisp/simple.el b/lisp/simple.el
index 072723cd64..15c7f1f935 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7148,18 +7148,20 @@ do-auto-fill
(setq fill-prefix prefix))))
(while (and (not give-up) (> (current-column) fc))
- ;; Determine where to split the line.
- (let* (after-prefix
- (fill-point
- (save-excursion
- (beginning-of-line)
- (setq after-prefix (point))
- (and fill-prefix
- (looking-at (regexp-quote fill-prefix))
- (setq after-prefix (match-end 0)))
- (move-to-column (1+ fc))
- (fill-move-to-break-point after-prefix)
- (point))))
+ ;; Determine where to split the line.
+ (let ((fill-point
+ (save-excursion
+ (beginning-of-line)
+ (let ((line-start (point)))
+ (move-to-column (1+ fc))
+ ;; Don't split earlier in the line than the length of the
+ ;; fill prefix, since the resulting line would be longer.
+ (fill-move-to-break-point
+ (if fill-prefix
+ (min (line-end-position)
+ (+ line-start (string-width fill-prefix)))
+ line-start))
+ (point)))))
;; See whether the place we found is any good.
(if (save-excursion
@@ -7167,9 +7169,6 @@ do-auto-fill
(or (bolp)
;; There is no use breaking at end of line.
(save-excursion (skip-chars-forward " ") (eolp))
- ;; It is futile to split at the end of the prefix
- ;; since we would just insert the prefix again.
- (and after-prefix (<= (point) after-prefix))
;; Don't split right after a comment starter
;; since we would just make another comment starter.
(and comment-start-skip
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index ad7aee1db1..c7330e4034 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -497,5 +497,16 @@ simple-test-undo-with-switched-buffer
(should (equal (line-number-at-pos 5) 3))
(should (equal (line-number-at-pos 7) 4)))))
+(ert-deftest auto-fill-mode-no-break-before-length-of-fill-prefix ()
+ (with-temp-buffer
+ (setq-local fill-prefix " ")
+ (set-fill-column 5)
+ ;; Shouldn't break after 'foo' (3 characters) when the next
+ ;; line is indented >= to that, that woudln't result in shorter
+ ;; lines.
+ (insert "foo bar")
+ (do-auto-fill)
+ (should (string-equal (buffer-string) "foo bar"))))
+
(provide 'simple-test)
;;; simple-test.el ends here
--
2.14.1.342.g6490525c54-goog
next prev parent reply other threads:[~2017-08-24 15:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-08 23:39 bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode Samuel Freilich
2015-06-26 23:40 ` bug#20774: Typo in Patch Samuel Freilich
2017-08-22 12:49 ` bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode npostavs
2017-08-22 16:00 ` Samuel Freilich
2017-08-23 3:56 ` npostavs
2017-08-23 18:20 ` Samuel Freilich
2017-08-24 2:16 ` npostavs
2017-08-24 15:11 ` Samuel Freilich [this message]
2017-08-25 1:45 ` npostavs
2017-08-29 3:37 ` npostavs
2017-08-29 3:55 ` npostavs
2017-08-31 0:51 ` npostavs
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACQEUgM-bfjUAwstyvLyAp8FN3qk6dk=w+GdPOntDpK3xHz53A@mail.gmail.com' \
--to=sfreilich@google.com \
--cc=20774@debbugs.gnu.org \
--cc=npostavs@users.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.