bugGNU nano - Bugs: bug #54042, running an external spell check...

 
 

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

bug #54042: running an external spell check should be undoable

Submitter:  Benno Schulenberg <bens>
Submitted:  Sun 03 Jun 2018 09:32:56 AM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  dolorous Open/Closed:  Closed

Jump to the original submission

Mon 10 Sep 2018 05:45:18 PM UTC, comment #23: 

Released in 3.0.  Thanks again.

Benno Schulenberg <bens>
Group administrator
Wed 08 Aug 2018 06:09:51 PM UTC, comment #22: 

No problem.

David Lawrence Ramsey <dolorous>
Group Member
Wed 08 Aug 2018 06:00:22 PM UTC, comment #21: 

Okay, I think it is now good enough for wider testing.  The whole lot has been pushed to master, commits b5f15cb5 to 94b4f072.

Thanks for working on this!

Benno Schulenberg <bens>
Group administrator
Wed 08 Aug 2018 05:53:44 PM UTC, comment #20: 

Argh.  Forgot about the statting; good catch.

I've tested your patch with: (1) a file with no flagged spelling errors, (2) a a file with flagged spelling errors where I directed it to ignore all errors, and (3) a file with flagged spelling errors where I aborted the spell checker without making any changes, and in all cases, the file is not marked as modified, as expected.

David Lawrence Ramsey <dolorous>
Group Member
Wed 08 Aug 2018 05:43:17 PM UTC, comment #19: 

Well, the code for statting the temp file is already present: around line 2912 (after applying the patch set).  But for the current setup, the check comes too late: the file has already been marked as modified by the new "cutting and pasting".  What should be done is: move the stat further up, and not do any replacing and other fiddling at all when the temp file was not changed.

The rough patch looks like the attached.

(file #44739)

Benno Schulenberg <bens>
Group administrator
Wed 08 Aug 2018 12:14:53 AM UTC, comment #18: 

Of those two, not having any undo item would be much trickier to accomplish, since it would involve manually altering the undo stack (as in other hypothetical cases of undone unchanged text that you've brought up), and I'm still trying to figure out some parts of the undo code.  And even then, you run into the same problem that you get when e.g. trying to undo cutting the magicline: even if no text has changed, the cursor position where you tried to do something to the text needs to be saved, which requires some kind of undo item, at least the way things are currently designed.

Not marking the file as modified could be accomplished by checking the return value of the spell checker, assuming of course that its return value indicates whether the file has changed (which would depend on which spell checker was used; aspell's documentation does not seem to document its return values at all).  Failing that, nano would have to compare the original and changed text itself, which would impact performance significantly if the file was big enough, and I'm not certain whether you'd decide that doing so required too much code.

In short... these are not easy problems to fix.  Restoring cursor position properly when undoing spell-check would probably be easier, though.

David Lawrence Ramsey <dolorous>
Group Member
Tue 07 Aug 2018 07:21:47 PM UTC, comment #17: 

The one thing that doesn't work right is when a spell check finishes immediately because the external spell checker does not find any misspellings.  In that case there should not be any undo item, and the file should not be marked as modified.

Benno Schulenberg <bens>
Group administrator
Tue 07 Aug 2018 06:58:30 PM UTC, comment #16: 

Good catch on COUPLE_END; thanks.  After some quick testing, your reordered patch set works as far as I can tell, and the squashing/commit message tweaks in it are fine with me.

David Lawrence Ramsey <dolorous>
Group Member
Tue 07 Aug 2018 06:46:38 PM UTC, comment #15: 

Here is a reordered set, with the first two patches squashed together and tweaked commit messages.  Please verify.

(file #44737)

Benno Schulenberg <bens>
Group administrator
Tue 07 Aug 2018 06:22:36 PM UTC, comment #14: 

Thanks.  (The COUPLE_END for execute_command() needs the same setting of the "filtering" string, of course.)

Will push the whole lot tomorrow.

Benno Schulenberg <bens>
Group administrator
Tue 07 Aug 2018 03:14:54 PM UTC, comment #13: 

No problem.  Back now, a little sooner than I thought.

I like the second version better, the one from comment #10.  It's a more flexible design, and its methods could be more easily extended for undoing justify later.  The first version's approach with a flag would probably be better used to fix the cursor position's not being saved after undoing, since that would apply both to spell-checking a whole file and eventually justifying a single paragraph.

Although the patch has one small problem: with it applied, alternate spell check is described properly, but filtering via ^R^X and M-\ is no longer described as "filtering".  I've attached a version of it with this fixed (a one-line change).

(file #44735)

David Lawrence Ramsey <dolorous>
Group Member
Mon 06 Aug 2018 05:18:15 PM UTC, comment #12: 

No worries.  There is no hurry.

Benno Schulenberg <bens>
Group administrator
Mon 06 Aug 2018 12:48:04 AM UTC, comment #11: 

Unfortunately, I've been busier than I thought, so it'll be a few more days until I can get back to my development machine.  Thanks for the undo/redo messages code, though; I'll look at both attempts as soon as I can.

David Lawrence Ramsey <dolorous>
Group Member
Sat 04 Aug 2018 08:06:20 AM UTC, comment #10: 

And here is the way that I suggested at first.  What do you like best?

(file #44703)

Benno Schulenberg <bens>
Group administrator
Sat 04 Aug 2018 07:57:04 AM UTC, comment #9: 

Here is what I fumbled together to get proper undo/redo messages for an external spellcheck.

(A later improvement could be to pass the xflag to add_undo(), but that would add a zero parameter to all the other calls too.)

(file #44702)

Benno Schulenberg <bens>
Group administrator
Thu 02 Aug 2018 01:27:00 AM UTC, comment #8: 

Good to know.  I'm not certain how to set the undo/redo messages, but I'll look into it.  It'll be a few days before I can get to it, though.

David Lawrence Ramsey <dolorous>
Group Member
Wed 01 Aug 2018 06:20:40 PM UTC, comment #7: 

Thanks for the patch set.  It works, as far as I can tell after a quick test.  Just one thing: it should not say that "Filtering" has been undone, but instead that "Spelling corrections" have been undone.  (Maybe it is possible to store the undo/redo message in the COUPLE_BEGIN and COUPLE_END items?)

Benno Schulenberg <bens>
Group administrator
Tue 31 Jul 2018 09:10:06 PM UTC, comment #6: 

Okay.  I fixed the build when using --enable-tiny --enable-speller, and resynced against git ac102a7; the new version is attached.  Or is it major enough that I should send it to the mailing list?

I suppose once this (or something like it) gets in, not preserving the cursor position on an undo will have to be reported as a separate bug.  Also, the formatter uses replace_buffer() as well, so it's affected by these patches, but I haven't touched the formatter code at all, so I'm not sure how it's affected.  Although your proposed patch to remove the formatter would also fix any potential problems with it :)

(file #44668)

David Lawrence Ramsey <dolorous>
Group Member
Tue 31 Jul 2018 05:54:37 PM UTC, comment #5: 

Putting back the cursor is not essential for making an external spell check (or a justification) undoable.  Of course, not restoring the cursor position is a bug, but something not being undoable is a bigger bug -- and discarding all the previous undo items is a major bug.  So, don't worry about the cursor position for now.

Benno Schulenberg <bens>
Group administrator
Tue 31 Jul 2018 02:25:07 AM UTC, comment #4: 

Thoughts on comment #2: The cut-to-eof used when adding an undo for alt spell-check has to change the cursor position from where the user pressed Ctrl-T to the beginning of the file, so that the entire file is properly cut, and the initial cursor position ends up lost.  If I understand the code correctly, saving the initial cursor position somewhere in the undo (possibly as part of COUPLE_BEGIN?) would fix this problem.

I'm not certain how to do it (I'm still figuring out COUPLE_BEGIN and COUPLE_END, even if I've made them work here), but justify will need it too when it's made undoable, since justify's search for a paragraph has to change the cursor position from where the user pressed Ctrl-J to where the paragraph begins, so that the paragraph can be properly justified, and the initial cursor position will end up lost.

David Lawrence Ramsey <dolorous>
Group Member
Mon 30 Jul 2018 10:23:51 PM UTC, comment #3: 

Another caveat: it probably needs NANO_TINY #ifdefs around the added undo code for when a user tries --enable-tiny --enable-speller, but I don't have any more time to check that at the moment.

David Lawrence Ramsey <dolorous>
Group Member
Mon 30 Jul 2018 10:17:36 PM UTC, comment #2: 

One caveat: with these applied, when nano undoes an alt spell-check, it doesn't put the cursor back exactly where it was when the user hit Ctrl-T.  This should be fixed, but I'm not sure how yet.

David Lawrence Ramsey <dolorous>
Group Member
Mon 30 Jul 2018 10:08:24 PM UTC, comment #1: 

Attached an attempt at this, made against git c231b77.  It's modeled after undo for external command filtering, in that it uses the cutbuffer to discard text before replacing it.  The marked spell check does a marked cut, and the unmarked spell check does a cut-to-eof from the top of the file.


(file #44663)

David Lawrence Ramsey <dolorous>
Group Member
Sun 03 Jun 2018 09:32:56 AM UTC, original submission:  

Currently, after an external spell check, the undo stack is discarded and nor the spelling corrections nor any earlier edits are undoable any more.

Expected behavior: just like after filtering the buffer (or marked region) through an external command, an external spelling correction ought to be undoable with a single M-U.

Benno Schulenberg <bens>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-09-10 bens Open/ClosedOpen Closed
    2018-08-08 bens StatusIn Progress Fixed
    2018-08-08 bens Attached File- Added statbefore.patch, #44739
    2018-08-07 bens Attached File- Added newset.zip, #44737
    2018-08-07 dolorous Attached File- Added 0007-undo-distinguish-between-general-filtering-and-spell.patch, #44735
    2018-08-04 bens Attached File- Added 0007-undo-distinguish-between-general-filtering-and-spe.patch, #44703
    2018-08-04 bens Attached File- Added 0007-undo-differentiate-between-general-filtering-and-spe.patch, #44702
    2018-08-01 bens StatusNone In Progress
        Assigned toNone dolorous
    2018-07-31 dolorous Attached File- Added nano-speller.zip, #44668
    2018-07-30 dolorous Attached File- Added nano-speller.zip, #44663

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code