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

rewrite search related code with isearch(ready to be reviewed) #560

Closed
wants to merge 1 commit into from

Conversation

jixiuf
Copy link
Contributor

@jixiuf jixiuf commented Feb 14, 2024

After a isearch(C-s) search, you can use n (meow-search ) or ;n ( nemw-reverse+meow-search) to search next/previous occurrence now.
need

(setq isearch-lazy-count t)
(setq isearch-lazy-highlight t)

image

@jixiuf jixiuf force-pushed the isearch branch 4 times, most recently from 0a352e0 to fca50ee Compare February 14, 2024 06:30
@jixiuf jixiuf changed the title [WIP]rewrite search related code with isearch rewrite search related code with isearch(ready to be reviewed) Feb 14, 2024
@DogLooksGood
Copy link
Collaborator

Thanks! I like the idea to reuse the functionalities of isearch/lazy-highlight.

A couple of issues I noticed,

  1. The meow-reverse is used to reverse the selection, you shouldn't use it to initiate search selection. All commands must be very clear for whether it works on a selection. meow-reverse is a selection specific command, thus it must do nothing and fallback to the fallback command. Use negative-argument for backward navigation.
  2. Using (message "") to hide the output isn't right. A command shouldn't have unexpected output. And in generally, a command should not call-interactively a command, unless it's a wrapper for that command.
  3. (setq msg (string-trim-right msg)) generates a global variable, it should be avoid.

@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 14, 2024

  1. for meow-reverse: they are doing the same thing.
    my branch:
(save-mark-and-excursion
         (meow-search nil)
         (setq this-command last-command))

old master branch:

(meow--highlight-regexp-in-buffer (car regexp-search-ring))
  1. fixed.
  2. msg is the argument of that function, it should not generates a global variable.

edit:
about (message "")
meow--search is used to search all occurrences in the buffer, when it stop at the last match, an error message would show, I think it is not needn't there?

@jixiuf jixiuf force-pushed the isearch branch 3 times, most recently from b333448 to abe8700 Compare February 14, 2024 09:14
@DogLooksGood
Copy link
Collaborator

I haven't tried it yet.
For meow-reverse, it shouldn't do anything if there's no selection. So there won't be a usage like ;n.
For the message stuff, typically in this case, we should go deep into the underlying implementation, avoid to use a high-level command directly.

@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 14, 2024

I haven't tried it yet. For meow-reverse, it shouldn't do anything if there's no selection. So there won't be a usage like ;n.

yes a selection is needed, After a isearch(C-s) search, the result would be selected, so ;n works

For the message stuff, typically in this case, we should go deep into the underlying implementation, avoid to use a high-level command directly.

I think let the isearch handle the detail, that is the easist way.

case-fold-search
search-upper-case
search-invisible
regexp searches or literal searches
These variables will affect the search results. If implemented on your own, it would essentially mean redoing what isearch does, otherwise, it can only support some specific cases. Currently, for the visit type of selection (not only the few internal commands like meow-visit, meow-mark-word create), the regular isearch command will also create a visit type of selection through the isearch-mode-end-hook. Therefore, using its isearch-repeat to determine the position of the fake cursor is the simplest and most accurate method.

这些变量 会影响搜索的结果, 如果自行实现,相当于把isearch做 的事重做一遍,否则只能支持其中某些特例。
目前对于 visit 类型的selection( 不只 meow-visit、meow-mark-word 等几个内部命令会创建),
普通的isearch 命令 也会通过isearch-mode-end-hook 人际 visit 类型的selection
所以直接使用它的 isearch-repeat 来决定fake cursor的位置是最简单也是最准确的

@jixiuf jixiuf force-pushed the isearch branch 3 times, most recently from 066c303 to ec2aacd Compare February 14, 2024 12:09
@DogLooksGood
Copy link
Collaborator

I'll give it a try.

@jixiuf jixiuf force-pushed the isearch branch 3 times, most recently from 290e0e4 to 8617f38 Compare February 16, 2024 06:20
@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 16, 2024

The update addresses the following two issues:

  1. Fixed the logic for clearing highlights, ensuring that highlights generated by the native isearch command won't be mistakenly cleared by pre-command-hook after setting (setq lazy-highlight-cleanup nil).
  2. After meow-mark-word, the selected region is marked as expandable word. meow-mark-word will use meow-search to perform a search, and the hit area will be marked as visit, meaning the selection changes from expandable word to unexpandable visit. This logic has now been corrected to no longer alter an existing selection.

更新了一版 处理以下2个问题:

  1. 清理highlight 的逻辑,确保(setq lazy-highlight-cleanup nil) 后使用原生isearch 命令产生的highlight不会被pre-command-hook误清理掉.
  2. meow-mark-word 后 会将选中区域标记为expandable word ,meow-mark-word 中会使用meow-search 执行搜索,搜索命中的区域会被标记为visit ,即 选区由expandable word -> unexpandable visit. 现将上面的逻辑做一个修正,不再变更已存在的selection

@DogLooksGood
Copy link
Collaborator

DogLooksGood commented Feb 27, 2024

@jixiuf I just tried the PR. I think it mostly works good, but there are edge cases.

  1. There's a use case in Vanilla Emacs where you start the selection with C-SPC, then extend it to somewhere with isearch, and this PR breaks the default behavior.
  2. The visit selection is extended by concecutive searches. For example, a C-s followed by a C-r with same search results in an empty selection.

I think the default command behaviors must remain the same. Probably we should look for a not so tight integration between meow's visit and isearch. For example, instead of creating a selection immediately, we can create this selection with a following meow-search.

@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 28, 2024

I make another force-push, could you try it.

@DogLooksGood
Copy link
Collaborator

It seems that meow-visit and meow-search no longer create selection.

@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 29, 2024

It seems that meow-visit and meow-search no longer create selection.

Yes.
So you want to create selection after meow-visit and meow-search but do not want it after isearch-forward?

@DogLooksGood
Copy link
Collaborator

Yes. The point is we shouldn't modify any of the builtin behaviors. Well meow-visit and meow-search create visit type selections.

@jixiuf
Copy link
Contributor Author

jixiuf commented Feb 29, 2024

another force-push.

@DogLooksGood
Copy link
Collaborator

Sorry, I think there are still some issues.

  1. We lost the occurrence counter.
  2. The meow-search is affected by the isearch direction.
  3. A post command hook is introduced, while I think it's not necessary.

Please leave the PR here, I may want to change something before I merge it. Thanks for the contribution anyway!

@jixiuf
Copy link
Contributor Author

jixiuf commented Mar 1, 2024

Sorry, I think there are still some issues.

  1. We lost the occurrence counter.
  2. The meow-search is affected by the isearch direction.
  3. A post command hook is introduced, while I think it's not necessary.

Please leave the PR here, I may want to change something before I merge it. Thanks for the contribution anyway!

It's ok. but for the occurrence counter you can just (setq isearch-lazy-count t)

@DogLooksGood
Copy link
Collaborator

DogLooksGood commented Oct 17, 2024

Sorry for the late response, I fixed the conflicts and pushed to a branch, check this: 9d94d7e

It still has some issues. I'll find some time to work on it.

I'll close this issue for now.

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

Successfully merging this pull request may close these issues.

2 participants