ctrlf shouldn't require a minor mode

[astoff]

After all, it's just a collection of isolated commands that don't change the general behavior of the editor.

I didn't notice any problems when running the ctrlf commands without the minor mode, but it strikes me as wrong to permanently advice minibuffer-message. This kind of setup is typically done when entering the minibuffer and torn down afterwards. Are there any technical reasons not to do so?

Also, there are a few things in this package that seem to solve simple and common problems in unusual and overengineered ways.

First, concerning the lazy loading setup: is this meant to save a few miliseconds of init time? A normal way to lazy load the main library but not the mode would be to move the mode definition to a separate file. This new ctrlf-mode.el would also include a normal keymap that anyone can customize in the expected way. (Although, to be honest, I don't see why anyone who cares so much about startup time wouldn't just bake their keybindings into the init file).

Now, I would argue that not even the keybinding setup warrants a minor mode for ctrlf. For the user's convenience, there could be a ctrlf-setup-keybindings function which adds some remaps (from isearch commands to the ctrlf equivalent) directly in the user's global map. (If you want to autoload the body of that function definition, that would be an okay abuse from my point of view.) For those who like the defaults, calling the function is as easy as enabling the mode; for those who don't, extra configuration is needed either way.

The above arrangent also solves issue #52, since the pdf-tools remapping of isearch would take precedence over the global mode remaps done by ctrlf. [EDIT: actually, this wouldn't solve the pdf-tools problem because, apparently, pdf-tools hacks into isearch instead of defining a new command. But it would solve a similar problem with the good old DocView, as well as the problem with vterm described in loc. cit.)

Finally, the approach of adding remaps to the global map also works out of the box for anyone who already tweaks their search bindings in some way (reasonable examples: swapping C-s and C-M-s; adding handier shortcuts for the stuff under M-s; reclaim C-r for something more useful; unreasonable example: CUA fan binds isearch, literally, to Ctrl-f).

PS: Your argument against Swiper applies even to grep. Hard to argue grep is not useful because it doesn't give context lines—it's useful precisely for this reason. Also, there's swiper-isearch which is not line based.

[raxod502]

This issue report consists of several parts.

• Suggestion to have a setup function which mutates global-map, instead of a minor mode: I don't think this is a good idea. The advantage of minor modes is that they can be turned off. If I wanted to try CTRLF, but wasn't sure if I liked it, then I wouldn't appreciate it if I could not simply disable the minor mode to undo all the changes. It's not uncommon for Emacs minor modes to simply provide a way to toggle a set of keybindings. If you, personally, don't want to enable the mode---fine, nothing stops you from binding the keys manually in global-map, if you want. But I don't see a reason to remove the mode for everyone else.
• Suggestion to avoid advising minibuffer-message globally: Yes, it seems reasonable to add the advice when entering the minibuffer and remove it when leaving. I don't think that will cause any regressions. I would merge a change as such.
• Suggestion to change the lazy-loading mechanism: That's also a reasonable way of doing it. But, unless changing it would solve a bug or other concrete problem, I'd prefer to leave it the way it is, since what we have works fine, even if it's not perfect.
• Suggestion to remove the lazy-loading mechanism: I'm afraid that I think startup time optimization is something on which we may never see eye to eye. It is a matter of taste. But yes, it is really to save a few milliseconds during startup, and to avoid the user having to put in any additional work to save those milliseconds. Call it a public service.
• Suggestion to use remap: Unfortunately, that causes problems too, and I decided to go with the principle of least surprise: Helm-minibuffer-history interferes with backward search: simple solution #51.
• Possible solution to Interferes with search in pdf-view buffers (pdf-isearch-mode) #52: I think the thing that solves Interferes with search in pdf-view buffers (pdf-isearch-mode) #52 here is not binding in global-map, but rather using remap. See previous point.
• Critique of comparison to Swiper: This will always be subjective, and you are not the first person to disagree with what I wrote. I think it would probably be best to rewrite the comparisons section to be more objective, and simply lay out the feature and design differences, rather than making an argument about which way is better. Similar changes have been made to the Selectrum comparison section: readme: add link to the consult and marginalia package selectrum#238 (comment)

Let me know if I missed anything.

[astoff]

Let me know if I missed anything.

No, that's it :-)

If I wanted to try CTRLF, but wasn't sure if I liked it

...I would enable it in the destructive way, and then restart Emacs if I wanted to go back. Maybe that's actually good excuse to want Emacs to fire up in 200 ms!

Anyway, I myself would be happy as long as I can use the commands without enabling a minor mode, but I do think there will always be people confused with the inevitable conflicts that the minor mode creates, since every other package is designed around the assumption that C-s is bound in the global map, which has lower precedence.

Unfortunately, that causes problems too, and I decided to go with the principle of least surprise: #51.

This issue would be solved by mapping C-s and C-r explicitly in ctrlf-minibuffer-bindings, assuming that Helm adds its stuff to minibuffer-local-map (although I would be happy with the "bug" described in #51, since I like to go back with C-S-s and have C-r show my search history).

I'm afraid that I think startup time optimization is something on which we may never see eye to eye.

I have absolutely nothing against a fast init. In fact, I would frown upon any package which forces me, for no clear reason, to activate a minor mode in the init, since that (virtually always) means loading a package at start time.

I think the thing that solves #52 here is not binding in global-map, but rather using remap. See previous point.

pdf-tools is weird, as I noted in the edit to my original message, but let's take the vterm issue described in the same issue. We have four option: to map commands (1) directly or (2) via remaps to (A) the global map or (B) a minor-mode map. I'm fairly certain that only combination (1B) will clash with a major mode that rebinds C-s and C-r.

[raxod502]

This issue would be solved by mapping C-s and C-r explicitly in ctrlf-minibuffer-bindings

Ah, but then you have the problem of using remap bindings in one keymap provided by the package, and hardcoded bindings in a different keymap. The advantage of remap bindings is largely that no extra configuration is necessary in order to use custom underlying bindings, and it seems to me like having the hardcoded bindings in a separate keymap largely eliminates this advantage.

[astoff]

Yes, the approach I'm suggesting (which I can summarize once again if you like, since the ideas are quite scattered) would be responsive to the user's configuration to enter ctrlf, but not once inside it. I think this is the best you can do before resorting to weird hacks. Note, in any case, that none of "reasonable examples" in my original message strips C-s of the "search" meaning, so they still benefit from the "remaps in the global map" approach for the commands.

PS: #67 is yet another issue caused by binding C-s to a keymap with too high priority. This won't stop as long as a ctrlf-mode exists...

PPS: If you really want it, you can make the hypothetical ctrlf-setup-keybindings command save the old bindings, thereby being reversible. And then you can even still call it ctrlf-mode.

PPPS: If you really want to make the minibuffer keybindings responsive to the user's config, you could try adding the global-map remaps first, then call something like where-is, and then include those keys (not as remaps) in the minibuffer map instead of hardcoding C-s and C-r. I personally would rather provide an explanation and a 3-line config snippet in the readme instead of trying to be too clever here.

[raxod502]

I've come up with a compromise that maintains ctrlf-mode as a minor mode, which I would like to preserve if possible. My new solution should solve #51, #52, #67, and #80 simultaneously while also working with remap bindings. I have tested it a bit and it seems to work, but I don't actually use most of the modes that have been mentioned in these issue reports, so I could use some help to confirm if the bug is resolved for all of them.

[astoff]

I still see drawbacks in the new scheme, in comparison with the tried-and-true approach of instructing the user to bind some keys to their global-map:

• To a Swiper user (who will typically have C-s bound to swiper-isearch in the global-map, as per Swiper's readme), ctrlf-mode will seem to be broken.
• It's tricky to make regexp search the default, i.e., bound to C-s. One needs to know about remaps and the internals of ctrlf to understand that binding isearch-forward-regexp to C-s in the global map will do the job.
• Even worse, if I wanted to make fuzzy search the default, I wouldn't immediately know how to achieve this in the new scheme — and I know Elisp and have read large chunks ctrlf's source code!

[raxod502]

To a Swiper user (who will typically have C-s bound to swiper-isearch in the global-map, as per Swiper's readme), ctrlf-mode will seem to be broken.

Won't pressing C-s in that configuration just trigger Swiper rather than CTRLF? I wouldn't call that broken---it seems like exactly what you'd expect from explicitly remapping C-s. You could always explicitly bind CTRLF to different keys if you want, and that would work equally well. What behavior would make more sense than what I've described here?

It's tricky to make regexp search the default [...] if I wanted to make fuzzy search the default, I wouldn't immediately know how to achieve this

You'd do this by adjusting ctrlf-mode-bindings, replacing ctrlf-forward-literal with ctrlf-forward-regexp or ctrlf-forward-fuzzy, and analogously for the other entries. That seems reasonably straightforward to me, but it could be improved in three ways:

• use a keymap instead of an alist (Add ctrlf-mode-map #64)
• add a user option specifically for customizing the default and alternate search styles: customise default search style without needing to rebind default keys #83
• add explicit documentation in the README for how to customize the default search style

What do you think of that?

The reason I resist removing the minor mode is that it will reduce the out-of-box user experience. I think it is better when software automatically works without the need for customization. Enabling a single minor mode to turn on a package is the best-case scenario for realizing this goal. If we didn't have a minor mode, then:

• users would have to bind multiple keys in their configuration (one for each of the six entries in ctrlf-mode-bindings)
• if we added a new default binding, users would not benefit from it on an upgrade
• we would have to add some kind of setup function to install the advice currently enabled (and disabled) in the minor mode
• if we added new bookkeeping or setup, we would have no way to make sure it was activated in existing configurations
• undoing the changes of CTRLF now requires knowledge of package internals, unless you restart Emacs

[astoff]

I wouldn't call that broken---it seems like exactly what you'd expect from explicitly remapping C-s.

Fair enough. I only brought this up because you are keen on providing a way to test the package which is reversible without restarting Emacs, and this stopped being possible in this common use-case.

• users would have to bind multiple keys in their configuration (one for each of the six entries in ctrlf-mode-bindings)

It looks like we both agree that setting up some keybindings in the init file is kinda clunky. However, this is how every other package works, and how everyone expects this to work.

On the plus side, copying six lines of (define-keys to my config would teach me exactly which commands are available.

• if we added a new default binding, users would not benefit from it on an upgrade

If some random new keybinding appeared on my config just because I updated a package, it's likely I would either never notice it, or be annoyed by the change.

• we would have to add some kind of setup function to install the advice currently enabled (and disabled) in the minor mode
• if we added new bookkeeping or setup, we would have no way to make sure it was activated in existing configurations

Okay, a global change to Emacs's behavior warrants a minor mode. But why does Ctrlf, being just a collection of search commands, needs to make any global modification to Emacs, apart from some keybindings? Why would it ever need any bookkeeping to run outside of a search session?

• undoing the changes of CTRLF now requires knowledge of package internals, unless you restart Emacs

This is the only problem you are solving here IMO. The solution is aptly implemented, given Emacs's constraints; unfortunately, it seems complex and brittle.

---

More importantly though: I wanted to bring to your attention that binding C-s and C-r to a minor mode map wreaks havoc because of the keymap precedence. It's better now with remaps.

Now, concerning #51 and the trick where you bind some stuff to nil: The conventional way to solve this problem would be to bind Ctrlf's forward/backward commands to C-s/C-r in Ctrlf's own minibuffer map. Looks much simpler and reliable. Does that not work, by any chance? Swiper works that way, and has zero key confilcts AFAIK (except with pdf-tools, but that's arguably a pdf-tools issue).

[raxod502]

Does that not work, by any chance?

Yeah, unfortunately, there's a serious problem with that, which is that it doesn't work with remaps. If I were to bind [remap isearch-backward] in the minibuffer map, then by default pressing C-r would execute helm-minibuffer-history. If I were to bind C-r directly, then it would work as intended, but then we wouldn't be using remaps anymore (and besides we'd have the issues #52, #67 as before).

The solution is aptly implemented, given Emacs's constraints; unfortunately, it seems complex and brittle.

Why thank you! :3
I admit your criticisms; they're fair.

this is how every other package works

I wouldn't say that's entirely the case, though---for example, looking through the modes enabled in my configuration, Counsel provides counsel-mode which is a collection of keybindings; Winner mode defines keys in its minor mode; Outline mode establishes bindings under C-c @; undo-tree rebinds standard Emacs commands as well as providing new ones; Smartparens provides a number of keybindings in its minor mode; and dumb-jump.el is a minor mode which provides only keybindings.

Actually, Counsel is just like CTRLF---it mostly provides just keybindings in its minor mode, but enabling the mode also enables a piece of advice :)

Why would it ever need any bookkeeping to run outside of a search session?

See ctrlf--condense-overlays and the associated advice, without which you'll get some fairly horrifying visual bugs.

[astoff]

If I were to bind C-r directly, then it would work as intended, but then we wouldn't be using remaps anymore (and besides we'd have the issues #52, #67 as before).

I don't understand this, and perhaps we are talking about different things. To me, the upshot of the discussion is: direct bindings in the minibuffer-local map are good; direct bindings in Ctrlf's minor mode map are bad. To expand this a bit, each "minibuffer session" (be it a read-from-minibuffer, a completing-read, or a completion-framework patched version thereof) installs a keymap in the minibuffer. If you bind C-s/C-r to ctrlf-forward/backward in Ctrlf's minibuffer map, and Ido binds C-s/C-r to its own thing in the minibuffer sessions it manages, there's no conflict that could possibly arise. Right?

Concerning #52, note that pdf-tools breaks with any alternative search package, and therefore is the real culprit here. This is so because it assumes C-s is bound to the good old isearch in the global map and doesn't bind anything to its local map. In contrast, note that there is no conflict between Ctrlf and the built-in DocView mode after your latest change.

I wouldn't say that's entirely the case, though

Winner mode is a bona fide minor mode, because it needs to somehow watch for window configuration changes. Counsel's minor mode is a mere convenience, I'd say (and that piece of advice it adds but never removes is not kosher, is it?). I actually prefer the approach of the Consult package, which is to provide a sample configuration for the user to copy and adapt to their taste. Of course that's not the way you want to do it here, which is all fine and good.

On a tangent: I generally find those minor mode bindings useless because of their length. Who types C-c @ C-f C-c @ C-f C-c @ C-f to jump 3 outline sections over? When I find a command valuable, I usually just sacrifice some other short keybinding for it.

See ctrlf--condense-overlays and the associated advice, without which you'll get some fairly horrifying visual bugs.

I don't know the details here, but is this supposed so fix said horrifying visual bugs in every minibuffer session, even those not "owned" by Ctrlf? Then perhaps there is a case to make this independent of the search commands.

Last but not least: I think I made my point here as well as I could. You have a different vision in some aspects, so please heed or ignore any of these comments as you see fit, and feel free to close the issue.

[raxod502]

direct bindings in the minibuffer-local map are good; direct bindings in Ctrlf's minor mode map are bad

Ah, well, the important feature to me is that the binding to start a search is the same as the one to continue one. If I'm used to C-s C-s, and then I enable CTRLF, then C-s C-s has the same effect as usual. If I have made regexp the default Isearch mode by swapping the global bindings for isearch-forward and isearch-forward-regexp, then C-M-s C-M-s starts and continues a literal search. But then if I enable CTRLF (with your proposed set of bindings), C-M-s C-M-s would start a literal search but then immediately switch it to regexp. That is what I want to avoid.

Who types C-c @ C-f C-c @ C-f C-c @ C-f to jump 3 outline sections over?

Well, presumably, you'd type C-3 C-c @ C-f, but I won't say it's not still inconvenient.

is this supposed so fix said horrifying visual bugs in every minibuffer session, even those not "owned" by Ctrlf?

Yes. It's mandatory because it needs to apply to commands external to CTRLF that might interfere with CTRLF search sessions asynchronously, unfortunately. Perhaps it would be possible to dynamically install and remove the advice as search sessions start and end. That would be an improvement.

I think I made my point here as well as I could. You have a different vision in some aspects, so please heed or ignore any of these comments as you see fit, and feel free to close the issue.

Sounds good!