Interferes with search in pdf-view buffers (pdf-isearch-mode)

[andersjohansson]

When using pdf-tools and its pdf-isearch-minor-mode to search in pdfs this works by pdf-isearch-minor-mode setting variables like isearch-search-fun-function to suitable functions that work on the pdf. This doesn't work at all when ctrlf-mode overrides the keys for isearch.

A simple solution would be to make it possible to disable ctrlf-mode locally. Then we could just do:

(add-hook 'pdf-isearch-minor-mode-hook (lambda () (ctrlf-mode -1)))

A more complex and more awesome solution would be to implement all the things that isearch does with these redefined functions. pdf-isearch-minor-modesets: isearch-search-fun-function, isearch-push-state-function, isearch-wrap-function and some other options.

[raxod502]

Making the mode per-buffer is a fine solution. I would be fine with adding a denylist of major/minor modes by default to CTRLF. Regarding the Isearch interface functions, is it sufficient for compatibility to call those instead of re-search-forward and re-search-backward? Does the highlighting also need to change?

[andersjohansson]

I think buffer-local is the easiest thing to do now. I looked a bit more into the code pdf-iseach uses to redefine how searches are done and it also involves things like setting state when starting and quitting, isearch-push-state-function. And there doesn't really seem to be a simple forwardand backwardfunction, maybe the redefined isearch-search-fun-function is supposed to take care of both. It also seems like pdf-iseach-search-functionis very dependent on isearch. I couldn’t find documentation of how this works beyond looking through the code, but it seems it would be too much extra stuff to add to ctrlf for little gain.

[jethrokuan]

Just wanted to add that another mode that this interferes with is shell-modes (e.g. in vterm), where Ctrl-r is used to search history.

[raxod502]

I see... unfortunately Isearch doesn't run into this problem because it's allowed to stick itself into global-map. I don't think we can get away with that, so perhaps a denylist is the best option.

There is of course also the horrifying hack solution of defining an extended menu item for the CTRLF keybinding which provides a filter function that let-binds ctrlf-mode back to nil and then does a key lookup to see if C-r would resolve to isearch-backward and only substituting with ctrlf-literal-backward in that case. But I do think that is quite a bit too horrifying.

[raxod502]

I've come up with a more robust keymapping scheme. It's a bit of a hack but should address the conflicting needs of these various bug reports. My new solution should solve https://github.com/raxod502/ctrlf/issues/51, https://github.com/raxod502/ctrlf/issues/52, https://github.com/raxod502/ctrlf/issues/67, and https://github.com/raxod502/ctrlf/issues/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.

[andersjohansson]

Hi, checking out the changes in ddfbc35, I still think that ctrlf needs to be disabled in pdf-view, since the remapping will remap isearch-forward etc. which is the command that should be invoked, but whose behaviour is changed in pdf-isearch-minor-mode (as discussed above).

This works well though:

(add-hook 'pdf-isearch-minor-mode-hook (lambda () (ctrlf-local-mode -1)))

The question is if a deny-list should be implemented. Something simple like:

(defun ctrlf-disable-local-mode ()
  "Unconditionally disable ‘ctrlf-local-mode’."
  (ctrlf-local-mode -1))

(defcustom ctrlf-deny-mode-hooks
  '(pdf-isearch-minor-mode-hook
    Info-mode-hook)
  "List of mode hooks where ctrlf should be disabled."
  :type '(repeat variable))

;; where should this be done?
(dolist (hook ctrlf-deny-mode-hooks)
  (add-hook hook #'ctrlf-disable-local-mode))

It’s no problem to write to a hook variable that is not defined, so no need to check if the mode is loaded or anything.

[raxod502]

Gotcha. Yes, that makes sense.

Honestly, thinking about the implications of load order and all that, I'd suggest not having a user option for this, and just putting the add-hook declarations individually into ctrlf.el (perhaps all wrapped in a check on a boolean user option, so that it can be disabled if absolutely necessary for some reason). Then, if you want to override a default, you can either just add-hook yourself, or you can remove-hook one of the existing ones.