zulip-terminal
zulip-terminal copied to clipboard
Edit message in an terminal editor subprocess
Create a new command OPEN_IN_TERMINAL_EDITOR (default ctrl o
) for editing the current message in terminal editor with python tempfile.
What does this PR do, and why?
I am using helix as my terminal editor and when I write in terminal I found myself using it's shortcut.
External discussion & connections
- [x] Discussed in #zulip-terminal in
Composing in terminal editor #T1394
- [ ] Fully fixes #
- [ ] Partially fixes issue #
- [ ] Builds upon previous unmerged work in PR #
- [ ] Is a follow-up to work in PR #
- [ ] Requires merge of PR #
- [ ] Merge will enable work on #
How did you test this?
- [ ] Manually - Behavioral changes
- [x] Manually - Visual changes
- [ ] Adapting existing automated tests
- [ ] Adding automated tests for new behavior (or missing tests)
- [ ] Existing automated tests should already cover this (only a refactor of tested code)
Self-review checklist for each commit
- [x] It is a minimal coherent idea
- [x] It has a commit summary following the documented style (title & body)
- [ ] It has a commit summary describing the motivation and reasoning for the change
- [x] It individually passes linting and tests
- [ ] It contains test additions for any new behavior
- [ ] It flows clearly from a previous branch commit, and/or prepares for the next commit
Visual changes
I update the pull request by adding an environment variable $ZULIP_EDITOR_COMMAND
for custom command like lapce -n -w TEMPFILE
which allow even desktop application , check and report if the program is not found.
I rename the command OPEN_EXTERNAL_EDITOR
.
I updated it.
I missed before the trailing empty line that may be create by code editor, to fix that I now use .rstrip()
to remove trailing empty line and space.
In addition to the comments, should I document $ZULIP_EDITOR_COMMAND somewhere, maybe in the README ?
I tested in most of my code editors I could use TEMPFILE:LINE:CHARACTER
to position the cursor on opening.
Zulip terminal will try to position the cursor at the same place as before the message was edit. There is no problem when the new message is smaller than cursor position.
@andersk Based on feedback on previous external-command PRs, do you have any concerns with this implementation?
I made an update using oslex library to split the command. There are errors because of updates of linting tools, I could fix them but it should be in another pull request.
It still work on my Linux setup for desktop and terminal editor.
I don't have a Windows computer to test it but does shutil.which
work well on Windows (with just exe name or the full path) ?
Update using shlex instead of oslex
@mek-yt Unfortunately this great PR got a bit buried with other work over the Summer, but seems essentially good to go?
Re-reading this:
- We don't support running on windows right now, but we could also explicitly block this on windows if path handling is incomplete (or of concern)
- Alternatively, or in addition, we could sit this behind a config-file options, so that users know that one (or a specific) environment variable is in use
- Following from the previous point, if possible then a possible followup - at least for editors opening in a different location - would be to display a popup before pausing, which could indicate to the user why the screen looks the way that it does? That could show the external editor name (path?), full command used (without temp-file?) and the environment variable it was taken from.
@andersk Do you have any outstanding concerns regarding this in its current state?
I updated the code with setting from ZULIPRC in editor
key, it should be ZULIPRC > $ZULIP_EDITOR_COMMAND
> $EDITOR
read order and print the source at launching. There are linting errors, I will check later if it is because of me.
@neiljp everything should be done and clean. I updated the message on opening and add
external editor command 'hx' specified in zuliprc file.
or with new config source environment
external editor command 'hx' specified from environment.
if there is no config in zuliprc or environment the message is
external editor command '' specified from environment.
Heads up @mek-yt, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main
branch and resolve your pull request's merge conflicts accordingly.
I fix the conflicts. Don't hesitate to tell me if there are some improvements need to merge this feature.
@mek-yt Thanks for all your work on this, it's really appreciated!
I'm working on resolving some conflicts and changes necessary to integrate with other recent commits, and plan to merge soon!
@mek-yt Current changes that I will shortly push back here before merging include:
- Split of FAQ out into a second commit, with rewording & extra notes
- Commit text adjustments
- Command description, README, and error text adjustments
- Use some more descriptive variables, and some defensive checks to avoid potential errors
- Updates of informative text to better represent the code
Generally my preference would be for a more spread-out sequence of commits, eg. we don't have environment variables as configuration, so that could be a clean separate addition.
In an ideal world I'd also request more tests, but I'd prefer we get this merged and move on with this useful feature! :tada: