* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode @ 2015-06-08 23:39 Samuel Freilich 2015-06-26 23:40 ` bug#20774: Typo in Patch Samuel Freilich ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Samuel Freilich @ 2015-06-08 23:39 UTC (permalink / raw To: 20774 [-- Attachment #1.1: Type: text/plain, Size: 1711 bytes --] 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 [-- Attachment #1.2: Type: text/html, Size: 3051 bytes --] [-- Attachment #2: emacs-auto-fill-different-first-line.patch --] [-- Type: text/x-patch, Size: 2610 bytes --] *** simple.el.old 2015-06-08 19:19:20.484397387 -0400 --- simple.el.new 2015-06-08 19:18:58.340305301 -0400 *************** *** 6559,6575 **** (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)))) ;; See whether the place we found is any good. --- 6559,6581 ---- (setq fill-prefix prefix)))) (while (and (not give-up) (> (current-column) fc)) ! ;; Determine where to split the line. ! (let* (line-start ! after-prefix (fill-point (save-excursion (beginning-of-line) ! (setq line-start (point)) ! ;; Skip the fill-prefix. Note that we might be on the first line of the ! ;; paragraph, and the fist line can differ in adaptive-fill-mode. ! (when (or (and adaptive-fill-mode ! adaptive-fill-first-line-regexp ! (looking-at adaptive-fill-first-line-regexp)) ! (and fill-prefix ! (looking-at (regex-quote fill-prefix)))) ! (setq after-prefix (match-end 0))) (move-to-column (1+ fc)) ! (fill-move-to-break-point (or after-prefix line-start)) (point)))) ;; See whether the place we found is any good. *************** *** 6578,6586 **** (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 --- 6584,6592 ---- (or (bolp) ;; There is no use breaking at end of line. (save-excursion (skip-chars-forward " ") (eolp)) ! ;; It is futile to split earlier in the line than the length of the ! ;; prefix, since we would just insert the prefix again. ! (and fill-prefix (<= (point) (+ line-start (length fill-prefix)))) ;; Don't split right after a comment starter ;; since we would just make another comment starter. (and comment-start-skip ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: Typo in Patch 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 ` 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-29 3:37 ` npostavs 2 siblings, 0 replies; 12+ messages in thread From: Samuel Freilich @ 2015-06-26 23:40 UTC (permalink / raw To: 20774 [-- Attachment #1.1: Type: text/plain, Size: 113 bytes --] There was a typo in that previous patch. Should have been regexp-quote, not regex-quote. Fixed version attached. [-- Attachment #1.2: Type: text/html, Size: 134 bytes --] [-- Attachment #2: emacs-auto-fill-different-first-line-fixed.patch --] [-- Type: text/x-patch, Size: 2611 bytes --] *** simple.el.old 2015-06-08 19:19:20.484397387 -0400 --- simple.el.new 2015-06-08 19:18:58.340305301 -0400 *************** *** 6559,6575 **** (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)))) ;; See whether the place we found is any good. --- 6559,6581 ---- (setq fill-prefix prefix)))) (while (and (not give-up) (> (current-column) fc)) ! ;; Determine where to split the line. ! (let* (line-start ! after-prefix (fill-point (save-excursion (beginning-of-line) ! (setq line-start (point)) ! ;; Skip the fill-prefix. Note that we might be on the first line of the ! ;; paragraph, and the fist line can differ in adaptive-fill-mode. ! (when (or (and adaptive-fill-mode ! adaptive-fill-first-line-regexp ! (looking-at adaptive-fill-first-line-regexp)) ! (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 (or after-prefix line-start)) (point)))) ;; See whether the place we found is any good. *************** *** 6578,6586 **** (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 --- 6584,6592 ---- (or (bolp) ;; There is no use breaking at end of line. (save-excursion (skip-chars-forward " ") (eolp)) ! ;; It is futile to split earlier in the line than the length of the ! ;; prefix, since we would just insert the prefix again. ! (and fill-prefix (<= (point) (+ line-start (length fill-prefix)))) ;; Don't split right after a comment starter ;; since we would just make another comment starter. (and comment-start-skip ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 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 ` npostavs 2017-08-22 16:00 ` Samuel Freilich 2017-08-29 3:37 ` npostavs 2 siblings, 1 reply; 12+ messages in thread From: npostavs @ 2017-08-22 12:49 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 Samuel Freilich <sfreilich@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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 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 0 siblings, 1 reply; 12+ messages in thread From: Samuel Freilich @ 2017-08-22 16:00 UTC (permalink / raw To: npostavs; +Cc: 20774 [-- Attachment #1: Type: text/plain, Size: 739 bytes --] 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@users.sourceforge.net> wrote: > Samuel Freilich <sfreilich@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? > [-- Attachment #2: Type: text/html, Size: 1233 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-22 16:00 ` Samuel Freilich @ 2017-08-23 3:56 ` npostavs 2017-08-23 18:20 ` Samuel Freilich 0 siblings, 1 reply; 12+ messages in thread From: npostavs @ 2017-08-23 3:56 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 Samuel Freilich <sfreilich@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? ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-23 3:56 ` npostavs @ 2017-08-23 18:20 ` Samuel Freilich 2017-08-24 2:16 ` npostavs 0 siblings, 1 reply; 12+ messages in thread From: Samuel Freilich @ 2017-08-23 18:20 UTC (permalink / raw To: npostavs; +Cc: 20774 [-- Attachment #1.1: Type: text/plain, Size: 1783 bytes --] 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@users.sourceforge.net> wrote: > Samuel Freilich <sfreilich@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? > [-- Attachment #1.2: Type: text/html, Size: 2616 bytes --] [-- Attachment #2: 0001-Do-not-split-line-before-length-of-fill-prefix.patch --] [-- Type: text/x-patch, Size: 3279 bytes --] From b098b41ca457760a6ddde633cac37e70fbc8aeb9 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 length of fill-prefix Previous logic already avoided splitting the line immediately after the exact fill prefix, but in adaptive-fill-mode, the fill-prefix may not be the same as the prefix of the first line of a paragraph. That resulted in auto-fill-mode breaking lines in such a way as to make the subsequent line as long or longer (Bug#20774). --- lisp/simple.el | 26 +++++++++++--------------- test/lisp/simple-tests.el | 11 +++++++++++ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/lisp/simple.el b/lisp/simple.el index 072723cd64..a1ba46f586 100644 --- a/lisp/simple.el +++ b/lisp/simple.el @@ -7148,18 +7148,17 @@ 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 (min (line-end-position) + (+ line-start (length fill-prefix)))) + (point))))) ;; See whether the place we found is any good. (if (save-excursion @@ -7167,9 +7166,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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-23 18:20 ` Samuel Freilich @ 2017-08-24 2:16 ` npostavs 2017-08-24 15:11 ` Samuel Freilich 0 siblings, 1 reply; 12+ messages in thread From: npostavs @ 2017-08-24 2:16 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 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):".) ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-24 2:16 ` npostavs @ 2017-08-24 15:11 ` Samuel Freilich 2017-08-25 1:45 ` npostavs 0 siblings, 1 reply; 12+ messages in thread From: Samuel Freilich @ 2017-08-24 15:11 UTC (permalink / raw To: npostavs; +Cc: 20774 [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-24 15:11 ` Samuel Freilich @ 2017-08-25 1:45 ` npostavs 0 siblings, 0 replies; 12+ messages in thread From: npostavs @ 2017-08-25 1:45 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 Samuel Freilich <sfreilich@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.) ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 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-29 3:37 ` npostavs 2017-08-29 3:55 ` npostavs 2 siblings, 1 reply; 12+ messages in thread From: npostavs @ 2017-08-29 3:37 UTC (permalink / raw To: Samuel Freilich, 20774 [-- Attachment #1: Type: text/plain, Size: 26 bytes --] [forwarding to bug list] [-- Attachment #2: Type: message/rfc822, Size: 5302 bytes --] [-- Attachment #2.1.1.1: Type: text/plain, Size: 517 bytes --] <npostavs@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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2.1.1.2: 0003-Do-not-split-line-before-width-of-fill-prefix.patch --] [-- Type: text/x-patch; name=0003-Do-not-split-line-before-width-of-fill-prefix.patch, Size: 3440 bytes --] From 118c8a43510a7d3020a552ce0e87e5a1b9ccec54 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 | 27 ++++++++++++--------------- test/lisp/simple-tests.el | 11 +++++++++++ 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/lisp/simple.el b/lisp/simple.el index 13cfa3487d..27990bb661 100644 --- a/lisp/simple.el +++ b/lisp/simple.el @@ -7151,18 +7151,18 @@ 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) + ;; Don't split earlier in the line than the length of the + ;; fill prefix, since the resulting line would be longer. + (when fill-prefix + (move-to-column (string-width fill-prefix))) + (let ((after-prefix (point))) + (move-to-column (1+ fc)) + (fill-move-to-break-point after-prefix) + (point))))) ;; See whether the place we found is any good. (if (save-excursion @@ -7170,9 +7170,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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-29 3:37 ` npostavs @ 2017-08-29 3:55 ` npostavs 2017-08-31 0:51 ` npostavs 0 siblings, 1 reply; 12+ messages in thread From: npostavs @ 2017-08-29 3:55 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 > From: Samuel Freilich <sfreilich@google.com> > > <npostavs@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode 2017-08-29 3:55 ` npostavs @ 2017-08-31 0:51 ` npostavs 0 siblings, 0 replies; 12+ messages in thread From: npostavs @ 2017-08-31 0:51 UTC (permalink / raw To: Samuel Freilich; +Cc: 20774 tags 20774 fixed close 20774 26.1 quit npostavs@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-08-31 0:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 2017-08-25 1:45 ` npostavs 2017-08-29 3:37 ` npostavs 2017-08-29 3:55 ` npostavs 2017-08-31 0:51 ` npostavs
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.