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

Fix previewer race condition #850

Closed
wants to merge 1 commit into from
Closed

Conversation

laktak
Copy link
Contributor

@laktak laktak commented Jun 1, 2022

This PR removes app.ui.loadFile() when invoking a shell request.

I have & scripts to turn the previewer on and off. Without this change the previewer will start again because it was invoked by loadFile before the script had a chance to turn it off. (I'm using this to launch and terminate an overlay GUI app to preview PDF documents and images.)

I've run this code for over a month and noticed no side effects.

@laktak laktak mentioned this pull request Jul 4, 2022
@@ -526,7 +526,8 @@ func (app *app) runShell(s string, args []string, prefix string) {
anyKey()
}

app.ui.loadFile(app.nav, true)
// do not call loadFile because of race condition with previewer
// app.ui.loadFile(app.nav, true)
Copy link
Collaborator

@ilyagr ilyagr Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this as a puzzle, and here's the problem this creates.

TLDR: I think the best solution may be to move this line to before line 494 (in the defer that happens for $ and !), unless there's something else that I missed. It may be worth it to put a comment as well about what this is for.

To see the problem, execute $touch test and move the cursor to the new test file in lf. Then, execute $echo Test >> test. In the release version of lf, the preview will be updated immediately. With your PR, it won't. Same thing will happen if you edit the file with $vim test.

I don't think there's much point to doing this refresh if the command is launched with % or |, though, here, before that command finishes. That also causes the bug you were trying to fix. So, moving this refresh to the codepath for $ and ! seems to me like it will solve both problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stopped using the previewer in favor of #864 and #865 (with https://github.com/laktak/termplug ... though I need to add lf and Linux to the readme sometimes).

After reading your comments I think this is not the right approach. A better solution might check if the previewed file was updated and only then run the previewer again.

I'm going to close this. Thanks for your help!

@ilyagr
Copy link
Collaborator

ilyagr commented Sep 3, 2022

BTW, the reason I'm picking on your PRs is that I'm actively using them. Thank you for making them!

@laktak laktak closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants