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.

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

"editor" command not working properly with spaces within an argument #581

Closed
IcaroBritto opened this issue Mar 25, 2019 · 13 comments · Fixed by #953
Closed

"editor" command not working properly with spaces within an argument #581

IcaroBritto opened this issue Mar 25, 2019 · 13 comments · Fixed by #953
Labels
bug Something isn't working 📌 This can't go stale

Comments

@IcaroBritto
Copy link

IcaroBritto commented Mar 25, 2019

I'm having a problem with the "editor" entry on the .jrnl_config file.
Basically what I am trying to do is start vim with the filetype defined to markdown.
The editor entry is like this:

{
...
   "editor": "nvim -c 'setlocal filetype=markdown'",
...
}

This command works fine on terminal, but this is not working on jrnl.
The problems are:

  • The filetype is not set (it is empty);
  • An error is shown when nvim opens like if the ' is being interpreted as a marker command.
  • The jrnl temp file is not set in nvim.

I've tried double quotes ("editor": "nvim -c \"setlocal filetype=markdown\"") and escaping white spaces ("editor": "nvim -c setlocal\\ filetype=markdown") but they didn't work either.

Can you help me with that?
Sorry for my English.

Nice piece of software by the way.

@maebert
Copy link
Contributor

maebert commented Apr 14, 2019

Internally, the commands to launch an editor are split on whitespaces and stored in an array, so "nvim -c 'setlocal filetype=markdown'" becomes ["nvim", "-c", "'setlocal", "filetype=markdown'"], causing the ' to be misinterpreted. There's no super elegant way to fix that unfortunately, but you try to could store 'setlocal filetype=markdown' in an environment variable and change it to something like "nvim -c $SETMD"

@IcaroBritto
Copy link
Author

Hi,
Thank you for the reply.
I did just like you said, but it didn't work :/
I think the environment variables aro not being passed when the editor command is executed.
I'm not a Python developer, but I was looking into the code and, correct if I'm wrong, looks like that the process is called at the line 134 of the util.py.
Maybe if the code was like this we could use environment variables:

...
env_copy = os.environ
subprocess.call(config['editor'].split() + [tmpfile], env=env_copy)
...

Again, I'm not a Python developer, so I'm totally not sure.
Thanks again!

@stale stale bot added the stale label Jul 7, 2019
@jrnl-org jrnl-org deleted a comment from stale bot Jul 8, 2019
@wren wren added bug Something isn't working and removed stale labels Jul 8, 2019
@wren
Copy link
Member

wren commented Jul 8, 2019

I've had similar issues. There's a PR that aims to address this bug, but I haven't gotten a chance to test it out.

@stale
Copy link

stale bot commented Aug 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Aug 7, 2019
@gregorybodnar
Copy link
Contributor

Let's keep this open. I might be able to test the PR with some contrived invocations. I'll try for the weekend.

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Aug 7, 2019
@gregorybodnar
Copy link
Contributor

I built up a test with #435 cherry-picked onto 2.0-rc3 and using this in my config:

  editor_test:
    editor: vim -c 'r! date' -f
    journal: /home/greg/editortest.journal

The PR preserves the quotes that are being passed, so it looks like this issue would be resolved. I don't have a Windows system to try with, so I'm just going to have to trust the Python docs for that case.

@wren
Copy link
Member

wren commented Aug 13, 2019

Yup, I agree that the PR looks pretty good. I want to give the original author a chance to update themselves. If they don't, though, we can look at our options. I'm not sure we can just cherry-pick their commit when we factor in licensing and stuff like that, but I'm sure we can make a PR with similar functionality (and more tests, anyway). Anyway, we can cross that bridge when we get to it.

@stale
Copy link

stale bot commented Oct 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Inactive issue: will be closed soon if no activity label Oct 12, 2019
@wren
Copy link
Member

wren commented Oct 15, 2019

I think subprocess is the right way to go, but passing shell=True is just too dangerous of an approach when the config file isn't secured. We'd basically just be executing arbitrary commands on a user's machine. Which we sort of already are, but that feels much worse.

Other than that, though. The oringinal PR looks good.

@stale stale bot removed the stale Inactive issue: will be closed soon if no activity label Oct 15, 2019
@wren
Copy link
Member

wren commented Oct 20, 2019

@gregorybodnar Looks like that PR staled out. Do you still have a chance to submit an updated PR?

@wren wren added the 📌 This can't go stale label Oct 25, 2019
@gregorybodnar
Copy link
Contributor

I have a long weekend just about to start, so I'll get it going.

@micahellison micahellison changed the title "editor" command not working properly "editor" command not working properly with spaces within an argument May 16, 2020
@wren
Copy link
Member

wren commented May 16, 2020

So, here's a thing.

On a mac:

$ python -c 'import sys; print(sys.platform)'
darwin

And this is our editor code:
subprocess.call(shlex.split(config['editor'], posix="win" not in sys.platform) + [tmpfile])

posix="win" not in sys.platform

"win"

darwin

😂

@wren
Copy link
Member

wren commented May 16, 2020

Fixing the above (change to win32) fixes all issues related to editor config values for me. I'm drafting a PR with this fix plus a few tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📌 This can't go stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants