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.

Sign up for GitHub

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

Jump to bottom

Add Helix editor (hx) to editors that support prefix positions #2156

Merged
merged 6 commits into from
Feb 18, 2023
Merged

Add Helix editor (hx) to editors that support prefix positions #2156

merged 6 commits into from
Feb 18, 2023

Conversation

bheylin
Copy link
Contributor

@bheylin bheylin commented Feb 12, 2023

This is dependant on Helix accepting the PR:

helix-editor/helix#5945

@bheylin
Copy link
Contributor Author

bheylin commented Feb 12, 2023

This change assumes that all Helix users update regularly.
We may want to consider a fallback to postfix position args for Helix (hx Cargo.toml:10).
But that would require checking the Helix version in use hx -V.

uncomfyhalomacro
uncomfyhalomacro reviewed Feb 12, 2023
View reviewed changes
zellij-server/src/os_input_output.rs Outdated
@@ -300,6 +300,7 @@ fn spawn_terminal(
|| command.ends_with("emacs")
|| command.ends_with("nano")
|| command.ends_with("kak")
|| command.ends_with("hx")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add helix? Some distros use the helix command instead of the hx command e.g. ArchLinux and openSUSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 9acf464

@bheylin bheylin temporarily deployed to cachix February 13, 2023 16:21 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Feb 13, 2023

Hey @bheylin - thanks for this!

For better compatibility, how about - specifically for helix - we use the old notation? I think an additional if statement or similar would be worth the total transparency for users. What do you think?

@bheylin
Copy link
Contributor Author

bheylin commented Feb 13, 2023

Hey @bheylin - thanks for this!

For better compatibility, how about - specifically for helix - we use the old notation? I think an additional if statement or similar would be worth the total transparency for users. What do you think?

I think for compatibility we have no other choice than to support the postfix notation file:<row>.
I'll make the change and at some stage in the future the code can be simplified to use prefix notation for Helix.

@bheylin bheylin temporarily deployed to cachix February 17, 2023 11:11 — with GitHub Actions Inactive
@imsnif imsnif temporarily deployed to cachix February 17, 2023 11:35 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Feb 17, 2023

Sorry, I added a comment and angered the rustfmt gods. I'll fix it, give this a small test run just to be sure and then merge if all is well.

@bheylin
Copy link
Contributor Author

bheylin commented Feb 17, 2023

Sorry, I added a comment and angered the rustfmt gods. I'll fix it, give this a small test run just to be sure and then merge if all is well.

No worries, you take the PR from here

@imsnif imsnif temporarily deployed to cachix February 18, 2023 10:13 — with GitHub Actions Inactive
@imsnif imsnif merged commit c8fdea8 into zellij-org:main Feb 18, 2023
@imsnif
Copy link
Member

imsnif commented Feb 18, 2023

Thank you very much!

joshheyse pushed a commit to joshheyse/zellij that referenced this pull request Mar 11, 2023
@bheylin @imsnif @joshheyse
feat(editor): scrollback positioning with Helix (zellij-org#2156)
47585f0 
* Add Helix editor (`hx`) to editors that support prefix positions

This is dependant on Helix accepting the PR:

helix-editor/helix#5945

* Add `helix` variant to accepted editors

* Add branch for Helix file opening

* style(code): add clarification comment

* style(fmt): whitespace

---------

Co-authored-by: Aram Drevekenin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uncomfyhalomacro uncomfyhalomacro uncomfyhalomacro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bheylin @imsnif @uncomfyhalomacro