Conversation
|
whoa. helix mode sounds cool. thanks. this will be interesting to play around with. |
|
the ci needs to be green to proceed. i think these are just formatting |
|
The manual test sequence is helpful for those who don't use helix. However, on my mac, it doesn't work precisely as you've laid out in those bullets. I'm not sure if they're just wrong or if it's a bug. |
|
I'll check it out! I've def been testing on windows layout so it's likely something I missed. That documentation there also might have gotten out of sequence in editing too. Good reminder to make sure the keyboard layout is consistent either way |
|
@fdncred if you want to kick off CI again, i was able to fix a few bugs and think it's working pretty smoothly, ran cargo test and cargo fmt --all locally, hopefully it will pass this time, not sure how the spell checker works though |
|
This is getting pretty close to complete w/comprehensive tests for the commands listed, but i'm gonna mark this as draft temporarily since i want to try to reconcile upstream Helix's architecture a little more closely with reedline's interface... hopefully it will reduce the footprint of this PR. |
|
Are you planning on moving forward with this at all? I'd be willing to help, although my rust knowledge is very limited. |
|
Hi, I am although I haven't had too much free time for open source lately. If youd like to pick it up I think right now it would just be helpful to try the demo and double check feature parity. I'm a total rust noob too so any refactoring that's more rust-idiomatic or reduces the size of the PR would definitely be welcome. Let me know if there's anything else I can help with, I'm gonna try to use some of the holiday season to catch up on things |
|
I've just tested it. Things that work/don't:
Thank you for working on this. Looking forward to having helix mode on nushell some day :-). I too always wish I would spend more time to work on open source. |
|
Hey there, I have been dreaming of a helix mode in nushell for a long time now. There is the possibility to use ctrl-o to open helix and edit the prompt, but it does not feel natural. |
|
@kronberger-droid |
|
I have some time to revisit this. I will see what to-dos are left before this might be merge ready and will let the thread know if I find areas to help! |
|
OK, i've made significantly more process but i feel like i'm running in circles with the w/b logic. The abstraction logic is a little odd in terms of where the word/whitespace boundaries are defined, e.g. the i believe theres some underspecified and overabstracted definitions here; What we need to do is settle on/reconcile definitions of the selection and cursor indices. helix defines its model with indexes for an "anchor" and a "cursor" with the selection being defined as the characters between these indexes. I'm running into limitations here both in terms of how vi handles these boundaries and my Rust knowledge in terms of what an appropriate abstraction might look like (e.g. passing "select" as a bool to these methods seems a little out of place here). After we resolve this i think this should be ready to merge. If anyone wants to propose a way forward that would be great! The cleaner the better, if we can brute force a solution without changing the abstrations thats fine; but i need to take a break after trying that approach for a bit. If someone can propose (and we can agree on) a good abstraction model that reconciles the two modes I think that's the ideal path forward. edit: a good entrypoint to the issue would be the cursor position for "w" when there are multiple space between words. I tried mapping this to EditCommand::MoveWordRight after trying EditCommand::MoveWordRightStart incorrectly placed the cursor at the beginning of the next word instead of the space right before it (latter is correct behavior for Helix, presumably not for vi) |
|
I've been looking into this a little bit. The It would mean new I am thinking about adding two commands:
This would give Helix its own motion path with no dependency on |
|
In my perfect world we'd find a crate the implements well known emacs, vim, and kakoune/helix keybindings and implement that crate and let it do the heavy lifting. Anyone know of such a crate or even multiple crates? I know about modalkit for vim. |
|
@fdncred I know we've talked about using modalkit for this feature before. i think now that the work is a little more scoped it's probably a good idea to revisit. I believe ratatui uses crossterm for this kind of stuff, think it could work? my initial thought is that it might be more designed for a terminal app than a shell app but initially it seems like it has at least a superset of the functionality we want. i'd probably lean towards modalkit for that reason edit: interestingly, there's a ratatui-modalkit project that combines the approaches; seems liike modalkit might be the way to go |
|
I could be wrong because I haven't studied it deeply but I thought modalkit was only for vim. Do you think it could work for helix as well? Ratatui definitely can use crossterm as a backend, we use it in nushell commands like |
|
modalkit has built-in configurations for vim and emacs but its main modules are the lower level primitives we want. maybe at some point we can even push a custom helix configuration upstream |
|
What is the plan now? I figure the semantic changes I suggested are something we can work on in the future since it is quite overkill for only solving the w/W motions. Since we could solve the problem just by keeping the semantics the same and adding two entries to
Am I seeing this right that I also found a small bug which happens for Otherwise it feels like there are enough stable features to start maybe going over the code to look if we can find some things which might be able to be refactored like @schlich mentioned before. |
|
Excellent suggestions. Perhaps the new plan could just be to essentially start from a fresh branch of main and do small red/green/refactor cycles for review, that way we can get this feature out and accomplish the refactoring in parallel |
|
Should we try to fix the last bugs on here, or go forward with your plan? @schlich |
|
If you can submit a patch to get things to work go for it! I can work on tidying things up before review. Should have time in the next few days. |
dd0ef3a to
445eaac
Compare
|
alrighty, i just pushed up a total rework focused on a 100% implementation of helix mode based on modalkit's primitives. it took me a while to figure out some patterns and i still dont think i'm all the way there, but this feels a lot more sustainable and workable, with the bonus that it's not going to rely on any vi/emacs code so we can fully gate it on the feature flag. It's regressed on some of the implementation technically but i'm gonna try to get at least the common normal/visual mode and mode switching actions done before i take a break. This has been a great project for learning me some Rust! |
src/edit_mode/hx/mod.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn delete_in_insert_clears_selection() { |
There was a problem hiding this comment.
is this right? for me, trying this out in helix only deletes the singular character under the cursor
There was a problem hiding this comment.
You are right! Handler and test need to be changed.
|
|
||
| /// Parse selection notation into (buffer, anchor, head). | ||
| /// | ||
| /// `[` marks the **anchor** position and `]` marks the **head** position |
There was a problem hiding this comment.
love this convenience!
| /// | ||
| /// Reedline uses byte offsets everywhere (insertion_point, HxRange), | ||
| /// but word motion logic iterates over `Vec<char>` with char indices | ||
| /// (matching Helix's char-index Rope API). This table bridges the two |
schlich
left a comment
There was a problem hiding this comment.
Overall this is great! Glad you were able to get some of the less prominent binds/actions in on this first round. Definitely a lot of room to cut down on LOC between repeated patterns and stuff that can easily be subbed out with imports from the modalkit/keybindings crates but the logic wasn't hard to follow for either the implementation or the testing. Even still there might be more syntactic sugar we can use to make the intent of the test cases more clear like you did with the bracket notation, but i'm satisfied the test cases are thorough.
Pleasantly surprised we have enough features implemented that I think a merge could probably actually be used by most people without too many hiccups! also a good stopping point to work on refactoring before the remaining features!!
I have a decent idea of where the seams are to start on the refactoring work but that can wait for a different pr/issue 🙂
thanks for picking up the torch!
| // Internally word motions use char indices but convert to byte offsets | ||
| // via CharOffsets. All ASCII test strings have identical char/byte values. | ||
|
|
||
| /// Run a motion function against a table of (sample, scenarios). |
There was a problem hiding this comment.
i wonder if rtest fixtures would be appropriate here
| /// Whether a motion resets the selection anchor (Move) or keeps it (Extend). | ||
| #[cfg(feature = "hx")] | ||
| #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub enum Movement { |
There was a problem hiding this comment.
Maybe this can be called something like MotionType?
There was a problem hiding this comment.
Yeah probably more descriptive if read out of context.
one of my previous iterations on this modeled the interactive example after Helix's built-in tutorial. it's probably not necessary to copy the whole thing verbatim but it did give a really nice experience. @kronberger-droid i can take point on that if you want? One other idea i had and took a halphazard stab at implementing while experimenting was property-based tests with the proptest crate. I think it can reduce some of the brute-force enumeration of test cases and simultaneously illuminate underlying behavior. Definitely for follow up work thouigh! |
Sure, thanks! |
I think could probably be useful. The question I ask myself then is why helix itself doesn't use it. It seems it would not replace handwritten tests, but just help with things like in bound testing. |
Co-authored-by: Martin Kronberger <kronberger@proton.me>
yeah, i think it would be likely to catch some edge cases that went under the radar at minimum, but what i really tend to like about property based testing is that it helps the API converge on behavior a bit. With enumerated tests you have to think really hard about if you missed an edge case or if you're enumerating on too many things, and even if you get it exactly right, that logic is not going to immediately transfer to other reviewers of the tests. Property testing can make sure you are testing at an appropriate degree of granularity and modularity and you don't accidentally put yourself in any paradoxical states. Diminishing returns for sure but at some point i will probably try to produce an illustrative example. Also i agree they are not a total replacement for enumerated tests which are definitely more explicit and easier to reason about in most ways. on the other hand i'd be lying if i said i was able to review e.g. test_w very thoroughly PS its funny you say that about the Helix tests because I remember very particularly being surprised that they didnt, when i checked a few years back and was trying to better understand the helix architecture. Proof that you dont need them for stable software! |
Haha yes, but I think an editor is maybe more forgivable in this regard. I feel like you can come quite far by just using it and if you find a edge case which confuses you, you fix it, add a test case for it and move on. |
|
What are our next steps. Should i suggest the changes for |
Co-authored-by: Martin Kronberger <kronberger@proton.me>
Co-authored-by: Martin Kronberger <kronberger@proton.me>
Co-authored-by: Martin Kronberger <kronberger@proton.me>
|
hey @kronberger-droid you may have missed the convo in #1033 but i think all that remains is slicing this up into smaller PRs for iterative review. The changes above were some restructuring to facilitate these next steps as well as implementing some changes we discussed. I'm comfortable breaking things up from here just let me know if theres anywhere you'd like to jump in. my main concern is making sure you are properly attributed for the work here; creating new PRs will likely overwrite the commits you made; if you are in any way concerned about attribution please let me know and we can work on a solution to make sure your work is properly referenced. If you are not concerned i can go ahead and keep proposing the next PRs in chunks. |
|
Hey, |
Edit 2/17/2026: PR in progress!!! See comments below