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: empty file previews as binary #1387

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

horriblename
Copy link
Contributor

Fixes a bug where empty files are mislabeled as 'binary' in preview due to extraneous bytes being passed to the scanner

How to reproduce bug:

  1. select an empty file for preview
  2. preview shows up as "binary"

Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

I haven't figured out if there's a way to just simply peek at the first two bytes, instead of actually reading them and then joining it back using io.MultiReader, but anyway the current fix looks fine for now.

@joelim-work
Copy link
Collaborator

How to reproduce bug:

  1. select an empty file for preview
  2. preview shows up as "binary"

Also add step 0. Ensure sixel option is enabled

@joelim-work joelim-work merged commit 59dd325 into gokcehan:master Aug 14, 2023
4 checks passed
@joelim-work
Copy link
Collaborator

Hi @horriblename

I merged this patch quickly because it was a bug and may confuse users who are testing the new sixel feature.

Do you think a better way to fix this might be to wrap the output in bufio.Reader and use that to Peek at the bytes?

Something like this:

diff --git a/nav.go b/nav.go
index 56dab74..ef98966 100644
--- a/nav.go
+++ b/nav.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"bufio"
-	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -802,7 +801,7 @@ func (nav *nav) preview(path string, win *win) {
 	reg := &reg{loadTime: time.Now(), path: path}
 	defer func() { nav.regChan <- reg }()
 
-	var reader io.Reader
+	var reader *bufio.Reader
 
 	if len(gOpts.previewer) != 0 {
 		cmd := exec.Command(gOpts.previewer, path,
@@ -836,7 +835,7 @@ func (nav *nav) preview(path string, win *win) {
 			}
 		}()
 		defer out.Close()
-		reader = out
+		reader = bufio.NewReader(out)
 	} else {
 		f, err := os.Open(path)
 		if err != nil {
@@ -845,14 +844,11 @@ func (nav *nav) preview(path string, win *win) {
 		}
 
 		defer f.Close()
-		reader = f
+		reader = bufio.NewReader(f)
 	}
 
-	prefix := make([]byte, 2)
 	if gOpts.sixel {
-		_, err := reader.Read(prefix)
-		reader = io.MultiReader(bytes.NewReader(prefix), reader)
-
+		prefix, err := reader.Peek(2)
 		if err == nil && string(prefix) == gSixelBegin {
 			b, err := io.ReadAll(reader)
 			if err != nil {

I haven't tested this very much though.

@horriblename
Copy link
Contributor Author

I've thought of that but was worried that it would cause a 'double buffered' situation, i.e. reader has its buffer but then the buf := bufio.NewScanner(reader) (a bit further down) would keep another buffer. I didn't measure anything but this feels worse.

Another solution is to replace the scanner using bufio.Reader.ReadString('\n') instead but idk if we'd need to take care of DOS-format endlines \r\n

Hope this makes sense, if it doesn't, give me a few days until my exams are over, and I'll get back to you :)

@joelim-work
Copy link
Collaborator

That's OK, it was just an idea. The extra buffer in bufio.Reader probably does make it less efficient than io.MultiReader, but I haven't bothered to test to see if it makes any noticeable impact.

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