helix
helix copied to clipboard
helix-editor
Configurable labels for custom menus and typable commands
I haven't done a ton of Rust development, and only a very little bit with Serde, so consider this an initial draft to see if the approach and proposed config format are sound. I'm happy to add docs as well if this looks reasonable.
That's a very cool change, though you'll have to update the doc.
I'm in mobile so can't check, but does this work recursively or only one level deep ? (I don't expect to ever need more than one level deep personally but others may)
It should work recursively. You can have have a labeled menu inside of another labeled menu. A very contrived version might look like:
[keys.normal.space.f]
label = "File"
f = "file_picker"
s = { label = "Save", command = ":write" }
c = { label = "Edit Config", command = ":open ~/.config/helix/config.toml" }
[keys.normal.space.f.e]
label = "Extra"
r = { label = "Reload", command = ":reload" }
I'm happy to update any docs, I just wanted to make sure the approach is sound and that label and command as terms in the configuration were OK before I started in on it.
This looks great! I use backspace to bring up a menu of build commands for different languages, so it'd be nice to relabel everything to make it look a bit prettier
It seems like there are no major objections to this approach, so I am going to try to start working on some documentation.
I believe the latest changes should return errors in any cases where I was previously choosing some default behavior that could differ from user intent.
What is the situation with this MR? @MattCheely are you still working on it?
As far as I'm aware I've resolved all of the review comments. I'm just waiting for it to be merged or to get more feedback.
As far as I'm aware I've resolved all of the review comments. I'm just waiting for it to be merged or to get more feedback.
Thanks for the update!
As I can see the situation, there is an open PR which contains this functionality and much more: https://github.com/helix-editor/helix/pull/5635, probably that one is more favourable.
Not necessarily. I think smaller self contained PRS can also be preferable and are a lot easier to review.
Our review bandwith is limited and considering the amount of PRs we recive it might take a while until a PR is reviewed
The merge in #5635 is honestly a complete rewrite as the Keymap API is basically re-written, I just wanted to give attribution to the PR author for the work he put in regardless. Sure, this PR is smaller (although the diff stat of the feature isn't). But you would have to put in at least twice the amount of time to review this PR and then mine, for basically the same feature implementations in the end.
(My extension to this feature on top of this PR is the added ability to give command sequences descriptions :) Possibly some documentation clarifications too.)
FWIW, I'm not too worried about attribution, so I don't mind what PR gets merged. I'm just keen to have the feature. 😁
Not sure if anybody is still interested in the PR, but I've been using it locally. I just rebased on the latest from master, and it seems there were some changes that introduced runtime config parsing errors. Some string type wrangling seems to have resolved it though.
I was testing it and could not make it work when it was a list of commands.
b = { label = "New Buffer", command = ":new" } work
b = { label = "New Buffer", command = [":new"] } don't work
I've updated this to work on master and got command sequences to work
Should i make a new pr, or try to push to this one, or is this not going to be merged
@Vulpesx I've been meaning to get around to updating this for the latest helix changes, but haven't had a lot of free time lately. If you want to open a PR against the branch in my fork to piggyback on this PR, feel free and I'll try to find time to test and merge it. Or if the helix team would merge a new PR, I have no objections. I'm just keen to have the feature, and don't care too much about how it lands.
I've updated this to work on master and got command sequences to work
Should i make a new pr, or try to push to this one, or is this not going to be merged
You can create a new PR against this repo so a wider audience is able to look at it :)
@Vulpesx This PR should have your changes incorporated now. Feel free to double check my work on the rebase, but I never encountered any merge conflicts and my updated local build seems to be working correctly.
I've been using the changes in this PR on my personal fork for a week now or so, and it works a treat <3
