minad / jinx

🪄 Enchanted Spell Checker
GNU General Public License v3.0
432 stars 21 forks source link

Repeated calls of `jinx-next` and `jinx-previous` outside of the minibuffer #60

Closed gusbrs closed 1 year ago

gusbrs commented 1 year ago

Hi @minad , I've seen you've generalized jinx-next and jinx-previous to work outside of the minibuffer and bound them in jinx-misspelled-map. You know I find them useful (https://github.com/minad/jinx/issues/30#issuecomment-1518148788), thanks!

I might be missing something, but it is hard to take full advantage of the feature, because jinx-next places point at overlay-end, where there's no overlay, so we cannot repeat the call, because there jinx-misspelled-map is not active.

I take it you meant to keep things consistent with the case where (minibufferp). And, of course, it is also visually better, since the cursor does not overlap with the word of interest. But inside the loop in jinx-correct we know beforehand the whole set of overlays, so it's easy to do that. However, when using jinx-next and jinx-previous in the buffer itself, we don't have that luxury.

gusbrs commented 1 year ago

Ah, using repeat-mode. Clever. Thank you!

minad commented 1 year ago

Yes, we also win the "n" and "p" keys thanks to the repeat mode. But overall the situation is not ideal with keymaps attached to overlays. The alternative would be to put the point on the overlay either right at the start or the end, but neither is a nice solution. I observed this problem before.

Another alternative is Embark integration:

    (keymap-set jinx-repeat-map "RET" jinx-correct)
    (embark-define-overlay-target jinx category (eq %p 'jinx-overlay))
    (add-to-list 'embark-target-finders 'embark-target-jinx-at-point)
    (add-to-list 'embark-keymap-alist '(jinx jinx-repeat-map embark-general-map))
    (add-to-list 'embark-repeat-actions #'jinx-next)
    (add-to-list 'embark-repeat-actions #'jinx-previous)
    (add-to-list 'embark-target-injection-hooks (list #'jinx-correct #'embark--ignore-target))))
minad commented 1 year ago

I also use this in my configuration for Flymake:

(put 'flymake-warning 'keymap '+flymake-overlay-map)
(put 'flymake-error 'keymap '+flymake-overlay-map)
(put 'flymake-note 'keymap '+flymake-overlay-map)
(defvar-keymap +flymake-overlay-map
  "M-n" #'flymake-goto-next-error
  "M-p" #'flymake-goto-prev-error
  "M-l" #'flymake-show-buffer-diagnostics)
(fset '+flymake-overlay-map +flymake-overlay-map)
(defvar-keymap +flymake-repeat-map
  :repeat (:enter (flymake-goto-next-error flymake-goto-prev-error))
  "M-n" #'flymake-goto-next-error
  "M-p" #'flymake-goto-prev-error
  "M-l" #'flymake-show-buffer-diagnostics
  "n" #'flymake-goto-next-error
  "p" #'flymake-goto-prev-error
  "l" #'flymake-show-buffer-diagnostics)
gusbrs commented 1 year ago

Yes, we also win the "n" and "p" keys thanks to the repeat mode.

I've seen it. Neat.

But overall the situation is not ideal with keymaps attached to overlays. The alternative would be to put the point on the overlay either right at the start or the end, but neither is a nice solution.

It's a tradeoff, I get the problem. Neither is ideal. But I think you went for a good solution. I hadn't thought of that. Btw, do you have any idea of how popular repeate-mode has become? I'm one happy user :-) but, in principle it is somewhat new, and disabled by default.

Another alternative is Embark integration:

Nice! I'll have to wait for embark-define-overlay-target, since I'm using release here. But I'm looking forward to it. Thanks!

I also use this in my configuration for Flymake:

Isn't this pretty much the same concept as what you adopted here? (Which is nice, of course, but perhaps I'm missing something).

minad commented 1 year ago

Btw, do you have any idea of how popular repeate-mode has become? I'm one happy user :-) but, in principle it is somewhat new, and disabled by default.

I think it is quite popular. It fills a real niche. Before that people mostly used Hydra or Transient for that, but these are way too heavy, also visually. Note also that there had been some ad-hoc repeat modes present in Emacs all the time, e.g., for text scaling. Repeat mode is just the generalization of that. Embark also brings its own repeat functionality, but it fills a slightly different niche.

Nice! I'll have to wait for embark-define-overlay-target, since I'm using release here. But I'm looking forward to it. Thanks!

I recommend the gnu-devel and nongnu-devel archives. I use rolling release for all installed packages, but I only use packages which break almost never. Most importantly, I want to contribute to packages so I like to use the newest development snapshots.

Isn't this pretty much the same concept as what you adopted here? (Which is nice, of course, but perhaps I'm missing something).

Yes, it is exactly the same. One could upstream this to Flymake.

minad commented 1 year ago

A word about Transient, even if I personally don't use it much. I solve most problems with normal prefix maps, repeat mode and Embark keymaps. Note that which-key closes the gap since it enhances prefix maps and repeat maps with a visual element. (However I also usually don't have which-key enabled since embark-prefix-help-command works sufficiently well.)

But of course Transient is much richer if one wants to modify arguments and settings with visual feedback. Therefore I think it has its place and should be reserved for rich UIs. It should not be used for simple keybinding problems, where repeat mode is a simpler and better fit. For example Transient could work as a jinx-correct UI as has been suggested in the other issue.

gusbrs commented 1 year ago

I think it is quite popular.

That's my impression too. But you certainly have a better perspective than mine.

I recommend the gnu-devel and nongnu-devel archives.

I know that's a widespread recommendation. But I'm an odd duck in that regard. Here releases have priority over rolling (and built-in over both), and I only pin to gnu-devel or melpa on specific cases (currently only two, jinx included). I very much prefer it. And it doesn't stop me from contributing. ;-)

A word about Transient

Oh, yes, out of my craving for a whole buffer jinx-correct which does not recheck everything every time, I recycled my old transient for flyspell and adapted it for jinx. My "solution" to this issue's problem in this context was to set the cursor type to bar for the duration of the transient (https://old.reddit.com/r/emacs/comments/12ugjhj/).