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

Add multi_windows and dict support #133

Closed
wants to merge 2 commits into from

Conversation

yehuohan
Copy link
Contributor

@yehuohan yehuohan commented Jul 26, 2021

  • Add multi_windows support for [Feature] Please support multi window like easymotion #10.
  • Add dict support for HopChar and HopPattern. With this commit, we can add dict for other language easily, even support dict for number, emoji and punctation, etc.
  • Add preview for HopPattern like (easymotion-sn) what does. As it's based on dict pattern, so it supports dict preview alse.

This was referenced Jul 27, 2021
@yehuohan yehuohan changed the title Add multi_windows support Add multi_windows and dict support Jul 27, 2021
@delphinus
Copy link
Contributor

Cool features! I PR-ed #39 that has the similar feature. It uses a calculated dict for :HopChar1, and an external binary for :HopChar2. (#39 does not support :HopPattern with this reason.)

I want to merge here and #39 if possible, so that many people are happy to hop!

@yehuohan
Copy link
Contributor Author

yehuohan commented Jul 28, 2021

@delphinus With pleasure to hear more dict support. How would you like to merge cmigemo dict? Would you like commit your cmigemo dict to multi-windows branch and then I bypass here or any other way you would like?
By the way, I see there are some punctations such as , . ! in utf-8.lua. Is it possible to collect those punctations to a single dict-lua file?

@delphinus
Copy link
Contributor

I transformed this dict (utf-8.lua) from vim-easymotion's one. The original already has punctuation entries and they have Japanese-specific Regexp.

Such as, we can hop by , into these characters below.

  1. , -- U+002C COMMA
  2. , -- U+FF0C FULLWIDTH COMMA
  3. 、 -- U+3001 IDEOGRAPHIC COMMA

2. & 3. are used only in Japanese, so it is not so useful that we separate them from utf-8.lua.

@yehuohan
Copy link
Contributor Author

Think for you reply. It's true that I thought it too simple. When I look back, I find out that taking punctations as part of a language is better and we don't need to maintain the difference of punctations that various language use.

@delphinus
Copy link
Contributor

Ah, I see. But I think it is difficult to solve “What are punctuation marks?”. Because some symbols include Japanese-specific regexp, ones from vim-easymotion at least. 🤔

  -- "感嘆符" means ! in Japanese
  ['!'] = [=[\%([≠!!]\|感\_s*嘆\_s*符\)]=],
  -- 〃 is a CJK ditto mark, that is not a quotation
  ['"'] = [=[[”″“〃"]]=],

……

  -- "パーセント" means % in Japanese
  ['%'] = [=[\%([%‰%]\|パ\_s*ー\_s*セ\_s*ン\_s*ト\)]=],
  -- "アンド", "アンパサンド" means & in Japanese
  ['&'] = [=[\%([&&]\|ア\_s*ン\_s*\%(ド\|パ\_s*サ\_s*ン\_s*ド\)\)]=],
  -- "アポストロフィ" means ' in Japanese.
  -- "ズ" is for the possessive case or the plural form.
  ["'"] = [=[\%([ズ”″‘′´’'']\|ア\_s*ポ\_s*ス\_s*ト\_s*ロ\_s*フ\_s*ィ\)]=],

……

As one solution, it should be good that we limit “punctuation marks” to .,:; only -- I think this does not make much sense only 4 entries.

@yehuohan
Copy link
Contributor Author

yehuohan commented Aug 1, 2021

It's seemd that I didn't commet clear. I said taking punctations as part of a language is saying that I think we place punctuations to language lua file (like the cmigemo dict file) is better. :)

@hadronized
Copy link
Owner

Thanks for the PR, I’ll have a look asap.

@yehuohan
Copy link
Contributor Author

Add commits to fix the issue of different windows with the same buffer:

  • The old stratagy: use the window with biggest area from different windows with the same buffer.
  • The new stratagy: crop the duplicated lines area of different windows with the same buffer.

The main change is tocrop_winlines function. Hope this won't disturb your code review.

doc/hop.txt Outdated
Defaults:~
`multi_windows = false`

`dict_list`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this configuration variable is not explicit enough. Can you change it to something that conveys what it’s used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about match_mappings or search_mappings? Means that extending the match space via mapping the pattern to what match_mappings provided.

@@ -0,0 +1,98 @@
-- Ascii
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, this is going to generate a regression in lots of configuration, starting with mine, where I use unicode on some of my keys (I’m using the bépo layout, so é is a direct key to me).

Not sure this is a good idea to do it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had ignored the case for different layout. And for users who use bépo layout, the key of dict from ascii.lua may be unicode keys, but not only the ascii keys.
I fix the issue by rename ascci.lua to zh.lua ( and add some unicode mappings to zh.lua used in Chinese language ), meaning zh.lua is a basic mappings to Chinese language.

lua/hop/hint.lua Outdated
local dict_char_pat = ''
-- checkout dict-char pattern from each dict
for _, v in ipairs(opts.dict_list) do
local val = require('hop.dict.' .. v)[char]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure how caching will be done by the Lua runtime here but even then I’m not liking this style, especially because that loop could be partially moved out to prevent having to get the cached dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about executing dict require at setup()?
This commit is a simple fix to this issue. If it's a correct fix, I'm pleasure to merge it to this PR.

@hadronized
Copy link
Owner

Can you resolve your conflicts so that we I can review properly? I haven’t had to review lately.

@yehuohan
Copy link
Contributor Author

Can you resolve your conflicts so that we I can review properly? I haven’t had to review lately.

I rebased this PR into a new branch new-multi-windows. But I'm not familar with the api nvim_buf_set_extmark and got a issue with end_col value outside range even with running a simple code below with source %:

"test script

let g:ns = nvim_create_namespace('test')
call nvim_buf_set_extmark(0, g:ns , 0, 0, {
    \ 'end_line': 1,
    \ 'end_col': 2,
    \ 'hl_group': 'Question',
    \ 'hl_eol': v:true,
    \ 'priority': 100 })

@phaazon Do you have any idea about the end_col parameter?

@yehuohan
Copy link
Contributor Author

yehuohan commented Oct 27, 2021

Can you resolve your conflicts so that we I can review properly? I haven’t had to review lately.

I rebased this PR into a new branch new-multi-windows. But I'm not familar with the api nvim_buf_set_extmark and got a issue with end_col value outside range even with running a simple code below with source %:

"test script

let g:ns = nvim_create_namespace('test')
call nvim_buf_set_extmark(0, g:ns , 0, 0, {
    \ 'end_line': 1,
    \ 'end_col': 2,
    \ 'hl_group': 'Question',
    \ 'hl_eol': v:true,
    \ 'priority': 100 })

@phaazon Do you have any idea about the end_col parameter?

My fault. I mixed up that end_col is 0-based exclusive while end_line is 0-based inclusive.

@phaazon Now multi_windows and match_mappings are resovled into two commits for review.

@hadronized
Copy link
Owner

This PR has been around for a long time and its scope is too wide (multi window, dict, previews). I’m going to close this, especially because of the conflicts. This is probably my fault because of the recent (big) redesign that I did to ship Hop v1.0 along with the extensions support.

However, I would happily review a subset of that PR for the multi-window support as a priority, as people are asking for it and I see that as a top-1 feature for Hop.

For dict support and previews, one PR each is clearly something I would like to review too.

Don’t hesitate to re-open 3 PRs @yehuohan for those topics.

@hadronized hadronized closed this Nov 5, 2021
@yehuohan
Copy link
Contributor Author

yehuohan commented Nov 8, 2021

Thank your comment and I'll split out each feature as single PR with the new plugin API.

@hiberabyss
Copy link

Thank your comment and I'll split out each feature as single PR with the new plugin API.

@yehuohan The dict feature could be very useful! Is there any progress? Thanks!

@yehuohan
Copy link
Contributor Author

yehuohan commented Jun 12, 2022

Thank your comment and I'll split out each feature as single PR with the new plugin API.

@yehuohan The dict feature could be very useful! Is there any progress? Thanks!

Glad to hear that those features are helpful. And yeah, new PRs had been opened: #236 #235 #205

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.

None yet

4 participants