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

Avoid refreshing previews for async shell commands #1164

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Avoid refreshing previews for async shell commands #1164

merged 1 commit into from
Mar 20, 2023

Conversation

joelim-work
Copy link
Collaborator

This pull request addresses a user complaint where previews are redrawn when opening a file, see #414 (comment) for more details.

I think this issue probably extends to running any other asynchronous shell commands which could take a while to complete (e.g. tar), and when this happens there is no point refreshing the preview immediately because the command would not have completed yet.

@gokcehan
Copy link
Owner

@joelim-work Thanks a lot for the patch. Since I need to describe this change in the changelog soon, could you provide me the proper context for this in a simple manner? I wasn't able to follow all the changes and discussions about this. From what I understand:

  • There was a change made in the on-select feature patch that caused an issue in image preview scripts (released in r28).
  • A fix for this issue made it into the master branch but that fix introduced another flickering issue for async commands (not yet released).
  • This patch fixes the flickering issue for async commands (not yet released).

Does that sound right?

@gokcehan gokcehan merged commit 8652707 into gokcehan:master Mar 20, 2023
@joelim-work joelim-work deleted the async-shell-avoid-refresh branch March 21, 2023 03:05
@joelim-work
Copy link
Collaborator Author

Hi @gokcehan, I agree that the conversation in #414 is a bit hard to follow because there are actually two issues being described:

  • The loading... message caused whenever a preview needs to be redrawn
  • Whether or not a preview needs to be redrawn when a file is opened

This pull request addresses only the second issue.

When a file is opened, the app.runShell function is called, which in turn calls ui.loadFile, causing the preview to be redrawn. Here is a timeline of the behaviour:

  • In r27, the preview will be redrawn when a file is opened (or when any other shell command is called). This behaviour is undesirable, as it causes the preview to flicker.
  • In r28, commit e10ef91 introduced a bug where the preview would not be redrawn if ui.loadFile was called and the current file didn't change. This happens to 'fix' the issue, because opening a file is an action which doesn't change the current file, but also causes other problems, like the preview not refreshing when the terminal size changes.
  • The commit 4a443ce which I added reverts the bug introduced e10ef91, which reintroduces the issue, prompting the discussion starting in Regression causing flashing lf with each selection #1133 and continuing in Flickering "loading…​" #414.
  • Finally, the issue is fixed in this pull request.

For the purposes of the changelog, I suppose you can just say that the preview is no longer redrawn if an async shell command is called.

@ilyagr
Copy link
Collaborator

ilyagr commented Mar 26, 2023

I'm not 100% sure about this, but I was thinking that it might be better to move the refresh to when the command finishes, in the goroutine at

lf/app.go

Lines 612 to 617 in 6ffc4a4

case "&":
go func() {
if err := cmd.Wait(); err != nil {
log.Printf("running shell: %s", err)
}
}()

Let me know if you have any thoughts about this. I'll think if I can come up with an example where it's useful.

@joelim-work
Copy link
Collaborator Author

joelim-work commented Mar 26, 2023

Hi @ilyagr, the original intention of this pull request was to avoid immediately refreshing the preview when launching an async command, but now that I think about it, it's also quite reasonable to refresh it when the async command actually finishes.

I did notice that running a % command causes a refresh after it's done, so I think we can just do the same for async as well?

--- a/app.go
+++ b/app.go
@@ -614,6 +614,7 @@ func (app *app) runShell(s string, args []string, prefix string) cleanFunc {
                        if err := cmd.Wait(); err != nil {
                                log.Printf("running shell: %s", err)
                        }
+                       app.ui.exprChan <- &callExpr{"load", nil, 1}
                }()
        case "$|":
                return func() {

Also please give me any example use cases you have, they are very important for testing purposes and PR descriptions.

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.

3 participants