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 quick keys #479

Merged
merged 34 commits into from
Mar 9, 2021
Merged

Add quick keys #479

merged 34 commits into from
Mar 9, 2021

Conversation

clemera
Copy link
Collaborator

@clemera clemera commented Mar 2, 2021

See #304. Ping @mohkale

@minad
Copy link
Contributor

minad commented Mar 2, 2021

How does this work? You start quick select and then each candidate is prefixed with a key or more keys, hh, hk etc depending on the number of candidates?

@clemera
Copy link
Collaborator Author

clemera commented Mar 2, 2021

The quick keys overwrite the candidate chars. I experimented with prefixing but it doesn't work well with horizontal display because the display width changes and I also think this is less bumpy so hopefully nicer as well.

@clemera
Copy link
Collaborator Author

clemera commented Mar 2, 2021

I will not merge this yet, please give it a try, I will look at it tomorrow again, good night!

@minad
Copy link
Contributor

minad commented Mar 3, 2021

I tried it. The overwriting does not work correctly. When running this from consult-buffer it adds keys like as, ad, ... I don't understand why it generates the unnecessary "a" prefix key. Regarding the implementation - why use a quick-fun closure instead of just an array of keys indexed by the candidate index - this would be more direct?

@minad
Copy link
Contributor

minad commented Mar 3, 2021

To be more precise - when it adds the keys "as", only the letter "s" overwrites the candidate and the "a" is prefixed, which makes the candidates move by one letter. And the "a" is unnecessary after all. Besides that - I like it. Maybe there is a more appropriate default face to be used which is more aligned with avy, but probably this should be fixed in the theme in the end.

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

Thanks, for trying. You get two keys when there are too few candidates to highlight with selectrum-quick-keys. I liked the visual consistency when I tried the prefix before but when overwriting it probably makes more sense to allow mixing single and multi chars.

As you noticed there is something wrong with consult-buffer. I will look into that and the closure thing you mentioned.

@minad
Copy link
Contributor

minad commented Mar 3, 2021

What about using M-q for quick select and put cycling one some other key? Maybe better because of the mnemonics or is M-m used by ivy for avy?

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

The reasoning for the bindings is coming from the bindings C-i for normal insertion and C-m for normal selection. M-i and M-m bindings aren't useful in the minibuffer and they can be seen as alternatives for the normal ones.

@minad
Copy link
Contributor

minad commented Mar 3, 2021

You get two keys when there are too few candidates to highlight

Yes, that is okay - avy does it in the same way. But I think it should still overwrite for single keys and double keys. There are two bugs - consult-buffer should not require more than a single key, I mean I still have only ten candidates. And the second bug is that the overwriting does not work correctly.

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

Yes, I did not test with consult, will look into that.

@minad
Copy link
Contributor

minad commented Mar 3, 2021

Regarding the keys, your argument makes sense. However C-m is also RET because of the ascii control key encoding, therefore I don't find the symmetry so obvious. In the default bindings you have M-m bound to back-to-indentation and C-m to RET, so there is no symmetry there either.

Generally I find the usage of the ascii control key legacy in Emacs confusing. I am currently using a modified key map where I rebind ijkl to navigation, this messes with the "default" muscle memory where C-i=TAB etc. Because of the ascii control keys I think Emacs keybindings are actually not that configurable as they should really be. It is a big mess with the input-decode-map and all these different keymap layers. Maybe I should just go back to Emacs default bindings 🤷

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

The problem is that I use selectrum--actual-num-candidates-displayed but for consult this is including the heading lines. Should we add another variable? One for inserted items and one for actual candidates?

@minad
Copy link
Contributor

minad commented Mar 3, 2021

Uh, okay. I guess we have to fix selectrum--actual-num-candidates-displayed then, such that it corresponds to the real number of candidates without the header lines? Do you know what effect that has? The number of lines generated by the display function does not necessarily correspond to the number of candidates!

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

The insertion functions return how many candidates they inserted because it depends on their behavior. But they don't know which ones are actual candidates.

@minad
Copy link
Contributor

minad commented Mar 3, 2021

The function selectrum--vertical-display-style returns the length of displayed-candidates, which is wrong. It is actually the number of lines. We should fix the candidates-display-strings function such that it returns a pair, the number of candidates and the line strings.

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

Before the grouping selectrum--candidates-display-strings did not add additional lines/candidates.

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

selectrum--candidates-display-strings could return the count of actual candidates + the candidates used for display and return that to the insertion functions, too?

@minad
Copy link
Contributor

minad commented Mar 3, 2021

Instead of returning the pair, the proper fix as I see it:

Alternatively, you may want to rewrite the candidates-display-strings function such that it returns actual groups ((group . cands) (group . cands), this would give the display style function more flexibility in the formatting. Furthermore we should not pass horizp to the candidates-display-strings function, such that things are more decoupled.

I think we should refactor that first before merging this PR.

@minad
Copy link
Contributor

minad commented Mar 3, 2021

There is the complication though that horizp also controls if annotations/extension is added. I am unsure about the best way to fix this. One could always return annotations and the horizontal style could just ignore them or one keeps the horizp/add-annotations flag.

@clemera
Copy link
Collaborator Author

clemera commented Mar 3, 2021

It is also used to decide about highlight extension.

@clemera
Copy link
Collaborator Author

clemera commented Mar 6, 2021

I need to do a some more refactorings on master before this gets merged.

@clemera clemera force-pushed the add-quick-keys branch 2 times, most recently from 33d819d to 1de86c3 Compare March 7, 2021 17:36
@clemera clemera force-pushed the add-quick-keys branch 2 times, most recently from 13ec457 to 329214e Compare March 9, 2021 19:15
@clemera clemera merged commit 62fc67e into radian-software:master Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants