Guarantee preview/previewClear ordering #537
Conversation
|
Actually, it's a bit racey, and on occasion it will preview and then immediately clear the currently selected file. I'm marking this as a draft for now. |
|
Alright, I rewrote this PR such that instead of using two syncing channels, I factored the preview handling into a separate loop which reads from a channel. Preview handling and clearing are effectively handling in one thread now. The |
nav.go
Outdated
| win := ui.wins[len(ui.wins)-1] | ||
| nav.previewGen(p, win) | ||
| path = p | ||
| } else if len(gOpts.cleaner) != 0 && len(gOpts.cleaner) != 0 && nav.volatilePreview { |
There was a problem hiding this comment.
Is it the same conditional twice?
There was a problem hiding this comment.
Whoops, I meant to put gOpts.previewer for the first one. Fixed.
| nav.previewGen(p, win) | ||
| path = p | ||
| } else if len(gOpts.cleaner) != 0 && len(gOpts.cleaner) != 0 && nav.volatilePreview { | ||
| cmd := exec.Command(gOpts.cleaner, path) |
There was a problem hiding this comment.
It shouldn't be, since nav.volatilePreview is part of the condition of the second branch, and that variable only gets set during calls to previewGen (which is is only called within previewLoop). So, the second branch of that if statement will only be evaluated after iterations where previewGen is called and the path is set.
nav.go
Outdated
| func (nav *nav) previewClear() { | ||
| if len(gOpts.cleaner) != 0 && nav.volatilePreview { | ||
| nav.volatilePreview = false | ||
| func (nav *nav) preview(path string) { |
There was a problem hiding this comment.
Maybe get rid of single line function here?
nav.go
Outdated
| log.Printf("cleaning preview: %s", err) | ||
| } | ||
| } | ||
| func (nav *nav) previewClear() { |
There was a problem hiding this comment.
Same as above, putting it inline can make it easier to follow I think.
nav.go
Outdated
| } | ||
| } | ||
|
|
||
| func (nav *nav) previewGen(path string, win *win) { |
There was a problem hiding this comment.
If you decide to get rid of the single line functions, maybe you can rename this back to preview.
| @@ -25,7 +25,7 @@ type app struct { | |||
| ui *ui | |||
| nav *nav | |||
| ticker *time.Ticker | |||
| quitChan chan bool | |||
| quitChan chan struct{} | |||
There was a problem hiding this comment.
What is the purpose of this change?
There was a problem hiding this comment.
I was originally running preview and previewClear in their own respective threads and synchronizing them through two chan struct{} variables. I changed all the instances of chan bool to be consistent with this. Using chan struct{} is idiomatic Go and it doesn't cause an allocation (since struct{} has a size of 0) whereas a chan bool will.
There was a problem hiding this comment.
@neeshy I have tried the patch and I did not experience any issues. I'm not testing this with image previews myself as I'm not very interested in it. We can merge this and let other users report issues. I have marked a few minor points in the patch. I think we can merge it when you are ready.
|
@neeshy It seems good, thank you. |
This is a followup to the discussion in #531 (see: #531 (comment))
I have successfully been able to produce a good ordering on calls to the preview and cleaner scripts. No matter how much delay I add to either script, the order always looks like this in my testing:
This solution also only blocks threads responsible for waiting on the previewer script and executing the cleaner script, so these changes don't block the UI itself.
I've also included a commit which passes the file path to the cleaner script.