helix icon indicating copy to clipboard operation
helix copied to clipboard

Keymap presentation refinements and underlying restructuring + consistent config use

Open gibbz00 opened this issue 1 year ago • 9 comments

Commit messages explain most of it. Proceeds: https://github.com/helix-editor/helix/pull/3053

Before After
image image
2023-01-17-170540_screenshot 2023-01-17-170522_screenshot

Command palette: Should I change the ">" character to a unicode arrow like: → ? Should I properly escape less than and greater than from lt and gt to < and > respectively?

Infoboxes issues which haven't been touched upon: https://github.com/helix-editor/helix/pull/5203, https://github.com/helix-editor/helix/pull/3958

EDIT: Accidentally snuck in https://github.com/helix-editor/helix/pull/5130, so guess this proceeds that one too, can open up a new PR which leaves it out if needed.

gibbz00 avatar Jan 17 '23 16:01 gibbz00

It looks like the keys are now sorted in the popup? We deliberately keep the order the same as defined in the keymap, because left/right/up/down should be grouped together and not sorted alphabetically for example.

archseer avatar Jan 17 '23 16:01 archseer

@gibbz00 could you maybe provide a summary of what exactly this does. There are 43 commits in this PR so reading trough them all is kind of a chore

pascalkuthe avatar Jan 17 '23 16:01 pascalkuthe

It looks like the keys are now sorted in the popup? We deliberately keep the order the same as defined in the keymap, because left/right/up/down should be grouped together and not sorted alphabetically for example.

Sorting only worked when keymap was added by the keymap! macro and not when loaded by user config. Also, arrow keys are often also bound with hjkl keys, which will show first. Order is: one letter -> C keys -> multi-letter. They aren't shown in the images because I have no_op them.

gibbz00 avatar Jan 17 '23 16:01 gibbz00

Right, but either way the popup should be arranged in definition order. Certain popups like goto are manually ordered by grouping together similar sets of functionality.

For instance w v and t are common operations so they were listed on top. left/right/up/down are now spread across the whole infobox

archseer avatar Jan 17 '23 16:01 archseer

Right, but either way the popup should be arranged in definition order. Certain popups like goto are manually ordered by grouping together similar sets of functionality.

For instance w v and t are common operations so they were listed on top. left/right/up/down are now spread across the whole infobox

No config:

image

gibbz00 avatar Jan 17 '23 16:01 gibbz00

Hmm it could be non-deterministic? I have no config and it's sorting by arrow direction first (not hjkl)

archseer avatar Jan 17 '23 16:01 archseer

Hmm it could be non-deterministic? I have no config and it's sorting by arrow direction first (not hjkl)

When using this PR?

gibbz00 avatar Jan 17 '23 16:01 gibbz00

Certain popups like goto are manually ordered by grouping together similar sets of functionality.

I would still argue that it is easier to find the command when they're sorted alphabetically.

image

gibbz00 avatar Jan 17 '23 16:01 gibbz00

@gibbz00 could you maybe provide a summary of what exactly this does. There are 43 commits in this PR so reading trough them all is kind of a chore

@pascalkuthe clean up of keymap, main.rs, docgen.rs, health.rs. It's a large PR so the commits are the summaries. I'm sorry :(

gibbz00 avatar Jan 17 '23 16:01 gibbz00

Alphabetical sort: we had that initially then replaced it, so this is strictly a downgrade. The goto popup is a great example:

  • g is on the top since it's a repetition of the key
  • h/l are grouped together
  • t/c/b are grouped together
  • a/m are grouped together
  • n/p
  • All the jumps that are LSP specific are also grouped

Alphabetical sort is useful if you're trying to find what a specific key does. But the current order is intended to make it easier to find based on functionality. Kakoune also sorts this similarly.

I like some of the cleanup in this PR though so I might start cherry picking changes into master to shrink the size of this PR a bit and make it easier to review. But I'd avoid renaming types just for the sake of clarity where possible: this is a large project with a lot of PRs in queue and any type of renaming can cause merge conflicts on a large portion of them.

archseer avatar Jan 18 '23 02:01 archseer

I've merged #5130 and #3053 so they can be rebased out from this PR now

archseer avatar Jan 18 '23 05:01 archseer

Alphabetical sort is useful if you're trying to find what a specific key does. But the current order is intended to make it easier to find based on functionality.

Ok, I'm starting to see your point. I can put the ordering from the default ordering back, can, but can I add alphabetical sort as a config option? I have a lot of config keybindings and the info boxes become because of that very messy.

I can also send in split PRs, maybe that can help with the review process.

But I'd avoid renaming types just for the sake of clarity where possible: this is a large project with a lot of PRs in queue and any type of renaming can cause merge conflicts on a large portion of them.

Of course, I'll try to limit this unless I strongly feel that the naming is outright deceptive.

gibbz00 avatar Jan 19 '23 12:01 gibbz00