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

Make sixel output flicker free #1943

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ func (e *setExpr) eval(app *app, args []string) {
if app.nav.height != app.ui.wins[0].h {
app.nav.height = app.ui.wins[0].h
clear(app.nav.regCache)
if gOpts.sixel {
app.ui.sxScreen.lastFile = ""
}
}
app.ui.loadFile(app, true)
}
Expand Down Expand Up @@ -350,7 +347,6 @@ func (e *setExpr) eval(app *app, args []string) {
app.ui.wins = getWins(app.ui.screen)
if gOpts.sixel {
clear(app.nav.regCache)
app.ui.sxScreen.lastFile = ""
}
app.ui.loadFile(app, true)
case "scrolloff":
Expand Down Expand Up @@ -1420,7 +1416,6 @@ func (e *callExpr) eval(app *app, args []string) {
}
if gOpts.sixel {
clear(app.nav.regCache)
app.ui.sxScreen.lastFile = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this might have to stay. I tried running the redraw command manually (or alternatively press <c-l>), and found that the sixel image got removed instead of being redrawn.

It might be good to check what happens when you try toggling other options that affect sixel images and their sizes, such as sixel/preview/ratios/drawbox.

}
for _, dir := range app.nav.dirs {
dir.boundPos(app.nav.height)
Expand Down
5 changes: 3 additions & 2 deletions misc.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,9 @@ func getFileExtension(file fs.FileInfo) string {
}

var (
reModKey = regexp.MustCompile(`<(c|s|a)-(.+)>`)
reRulerSub = regexp.MustCompile(`%[apmcsfithd]|%\{[^}]+\}`)
reModKey = regexp.MustCompile(`<(c|s|a)-(.+)>`)
reRulerSub = regexp.MustCompile(`%[apmcsfithd]|%\{[^}]+\}`)
reSixelSize = regexp.MustCompile(`"1;1;(\d+);(\d+)`)
)

var (
Expand Down
84 changes: 42 additions & 42 deletions sixel.go
Original file line number Diff line number Diff line change
@@ -1,69 +1,69 @@
package main

import (
"fmt"
"log"
"os"
"strings"
"strconv"

"github.com/gdamore/tcell/v2"
)

const (
gSixelBegin = "\033P"

// The filler character should be:
// - rarely used: the filler is used to trick tcell into redrawing, if the
// filler character appears in the user's preview, that cell might not
// be cleaned up properly
// - ideally renders as empty space: the filler alternates between bold
// and regular, using a non-space would look weird to the user.
gSixelFiller = '\u2000'
)

type sixelScreen struct {
xprev, yprev int
sixel *string
altFill bool
lastFile string // TODO maybe use hash of sixels instead to flip altFill
lastFile string
lastSxW int
lastSxH int
lastWinW int
lastWinH int
}

func (sxs *sixelScreen) fillerStyle(filePath string) tcell.Style {
if sxs.lastFile != filePath {
sxs.altFill = !sxs.altFill
}

if sxs.altFill {
return tcell.StyleDefault.Bold(true)
}
return tcell.StyleDefault
}
func (sxs *sixelScreen) unlockSixel(win *win, screen tcell.Screen, filePath string) {
if filePath != "" && (filePath != sxs.lastFile || win.w != sxs.lastWinW || win.h != sxs.lastWinH) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The check should be sxs.lastFile != "" as it represents whether there was a previous sixel image drawn or not. filePath is the path of the newly selected file, and is practically never blank.

Suggested change
if filePath != "" && (filePath != sxs.lastFile || win.w != sxs.lastWinW || win.h != sxs.lastWinH) {
if sxs.lastFile != "" && (filePath != sxs.lastFile || win.w != sxs.lastWinW || win.h != sxs.lastWinH) {

screen.LockRegion(win.x, win.y, sxs.lastSxW, sxs.lastSxH, false)

func (sxs *sixelScreen) showSixels() {
if sxs.sixel == nil {
return
}

// XXX: workaround for bug where quitting lf might leave the terminal in bold
fmt.Fprint(os.Stderr, "\033[0m")

fmt.Fprint(os.Stderr, "\0337") // Save cursor position
fmt.Fprintf(os.Stderr, "\033[%d;%dH", sxs.yprev, sxs.xprev) // Move cursor to position
fmt.Fprint(os.Stderr, *sxs.sixel) //
fmt.Fprint(os.Stderr, "\0338") // Restore cursor position
}

func (sxs *sixelScreen) printSixel(win *win, screen tcell.Screen, reg *reg) {

if reg.path == sxs.lastFile && win.w == sxs.lastWinW && win.h == sxs.lastWinH {
return
}
if reg.sixel == nil {
sxs.lastFile = ""
return
}

// HACK: fillers are used to control when tcell redraws the region where a sixel image is drawn.
// alternating between bold and regular is to clear the image before drawing a new one.
st := sxs.fillerStyle(reg.path)
for y := range win.h {
st = win.print(screen, 0, y, st, strings.Repeat(string(gSixelFiller), win.w))
tty, ok := screen.Tty()
if !ok {
log.Printf("returning underlying tty failed during sixel render")
return
}
ti, err := tcell.LookupTerminfo(os.Getenv("TERM"))
if err != nil {
log.Printf("terminal lookup failed during sixel render %s", err)
return
}
v, _ := tty.WindowSize()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think tty.WindowSize() can technically fail but it never happened during my testing. Maybe it's better to handle the error just in case.

w, h := v.CellDimensions()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use the same variable names as the sixel demo:

  • ws for the window size
  • cw for the cell width
  • ch for the cell height

matches := reSixelSize.FindStringSubmatch(*reg.sixel)
if matches == nil {
log.Printf("sixel dimensions cannot be looked up")
return
}
iw, _ := strconv.Atoi(matches[1])
ih, _ := strconv.Atoi(matches[2])

sxs.xprev, sxs.yprev = win.x+1, win.y+1
sxs.sixel = reg.sixel
// width and height are -1 to avoid showing half filled sixels
screen.LockRegion(win.x, win.y, iw/w-1, ih/h-1, true)
ti.TPuts(tty, ti.TGoto(win.x, win.y))
ti.TPuts(tty, *reg.sixel)
sxs.lastFile = reg.path
sxs.lastSxW = iw
sxs.lastSxH = ih
Comment on lines +65 to +66
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect, iw and ih are the dimensions of the image in pixels, but they are being passed directly to screen.LockRegion when unlocking afterwards, which expects the dimensions to be in cells.

So I did some more investigation on my side, and I think what happens is this:

  • Locking a region means that tcell won't be able to draw to that region
  • Unlocking a region does the opposite, but also marks the region as 'dirty' so when the screen is cleared at the start of the draw function, the sixel image will be cleared during the ui.screen.Show() call at the end. If a new sixel image is drawn in the meantime, it won't be affected as the corresponding region is locked again, but this will clear the residue from the previous image.

So I would make the following changes:

  • Use screen.LockRegion(win.x, win.y, iw/w, ih/h, true) when drawing the sixel image. I don't think the -1 is needed as this is integer division and won't exceed the size of the sixel image anyway.
  • Use screen.LockRegion(win.x, win.y, win.w, win.h, false) when clearing the sixel image. This marks the entire preview window as dirty, which is essentially what happens with the current code when using large values like iw and ih.
  • sxs.lastSxW and sxs.lastSxH can be deleted as they are no longer required.
  • I think it's fine to use rename the unlockSixel function back to clearSixel since it appears to do more than unlocking, and clearSixel is more descriptive.

sxs.lastWinW = win.w
sxs.lastWinH = win.h
}
11 changes: 4 additions & 7 deletions ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ func newUI(screen tcell.Screen) *ui {
styles: parseStyles(),
icons: parseIcons(),
currentFile: "",
sxScreen: sixelScreen{},
sxScreen: sixelScreen{"", 0, 0, 0, 0},
}

go ui.pollEvents()
Expand Down Expand Up @@ -1037,7 +1037,6 @@ func (ui *ui) draw(nav *nav) {
ui.screen.SetContent(i, j, ' ', nil, st)
}
}
ui.sxScreen.sixel = nil

ui.drawPromptLine(nav)

Expand Down Expand Up @@ -1080,8 +1079,9 @@ func (ui *ui) draw(nav *nav) {
curr, err := nav.currFile()
if err == nil {
preview := ui.wins[len(ui.wins)-1]

ui.sxScreen.unlockSixel(preview, ui.screen, curr.path)
if curr.IsDir() {
ui.sxScreen.lastFile = ""
preview.printDir(ui, ui.dirPrev, &context,
&dirStyle{colors: ui.styles, icons: ui.icons, role: Preview},
nav.previewLoading)
Expand Down Expand Up @@ -1116,10 +1116,6 @@ func (ui *ui) draw(nav *nav) {
}

ui.screen.Show()
if ui.menuBuf == nil && ui.cmdPrefix == "" && ui.sxScreen.sixel != nil {
ui.sxScreen.lastFile = ui.regPrev.path
ui.sxScreen.showSixels()
}
}

func findBinds(keys map[string]expr, prefix string) (binds map[string]expr, ok bool) {
Expand Down Expand Up @@ -1523,6 +1519,7 @@ func (ui *ui) readExpr() {
}

func (ui *ui) suspend() error {
ui.sxScreen.lastFile = ""
return ui.screen.Suspend()
}

Expand Down