Edit message in a terminal editor subprocess

[mek-yt]

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

• 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
• 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

• It is a minimal coherent idea
• It has a commit summary following the documented style (title & body)
• It has a commit summary describing the motivation and reasoning for the change
• 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

[neiljp]

@neruyzo Thanks for thinking of this feature and contributing 👍

Once I had set $EDITOR this worked for me with vim, though prior to that it simply failed with no error.

Was ctrl o the shortcut in your editor, or from something else? Selecting a new shortcut key is often challenging as it then is difficult to change later.

One issue I found is that there seems to be a blank line added after the text in the compose box, after resuming from the editor?

One followup we could discuss may be to have a header line automatically generated in the message to indicate the recipients/context, which would then be stripped out later. At an extreme we could even defer compose to an editor, though we'd lose features like validation and autocomplete that we currently have.

[mek-yt]

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.

[neiljp]

@neruyzo Thanks for improving this - I left a few more comments.

The other point that occurs for me is the blank line issue I mentioned in the previous review - do you see this too?

For example, for This text in vim, becomes

This text[]

in the compose box ([] being the cursor)

[mek-yt]

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.

[neiljp]

@neruyzo For use with terminal editors this looks fine to me 👍

I'll adjust the commit text if we merge this as-is soon, to match the style we use.

The extra line issue looks resolved, and cursor position isn't a problem 👍

Re the description, I definitely agree that Open works in terms of ctrl o, and looking at adjacent entries, we use 'message' rather than 'composition' or similar, so it's at least consistent for now :)
(it's also better in the more prominent position 👍)

I tried a little to debug the gvim issue, that it opens with no content. By the time that gvim opens with the PR right now, the file is empty/gone. If I add a sleep(20) after the open, ZT pauses and the content does enter the editor... and if (and only if) I edit and save the file within 20s does the content transfer back to ZT.

Other than these minor issues, which we can document, I do wonder if using a context manager with the NamedTemporaryFile may be cleaner/simpler - it already can be used to write to and read from a file, and is guaranteed to always clean up.

ZULIP_EDITOR_COMMAND works well enough, and we should document it, though a config file option may be easier in the long run. We can document in the README, or maybe just placing everything into a FAQ entry like 'Can I compose messages in another editor?' might work well enough. That can include how to set it up, as well as what values are known to work (or not).

Re the TEMPFILE:LINE:CHARACTER point, do you mean passing eg. /tmp/tmp12345filename:5:1 as an option? That can certainly be an option, and I'd welcome an issue being filed with anything that we don't complete here.

[neiljp]

@andersk Based on feedback on previous external-command PRs, do you have any concerns with this implementation?

[mek-yt]

Update using shlex instead of oslex

[neiljp]

@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?

[mek-yt]

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.

[mek-yt]

@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.

[mek-yt]

I fix the conflicts.
Don't hesitate to tell me if there are some improvements need to merge this feature.

[neiljp]

@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!

[neiljp]

@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! 🎉