Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to configure default direction of flyspell-correct-wrapper #81

Open
d12frosted opened this issue Aug 28, 2020 · 8 comments
Open
Milestone

Comments

@d12frosted
Copy link
Owner

It would be nice to have a configuration variable for the default direction of flyspell-correct-wrapper.

Optionally (I am not sure if it's a good idea, any input would be helpful), value of this variable should be modified when changing direction using universal arguments. For example, if the value of that variable is set to forward, invocation of flyspell-correct-wrapper with C-u C-u starts backward spell correction and the value of direction is also changed, so the next execution of flyspell-correct-wrapper without any universal arguments will lead to backward spell correction. What I personally dislike about this feature, it creates cognitive load, as you don't know current value of direction unless you call flyspell-correct-wrapper or inspect the value of variable. For complex cases where you constantly need to switch direction, it's better to use hydra (see #69). But I would love to hear more from users.

See the second point in #48 for more information.

@gusbrs
Copy link
Contributor

gusbrs commented Aug 31, 2020

As far as my personal taste goes, and the use I make of flyspell-correct, I wouldn't like to change the default direction of flyspell-correct-wrapper, and I would also not like this direction to be "sticky", according to the use of C-u C-u in past invocations of flyspell-correct-wrapper. Of course, configurability is something we all care about, and to each their own.

But I do see potential uses for this kind of control at another level. I'll provide two examples.

The hydra suggested at #69 does some sort of "current direction management" with my/hydra-flyspell-direction. The purpose of that is not really for a call to flyspell-correct-wrapper but in consideration of one particular case: when we just flyspell-correct-at-point we are no longer at a spelling mistake, so calling "c" again at that point would just provide an useless error message. So I provided that "c" should give a sensible default, to go to the "next" mistake, as defined by the previous navigation command. That's what my/flyspell-correct-at-point-maybe-next does.

A second example. I have my bindings for flyspell-correct as:

(:map flyspell-mode-map
      ("C-;" . flyspell-correct-wrapper)
      ("C-." . flyspell-correct-next)
      ("C-," . flyspell-correct-previous))
(:map flyspell-correct-ivy-map
      ("C-;" . flyspell-correct-ivy-skip)
      ("C-." . flyspell-correct-ivy-skip)
      ("C-," . flyspell-correct-ivy-skip))

With flyspell-correct-ivy-skip defined as in #58 (comment).

I mostly use C-;, but occasionally use C-. or C-, when I want to be explicit regarding direction. Now, the bindings in flyspell-correct-ivy-map allow me to skip by repeating the key, whichever one I have started with. But it would be nice to have something with a definite direction there too for C-. and C-,, that is, if I could have there functions able to change "mid-flight" the direction of rapid-mode.

In other words, I wouldn't really care about having such options at the "UI level", but having some such handles at an "API level" might prove interesting. Note: "might", and I'm not sure if it's worth the added complexity. The examples are not stellar either: the first ostensibly does not use flyspell-correct-wrapper, and the second is likely achievable by other means with the current existing infrastructure. Just food for thought, as you asked for opinions.

Edit: Thinking again about this, I've come to the conclusion the comment was short-sighted. Everything I said might be useful is already available as API at flyspell-correct-move, it's easy to build from there. So, just forget this... 🤷

@okamsn
Copy link

okamsn commented Aug 31, 2020

Optionally (I am not sure if it's a good idea, any input would be helpful), value of this variable should be modified when changing direction using universal arguments.

I offer the opinion that this would be confusing. At least, the behavior should be configurable.

If I understand, automatically changing the variable would make the default value less meaningful. In addition, although it can be useful, it is hard to say that a person wants to change directions for the next run just because they wanted to change directions for the current run.

Maybe a separate command to toggle the default direction in the buffer would suffice? An indicator in the modeline would help with cognitive load, but I don't think that is a counter against what I said above.

@d12frosted
Copy link
Owner Author

and I would also not like this direction to be "sticky"

@gusbrs totally agree! I also don't like this idea.

I love your second example. Simplified hydra. Though please note, that skip is not the same as rapid mode. Rapid mode also jumps to the next misspelled word after correction.

Edit: Thinking again about this, I've come to the conclusion the comment was short-sighted. Everything I said might be useful is already available as API at flyspell-correct-move, it's easy to build from there. So, just forget this... 🤷

Don't worry, I love to get more and more usecases.

I offer the opinion that this would be confusing. At least, the behavior should be configurable.

Cool. Thanks.


The more I think about the less I like the idea of having configuration variable.

We have two functions:

  • correct-previous
  • correct-next

So... it's already here :) use whatever function you like - bind your default.

Variable of direction for wrapper makes sense only if one of the following holds true:

  • we modify it by using universal arguments or using separate function (I am hard against the first one)
  • only correct-wrapper supports rapid mode
  • only correct-wrapper supports switching direction (temporarily)

So in my understanding if we

  1. provide a way to enable rapid mode in correct-previous and correct-next
  2. provide a way to change direction in correct-previous and correct-next

We don't need any configurations at all. I think both of this things can be even done via actions. So

  1. rapid mode can be enabled either by universal argument or by rapid action returned by interface
  2. direction can be changed by change-direction action returned by interface
  3. wrapper is no longer needed
  4. no variables are needed

WDYT?

@gusbrs
Copy link
Contributor

gusbrs commented Sep 2, 2020

So in my understanding if we

  1. provide a way to enable rapid mode in correct-previous and correct-next
  2. provide a way to change direction in correct-previous and correct-next

We don't need any configurations at all. I think both of this things can be even done via actions. So

  1. rapid mode can be enabled either by universal argument or by rapid action returned by interface
  2. direction can be changed by change-direction action returned by interface

I like it. Very much. +1

I've just one small thing to add. In this setting, with a heavier reliance on actions for basic aspects of the UI, it would be nice if the actions could be called by commands, and bound to keys, in a more institutionalized way than setting the value of an internal variable, as is done in #58 (comment).

@d12frosted
Copy link
Owner Author

it would be nice if the actions could be called by commands, and bound to keys

Totally agree. This should be also done.

@d12frosted
Copy link
Owner Author

Just had a though that action for changing direction is not the same as C-u C-u flyspell-correct-wrapper. In a sense that now you have to bind two functions. So maybe we should leave flyspell-correct-wrapper as is for people willing to have one dwim-style function.

@d12frosted
Copy link
Owner Author

And also, I am not sure about how this action (for changing direction) should work. I think it should act like skip (e.g. do nothing to misspelled word at point), but then it looks frustrating.

For example, I have mistakes before and after the point. I am calling flyspell-correct-previous in a rapid mode. I get to the last mistake. And I want to correct it and then switch direction. I can't use this action since it skips the word. So I have to correct the word which will end the correction session and I will have to use flyspell-correct-next now in a rapid mode. Or in case I was using flyspell-correct-wrapper - C-u C-u flyspell-correct-wrapper now. Which is totally fine as nothing has changed in the user flow.

Now, if I run flyspell-correct-previous or flyspell-correct-wrapper and realise that I didn't use the right direction - I just execute the action and current (first previous) word is skipped and I am correction first next word. Cool! In your use case @gusbrs where you have keybindings in the flyspell-correct-ivy-map - you could do something like this:

(:map flyspell-correct-ivy-map
      ("C-." . flyspell-correct-action-next)
      ("C-," . flyspell-correct-action-previous))

And even during correction session you could quickly switch directions. Which is nice.

Now, if you have only misspelled words after the point - flyspell-correct-previous and flyspell-correct-wrapper do nothing. And you can't use this action - you have to C-u C-u flyspell-correct-wrapper or flyspell-correct-next. Which also makes sense.

So it seems that I have covered all use cases and really this action could act as skip + direction change.

@gusbrs
Copy link
Contributor

gusbrs commented Sep 3, 2020

Just had a though that action for changing direction is not the same as C-u C-u flyspell-correct-wrapper. In a sense that now you have to bind two functions.

In the general case, true. But not necessarily we have to bind two outer-level bindings for correct-previous/correct-next, provided the interface has a dedicated keymap. In which case we can have a single point of entry in the global-map, for correct-previous, and other bindings in the interface keymap.

So maybe we should leave flyspell-correct-wrapper as is for people willing to have one dwim-style function.

I think these changes will make flyspell-correct-wrapper less attractive, but I can see that some people might still want it as providing a single interface. But, even if it were to be rendered functionally obsolete, it's been the main recommended binding for flyspell-correct for a while, and backwards compatibility concerns alone would suggest to keep it.

Cool! In your use case @gusbrs where you have keybindings in the flyspell-correct-ivy-map - you could do something like this:

(:map flyspell-correct-ivy-map
      ("C-." . flyspell-correct-action-next)
      ("C-," . flyspell-correct-action-previous))

And even during correction session you could quickly switch directions. Which is nice.

That is precisely what I'd love to do. ;-) I'm glad sharing that little example was not that useless after all.

So it seems that I have covered all use cases and really this action could act as skip + direction change.

I agree here. I also think it would be more useful to "toggle direction and go to 'next/previous'" rather than just "toggle direction and stay put".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants