Allow to configure behaviour of C-g

[d12frosted]

Current behaviour: when hitting C-g during flyspell-correct-wrapper, the point is restored to the place where it was /before/ calling flyspell-correct-wrapper.

Desired behaviour: allow to configure flyspell-correct-wrapper in a way that C-g behaves like STOP action by default. In this case we might want to have an action to restore the point. 🤷

This concern was raised multiple times, the last occurrences that I remember are: #48 (by @Boruch-Baum) and #73 (by @WorldsEndless).

[gusbrs]

I'm not sure how general this approach is, but I'd say this is already doable without any further modification with the interfaces that allow for a dedicated keymap (I'm not sure if such maps are exposed for all interfaces though). And this might be as general as it gets, because configuring the behavior of C-g, as far as I understand, will imply rebinding it to something that would behave as we want, and we probably would not want to do it for the minibuffer-local-map or similar. Under this premise, we could get this working for the Ivy interface, for example, simply with:

(defun flyspell-correct-ivy-stop ()
  (interactive)
  (ivy-exit-with-action
   (lambda (_) (setq flyspell-correct-ivy--result (cons 'stop "")))))

(define-key flyspell-correct-ivy-map (kbd "C-g") #'flyspell-correct-ivy-stop)

This uses the feature implemented at https://github.com/d12frosted/flyspell-correct/issues/58#issuecomment-586598691, allowing to bind the skip action in flyspell-correct-ivy-map. Btw, @d12frosted , I do think this feature deserves mention in the Readme, I've been using since then and it is absolutely brilliant to be able to "skip once more" on demand, independently of having remembered to add the prefix argument before calling flyspell-correct-wrapper.

Edit: I've just learned today of ivy-make-magic-action, so that this could even become the one liner:

(define-key flyspell-correct-ivy-map (kbd "C-g") (ivy-make-magic-action 'flyspell-correct-ivy "p"))

[gusbrs]

Regarding:

In this case we might want to have an action to restore the point.

I also have some thoughts, and a suggestion.

Currently, flyspell-correct-wrapper on one hand, and flyspell-correct-previous and flyspell-correct-next, on the other, are not really consistent regarding setting the mark on the original point. The first does set the mark at its start with:

(when (or (not (mark t))
	  (/= (mark t) (point)))
  (push-mark (point) t))

While the other two don't.

This actually only makes a significant difference when the action is stop, which is the case at hand. The difference is that, if we leave the spell-checking session with action stop, popping the mark successively, we will eventually reach the original point for flyspell-correct-wrapper, but not for flyspell-correct-previous or flyspell-correct-next.

Either way, I think this could be improved for all three cases, by pushing the mark to the original point, when leaving with a stop action in all three cases. Like that we'd have the original point position at the top of the mark ring. This could be done at the end of flyspell-correct-move under the final (when hard-move-point ...) condition, perhaps also checking if we are really leaving with a stop action, as logically, there is another case when hard-move-point can be non-nil.

If this is done, when leaving with a stop action, we could return to the original point position by popping the mark, once. And consistently for all three commands (and the mark setting at the beginning of flyspell-correct-wrapper could be dropped, I think).

This, of course, would affect the stop action whether it is somehow bound to C-g or not. But I think it would provide better behavior for stop regardless of that.

[d12frosted]

Cool. I am glad that you are using that things from #58 :)

---

Either way, I think this could be improved for all three cases, ...

PR is welcome :) Regardless the stop/C-g thing.

[gusbrs]

Cool. I am glad that you are using that things from #58 :)

More than "using", this has become a game changer for my workflow. As far as "killer features" go, this is currently only second to returning point after correction. ;-)

I'm glad to try my hands on a PR. But I've got some further thoughts on mark setting, which I thought I'd share first.

Besides what I've mentioned above, I has struck me that flyspell-correct is pretty aggressive in its mark setting behavior. It actually pushes the mark at every step of the overlay loop at flyspell-correct-move, regardless of the action taken. That is, it pushes the mark at every misspelled word. If we are correcting 3-4 words in the current paragraph, this is of no major consequence. But in a longer spell-checking session, this essentially trashes the mark ring information (mark-ring-max is 16 by default, we don't need to go very far to override most or all the information there). Why does flyspell-correct do this? What is the use case for this mark setting behavior?

I suggest flyspell-correct could be more conservative in this respect. I think a good criterium would be to push the mark only when an actual change has been made (to the buffer, or to the dictionary etc) thus removing the overlay. The logic to this would be: if the overlay is intact we can go back there repeating the command, if it's not, a mark would allow us to do so.

WDYT?

If you think this is a good idea, there's still the question of what is the best point to insert the mark: if at flyspell-correct-at-point when we actually call flyspell-do-correct, or at flyspell-correct-move conditional on the action taken. The first has an advantage and a problem. The advantage is that we are really sure a change has occurred (if calling it from flyspell-correct-move and cancelling we'll have pushed the mark with no change). The problem is that when calling flyspell-correct-at-point directly there is no reason to set the mark.

Anyway, summing up, I'm suggesting two changes: i) push the mark to the original point position at the top of the mark-ring when leaving with a stop action; ii) be more selective in the mark setting behavior by pushing the mark only when there is change.

I can prepare a PR for both or either, once there's agreement on where to go.

[d12frosted]

Why does flyspell-correct do this?

For two reasons. The first one is historical. And the second one is me who didn't think about it. As you can see, mark pushing is not consistent and not thought through.

I suggest flyspell-correct could be more conservative in this respect. I think a good criterium would be to push the mark only when an actual change has been made (to the buffer, or to the dictionary etc) thus removing the overlay. The logic to this would be: if the overlay is intact we can go back there repeating the command, if it's not, a mark would allow us to do so.

Totally agree. So actions like skip should not push the mark.

Regarding the place - I think flyspell-correct-move is a better place. I totally agree that flyspell-correct-at-point when used directly should not push mark. On the other hand, flyspell-correct-move gives more flexibility on decision-making.

push the mark to the original point position at the top of the mark-ring when leaving with a stop action

Oh, this is very good idea. I like it. So after stop you can quickly restore your original location. Very clever.

be more selective in the mark setting behavior by pushing the mark only when there is change

And I like this idea as well. Good job @gusbrs

I will be happy to get a PR for both of these ideas.

[gusbrs]

I will be happy to get a PR for both of these ideas.

Great! I'll prepare it.

[gusbrs]

Hi @d12frosted , PR done!

I add some comments on it here.

I ended wrapping the flyspell-correct-move save-excursion block into unwind-protect for this, the reason I did this was that a mark clean-up was needed even when leaving with "C-g". Doing so made the diff look large, due to indentation change, but I really just tampered with mark related code. Still, I do think the unwind-protect is a good idea here beyond this particular clean-up, considering the reliance of flyspell-correct on "C-g" to functionality (it is the basic way to leave rapid-mode and to cancel an ongoing correction). But if you fell differently about it, I can step back on this.

Second, as I'm aware you know, the position of the marks set in flyspell-correct-move and the hard-move-point for the case of point initially being on a misspelled word are not really precise. Whenever we cannot rely on the machinery of save-excursion to set point, we might be a little off the desired position, if there was a difference of length in the words we corrected, relative to the original ones, during a given session. flyspell-do-correct cleverly controls for this for a single correction, but things get much more complicated in flyspell-correct-move when we are looping over a set of overlays of arbitrary length, and leaving part of the relevant data in the mark-ring along the way. This is really no big deal when we are dealing with a restricted scope of correction. Let's say, the current paragraph, or even what is currently visible on the screen. But it can become significant in a longer spell-checking session, in which case it is also doubtful if setting marks at every correction would really be worth it and called for. I think this might be particularly relevant at #69 , if it is to rely on flyspell-correct-move directly or indirectly. If this comes to be the case, perhaps a possibility to disable mark setting altogether in flyspell-correct-move (when called in a program, not interactively) would be something to consider. That should not be difficult, of course, and can be dealt with when this actually arises.

[gusbrs]

Second, as I'm aware you know, the position of the marks set in flyspell-correct-move [...] are not really precise.

Nah, that was a bug... Pre-existing, to be fair. Details in a second commit made to the PR, which fixes it.

[d12frosted]

I ended wrapping the flyspell-correct-move save-excursion block into unwind-protect for this, the reason I did this was that a mark clean-up was needed even when leaving with "C-g". Doing so made the diff look large, due to indentation change, but I really just tampered with mark related code. Still, I do think the unwind-protect is a good idea here beyond this particular clean-up, considering the reliance of flyspell-correct on "C-g" to functionality (it is the basic way to leave rapid-mode and to cancel an ongoing correction). But if you fell differently about it, I can step back on this.

Yes, good point. Thanks for doing this.

[minad]

I would like to have C-g to always stop. Then we could even remove the STOP actions from the frontends.

[d12frosted]

The more I think about it, the more I agree that it just should STOP, as you still can return to the previous place by popping the mark. @minad care to send a PR? 😸

[minad]

Maybe I will send a PR (no promises!), but with my PR #94 I am already very happy with the functionality offered by your package :)