bugGNU nano - Bugs: bug #53742, an external spell check of a...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #53742: an external spell check of a selected region inserts extra newline

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Tue 24 Apr 2018 07:23:02 PM UTC  
 
Severity:  4 - Important Status:  Fixed
Assigned to:  Benno Schulenberg <bens> Open/Closed:  Closed

( Jump to the original submission)

Sat 28 Apr 2018 09:19:57 AM UTC, comment #9: 

The fix has been released in 2.9.6.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 25 Apr 2018 03:29:44 PM UTC, comment #8: 

Thanks for checking and testing.  Pushed to master, commit faa0eb99.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 25 Apr 2018 03:17:19 PM UTC, comment #7: 

Addendum: As in, I've also tested it, and it works as far as I can tell.

David Lawrence Ramsey <dolorous>
Project Member
Wed 25 Apr 2018 03:13:58 PM UTC, comment #6: 

Looks good to me.

David Lawrence Ramsey <dolorous>
Project Member
Wed 25 Apr 2018 09:43:55 AM UTC, comment #5: 

Here is the alternative patch.  In my eyes this is neater: it doesn't need an extra variable to save and restore the flag.

(file #44024)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 25 Apr 2018 09:21:43 AM UTC, comment #4: 

Yeah, we reached the same conclusion simultaneously.  I added a pause in do_alt_speller(), before the external speller is forked, and looked at the temp file: it already had an added 0x0A -- as one would expect when nonewlines isn't set.  (And anyway, it would be strange if the speller changed the content of the temp file without changing the timestamp -- a changed timestamp would have caused the Modified to be set.)

So, I had the attached patch prepared.  But then I looked further: do_int_spell_fix() stashes the flags.  If we pull that code two levels up, we don't have to save and restore the NO_NEWLINES flag separately.  A new attachment is coming up shortly.

(file #44023)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 25 Apr 2018 08:31:49 AM UTC, comment #3: 

Forget the previous commentary with regards to spell checkers adding newlines; I was mistaken.  If, as in your example, you ignore all misspelled words, then the spell checker isn't what's adding the magicline and throwing everything off.

It turns out that nano's actually doing that; the code to write the temporary file to disk in do_alt_speller() doesn't turn off magiclines properly as is done elsewhere, and the old method of partitioning directly worked around it, so it wasn't an issue in spite of that.

The attached patch against git 62007c8 fixes the problem.

(file #44021)

David Lawrence Ramsey <dolorous>
Project Member
Wed 25 Apr 2018 02:14:42 AM UTC, comment #2: 

I've looked into it, and that commit does cause the problem.  It's a combination of an oversight on my part and an undocumented behavior of aspell (and possibly other external spell checkers).

Specifically, when the marked text gets written to a temporary file, the file has no newline at the end.  However, after it's processed by the spell checker, the spell checker "helpfully" adds a newline to it.  The code (as written now) assumes that the only changes made to the text were spelling-related, and doesn't account for this.

The old code didn't have this problem because it used partitioning directly, which meant that if the new file had a magicline in it, it could remove it via remove_magicline() before unpartitioning.  However, the new code uses partitioning indirectly, via read_file(), which means that remove_magicline() won't work at all, and thus there's no easy way to remove the added magicline.

As for how to fix it, I'm not certain of the best way.  I'm thinking of adding a parameter to read_file() that makes it deliberately not read the last line of the file, and using that parameter only when the marked text that's being replaced doesn't end in a newline.  (Or maybe you can think of another solution.)

David Lawrence Ramsey <dolorous>
Project Member
Tue 24 Apr 2018 07:36:41 PM UTC, comment #1: 

David, I'm guessing that commit 234bd9c9 is the culprit for this -- I'll do a full bisect tomorrow.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 24 Apr 2018 07:23:02 PM UTC, original submission:  

To reproduce, run:

  nano --ignore --speller="aspell -l en -x -c" +3,45 NEWS

Then type:  M-A  M-\  ^T  i  i  i

See how the third line has been broken into two (but the buffer has not been marked as Modified), and how now only the first two lines are highlighted instead of also half of the third.  Weird!

Bug exists since around 2.8.1 -- version 2.7.5 is still fine.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dolorous (Posted a comment)
  • -email is unavailable- added by bens
  • -email is unavailable- added by bens (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-28 bens Open/ClosedOpen => Closed
    2018-04-25 bens StatusIn Progress => Fixed
    2018-04-25 bens Summarydoing an external spell check of a selected region inserts a newline => an external spell check of a selected region inserts extra newline
    2018-04-25 bens Summarydoing an external spell check inserts a newline (even when nothing was changed) => doing an external spell check of a selected region inserts a newline
    2018-04-25 bens Summaryexternal spell checker inserts newline, even when nothing was changed => doing an external spell check inserts a newline (even when nothing was changed)
    2018-04-25 bens Attached File- => Added 0001-speller-don-t-add-an-extra-newline-when-saving-the-t.patch, #44024
    2018-04-25 bens Attached File- => Added set-nonewlines-when-spellchecking-a-selection.patch, #44023
        StatusNone => In Progress
    2018-04-25 dolorous Attached File- => Added 0001-text-don-t-add-magiclines-to-temp-files-sent-to-the-.patch, #44021
    2018-04-24 bens Carbon-Copy- => Added dolorous

    Back to the top


    Powered by Savane 3.7