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

Spawned shell (!) with kitty (--cwd=current) #759

Closed
Kratacoa opened this issue Oct 14, 2020 · 25 comments
Closed

Spawned shell (!) with kitty (--cwd=current) #759

Kratacoa opened this issue Oct 14, 2020 · 25 comments

Comments

@Kratacoa
Copy link

Issue

When I navigate to a directory in nnn, exit it with ! and open another window with launch --cwd=current (which is the command for kitty that allows to open a new window inside the previous directory), it opens inside home directory.
Actually, after doing such an action, even if i cd outside, it keeps opening inside the home directory.
According to kitty's author, this is because nnn doesn't set the shell as the active process for the terminal device.
Kitty uses the current directory of the active foreground child process associated with the terminal device in the active window.

Environment details (Put x in the checkbox along with the information)

[x] Operating System: Void Linux, musl version
[ ] Desktop Environment:
[x] Terminal Emulator: kitty
[x] Shell: fish
[ ] Custom desktop opener (if applicable):
[x] Program options used: aeEnx
[ ] Configuration options set:
[x] Issue exists on nnn master

@Kratacoa Kratacoa added the bug label Oct 14, 2020
@jarun
Copy link
Owner

jarun commented Oct 14, 2020

! does not exit nnn, it spawns a shell in the current dir. (In case you want to setup cd on quit, visit the quickstart section.)

We took care of the chdir() thing in cfd4a66.

Even if I spawn a shell within nnn and open thunar, I see it opening in the spawned directory:

It's possible I don't quite understand what you are talking about. In that case, leaving it open for others to pitch in.

@jarun
Copy link
Owner

jarun commented Oct 14, 2020

I also see:

2 ~/GitHub/nnn/src$ pwd
/home/vaio/GitHub/nnn/src
2 ~/GitHub/nnn/src$ echo $PWD  
/home/vaio/GitHub/nnn/src

So the current working directory is set correctly within the spawned shell. I am not sure why any other elaborate mechanism is required for any other subsequent processing to understand which location should be opened in the next window.

@jarun
Copy link
Owner

jarun commented Oct 14, 2020

I tried a similar test by opening another tab in xfce4-terminal and I see it opens in home. So I think this is the problem you wanted to mention.

@0xACE
Copy link
Collaborator

0xACE commented Oct 14, 2020

I tried a similar test by opening another tab in xfce4-terminal and I see it opens in home. So I think this is the problem you wanted to mention.

Afaik this happens because the terminal itself reports it as it's own cwd and not the process that is running in the foreground. I have a similiar situation with st and tmux, just never bothered caring for it... The author in the other thread mentioned vim working properly, though I see the same issue with vim.... alas I'm not familiar with launch --cwd=current so I can't investigate at this time...

EDIT: just looked at spawn() that function has significantly changed from what I remember it being... Don't have time to research this....

@jarun
Copy link
Owner

jarun commented Oct 14, 2020

@KlzXS @leovilok please see if this can be resolved easily (without forking and all... I guess this has to do with tcsetpgrp and tcgetpgrp).

Otherwise I am fine with not fixing it and moving on. We can't keep forking as if we have unlimited resource.

@leovilok
Copy link
Collaborator

Why not simply use --cwd=$PWD?

@leovilok
Copy link
Collaborator

I guess you're right about tcsetpgrp, but we wouldn't even need to set it to the subshell PID: setting it to the PID of nnn¹ should be enough, as we already do chdir().

¹: well after calling setpgid(getpid(), 0) to create the new process group.

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

@leovilok can you please raise the PR? I experimented with something similar and it didn't work with xfce4-terminal. Maybe I missed something.

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

Ignore the earlier comment. Tested with kitty. Works fine.

@jarun jarun closed this as completed in c074572 Oct 15, 2020
@jarun
Copy link
Owner

jarun commented Oct 15, 2020

@Kratacoa please confirm that the patch works as expected for you.

@Kratacoa
Copy link
Author

Kratacoa commented Oct 15, 2020

Nope, I'm afraid it doesn't work.
I've compiled the main branch as of the latest commit, using last release of kitty.
How can I provide some useful information?

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

The way I tested this is:

  • add the following on kitty config to map F2 to open new tab: map f2 launch --cwd=current
  • run nnn
  • navigate to a different directory
  • press !
  • press F2

@Kratacoa
Copy link
Author

Kratacoa commented Oct 15, 2020

Yeah, my setup is pretty much the same. Cannot reproduce the correct behaviour.
Although, now it does work properly when I open a window while inside nnn (by opening the window inside currently viewed directory).

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

I just tested with kitty-0.19.1 and it works for me there as well. Sorry but I can't spend more time on this.

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

My complete kitty config for reference:

allow_remote_control yes 
font_size 8.0 
map f2 launch --cwd=current

@jarun
Copy link
Owner

jarun commented Oct 15, 2020

For future reference. The equivalent in xfce4-terminal is

xfce4-terminal --tab --working-directory=$PWD

jarun added a commit that referenced this issue Oct 17, 2020
@jarun
Copy link
Owner

jarun commented Oct 17, 2020

Reopening as we had to revert the patch due to side-effects. Feel free to pitch in.

@jarun jarun reopened this Oct 17, 2020
@leovilok
Copy link
Collaborator

We might want to reset the terminal process group on exit to what it was before.

@jarun
Copy link
Owner

jarun commented Oct 18, 2020

Please try it out and please confirm with @Kratacoa.

Also, please confirm #763 is not reopened in the workflow.

@jarun
Copy link
Owner

jarun commented Oct 24, 2020

@leovilok in case you might not have noticed, please feel free to experiment with this. I did try what you suggested but it didn't work. Very much possible I made a mistake somewhere.

@leovilok
Copy link
Collaborator

@jarun I had noticed, but my interest was quite low, and I finally forgot about this. Note that I never tested anything in this issue, every suggestion I made came from the docs or other discussion I read on the web. But I'm still unsure about how/when getpgid/setpgid should be used (except for kitty). I don't know how other TUI programs use it, I've not seen source code with these functions (except kitty source).

@jarun
Copy link
Owner

jarun commented Oct 24, 2020

No problem! I will leave it open. I assumed you have more clarity on this.

@jarun
Copy link
Owner

jarun commented Dec 11, 2020

This is open for a while and I don't see anyone picking it up.

Closing the issue and adding a line item in the ToDo list for the same.

@jarun jarun closed this as completed Dec 11, 2020
@jarun jarun mentioned this issue Dec 11, 2020
21 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2021
@jarun
Copy link
Owner

jarun commented May 5, 2021

@jarun jarun changed the title nnn's ! command doesn't work properly with kitty (--cwd=current) Make spawned shell (!) work with kitty (--cwd=current) Jul 7, 2021
@jarun jarun changed the title Make spawned shell (!) work with kitty (--cwd=current) Spawned shell (!) with kitty (--cwd=current) Jul 7, 2021
@jarun
Copy link
Owner

jarun commented Oct 14, 2021

We also suggested an easy solution for this here using OSC 7 but it wasn't preferred.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants