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

preview-tui: error to foreground #970

Merged
merged 2 commits into from
Apr 27, 2021
Merged

preview-tui: error to foreground #970

merged 2 commits into from
Apr 27, 2021

Conversation

luukvbaal
Copy link
Collaborator

Bring NNN_FIFO error to foreground and printf instead of echo

@luukvbaal
Copy link
Collaborator Author

The changes from #966 require the listen_on config option or --listen-on flag for kitty previews.

This option relegates the kitty remote control to a socket instead of terminal escape sequences which don't get caught properly when sent from a background process for some reason.

If anyone has any insight as to why this might be the case and if it can be fixed, I would be interested to know. For now however, suggesting the use of the listen_on config seems fine to me.

@luukvbaal
Copy link
Collaborator Author

luukvbaal commented Apr 27, 2021

If anyone has any insight as to why this might be the case and if it can be fixed, I would be interested to know. For now however, suggesting the use of the listen_on config seems fine to me.

I would say it's caused by the race condition for /dev/tty kovidgoyal mentions in in kovidgoyal/kitty#2658 (comment).
Suggesting the use of listen_on seems the best way forward in my opinion.

@jarun jarun merged commit d998943 into jarun:master Apr 27, 2021
@jarun
Copy link
Owner

jarun commented Apr 27, 2021

Thank you!

@jarun
Copy link
Owner

jarun commented Apr 27, 2021

@luukvbaal can you please add the kitty specific notes in the ToDo list in brief? We need to put it in the release notes for users who are on kitty.

@jarun jarun mentioned this pull request Apr 27, 2021
36 tasks
@jarun
Copy link
Owner

jarun commented Apr 27, 2021

I added it. Also, in my experience previews are pretty smooth now with tmux.

@luukvbaal
Copy link
Collaborator Author

@luukvbaal can you please add the kitty specific notes in the ToDo list in brief? We need to put it in the release notes for users who are on kitty.

Kitty users will also be greeted with an error about the listen_on config but having it in the changelog is good. Thanks for adding it.

I added it. Also, in my experience previews are pretty smooth now with tmux.

Yeah I myself use tmux previews as well and it really feels as if its part of nnn with the recent changes in my opinion!

@jarun
Copy link
Owner

jarun commented Apr 27, 2021

I see an issue with preview-tui-ext... when I hover on a directory, the previewer shows all entries in that dir recursively. All defaults. If the dir has a subdir with too many entries, I don't get to see the other entries in the subdir.

@luukvbaal
Copy link
Collaborator Author

luukvbaal commented Apr 27, 2021

Yeah I don't see the point of it either but this is the tree output that has always been default in preview-tui as far as I can tell. Except the -i flag is left there by mistake from when I used tree for iconlookup preview. #972 restores the old default.

We can discuss a new default I guess, a non-recursive preview of the hovered directory makes the most sense to me(the current iconlookup view) but I think @leovilok suggested the paged tree view be kept default since it was a requested feature.

@jarun jarun mentioned this pull request May 19, 2021
50 tasks
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants