meow-edit / meow

Yet another modal editing on Emacs / 猫态编辑
GNU General Public License v3.0
1.07k stars 128 forks source link

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

Open jixiuf opened 3 months ago

jixiuf commented 3 months ago

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

DogLooksGood commented 3 months ago

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 commented 3 months ago
  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))
  2. fixed.
  3. 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?

DogLooksGood commented 3 months ago

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 commented 3 months ago

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的位置是最简单也是最准确的

DogLooksGood commented 3 months ago

I'll give it a try.

jixiuf commented 3 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

DogLooksGood commented 2 months ago

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

jixiuf commented 2 months ago

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 commented 2 months ago

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

jixiuf commented 2 months ago

another force-push.

DogLooksGood commented 2 months ago

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 commented 2 months ago

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)