d12frosted / flyspell-correct

Distraction-free words correction with flyspell via selected interface.
Other
203 stars 14 forks source link

Cursor jump on long-range error finds [patch included] #32

Closed Boruch-Baum closed 6 years ago

Boruch-Baum commented 7 years ago

flyspell-correct-previous-word-generic will jump to the previous error no matter how far back in the buffer the error is located. For me, that's not a bug, but a desirable feature, but what I don't like is that after exiting the function, point jumps back to where it started instead of staying where it made the correction. If you agree that behavior is desirable, attached is a patch that seems to make the change.

Also, on a related note. I decided to use your package in conjunction with flyspell-buffer and flyspell-region to navigate forward and backward among discovered spelling errors. It works great. Here's what I do (you're welcome to use it and share it):

(setq  my-flyspell-direction t)
(defun my-flyspell-popup-wrapper (arg)
  "Search for the previous or next spelling error and suggest
corrections via popup interface. Use the prefix argument to
toggle direction."
  (interactive "P")
  (when arg (setq my-flyspell-direction (not my-flyspell-direction)))
  (if my-flyspell-direction
    (flyspell-correct-previous-word-generic (point))
   (flyspell-correct-next-word-generic (point))))

(define-key flyspell-mode-map (kbd "M-$") 'my-flyspell-popup-wrapper)

patch_fly.txt

d12frosted commented 7 years ago

Hey @Boruch-Baum,

Indeed, intention of flyspell-correct-previous-word-generic is to jump to previous word, allow user to select correction and then return to previous point. Note that it should jump to last visible incorrect word. I think in most cases it's a good thing, though I might let users to configure that 😸

but what I don't like is that after exiting the function, point jumps back to where it started instead of staying where it made the correction.

Well, for me it's a 'killer feature' of flyspell-correct-previous-word-generic 😸 Because usually I type pretty fast and I realised that I made a mistake in a word after I typed few more words. And I use flyspell-correct-previous-word-generic to quickly fix that and return to typing. Distraction free 😸

But in case you would like to disable that behaviour, I'll make it configurable.


Also you mentioned flyspell-buffer and flyspell-region. I will think about integrating them into library, so you don't have to call them manually. I mean, it might be useful in some cases 😸

I haven't updated the library in a while and you gave me some ideas 😸 Thanks a lot.

P. S. Regarding your wrapper - thanks for sharing. I think I should also expose function that behaves like your wrapper. Would it be flyspell-correct-word-around-generic?

P. P. S. At this point I think that I should've took flyspell-correct-word-previous-generic name instead of flyspell-correct-previous-word-generic 😸 But that's not that important I think.

Boruch-Baum commented 7 years ago

Hi @d12frosted. A lot for me to write today, so I'll start with some simple responses from the bottom of your most recent comment. You asked about naming of two functions. In general, I prefer shorter function names, and would have liked a shorter general prefix than flyspell-correct. Also, since all the functions seem word-based, the word word could be dropped. For now, I would just call the wrapper flyspell-correct, and the others flyspell-correct-previous, flyspell-correct-next, but read the rest of this comment first.

I started fiddling around with the code. Please see the attached file for the ideas that I came up with. If you want me to re-submit them as a formal pull-request, I can do that, but it might just be simpler to discuss the code, and let you decide what / whether you want to incorporate.

1] I noticed that flyspell-correct-previous-word-generic and flyspell-correct-next-word-generic were basically identical, so I combined them into a single function flyspell-correct-move and turned the current function names into wrappers. This should make it easier and safer to maintain the code in the future.

2] I added a feature to push the point onto the mark ring for each spelling correction, so one can review and navigate the buffer to corrected words after running the commands.

3] I added a "rapid" feature to automatically progress to every remaining spelling error in the buffer in the current direction (very cool, in my opinion).

4] I corrected my oversight to have the function continue past the limits of the buffer that are initially visually displayed (although there remains a bug there in the display of the popup in the forward direction).

5] I updated my wrapper function to allow for the rapid feature in addition to the direction toggling.

5.1] This part is the most subject to change, depending upon how you modify the underlying package.

5.2] My way of handling the various options reflects that I don't like memorizing too many different keybindings, so I prefer to have one binding and use C-u, C-u C-u, etc. That's just me, and you can surely decide different.

[ UPDATE: My next comment has an updated version of the following attachment, so if you haven't looked at it yet, ignore this version. ] flyspell-correct_ideas.txt

Boruch-Baum commented 7 years ago

This updated attached file:

1] Eliminates duplicate entries on the mark ring, and places point at most recent correction when using the rapid correct option.

2] Removes calls to save-excursion and save-restriction that seem unnecessary now.

[UPDATE] this version overtaken by the one in the next comment. flyspell-correct_ideas.txt

Boruch-Baum commented 7 years ago

This update attached file:

1] Solves the problem of the popup not displaying properly when advancing off the visible display.

2] fixed a bug I introduced by trying to save an instance of setq

This is probably the last of the changes for this round until I hear back from you.

flyspell-correct_ideas.txt

d12frosted commented 7 years ago

Thank you for sharing your thoughts and the code. I'll take a look at it soon 😸

Boruch-Baum commented 7 years ago

I discovered a bug in my code suggestion in the case of spell-checking a buffer for which the mark-ring is nil, ie. no marks had ever been set in the buffer. The attached file updates that section of code.

flyspell-popup-wrapper.txt

d12frosted commented 7 years ago

@Boruch-Baum Sorry, I am in travel mode, still haven't looked at your code. Will do ASAP once I return.

Boruch-Baum commented 7 years ago

Ack. Thanks for the heads up.

Boruch-Baum commented 6 years ago

Hi Boris. I've updated my fork of your repository to account for all your recent commits, and my patches / pull requests still seem to be working! I find the the 'rapid' feature in particular very useful for when I forget to enter flyspell mode until halfway writing a document, or when proofing someone else's work. I really think others would appreciate and benefit from the feature.

d12frosted commented 6 years ago

@Boruch-Baum great to hear! I will take a look :)

d12frosted commented 6 years ago

I am going to close this ticket and open a PR with your changes. Let's continue our discussion there. 😸