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.

Sign up for GitHub

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

Jump to bottom

Various screen redraw fixes for wide characters, narrow screens etc. #202

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Various screen redraw fixes for wide characters, narrow screens etc. #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9af567
Various screen redraw fixes for wide characters, narrow screens etc.
tpodowd May 16, 2022
586d8ee
Fix simple race condition related to offset calculations introduced.
tpodowd Feb 28, 2023
62ab2cf
Fix race condition between writing and prompting.
tpodowd Mar 3, 2023
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
4 changes: 3 additions & 1 deletion ansi_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (a *ANSIWriterCtx) ioloopEscSeq(w *bufio.Writer, r rune, argptr *[]string)
arg := *argptr
var err error

if r >= 'A' && r <= 'D' {
if (r >= 'A' && r <= 'D') || r == 'G' {
count := short(GetInt(arg, 1))
info, err := GetConsoleScreenBufferInfo()
if err != nil {
Expand All @@ -137,6 +137,8 @@ func (a *ANSIWriterCtx) ioloopEscSeq(w *bufio.Writer, r rune, argptr *[]string)
info.dwCursorPosition.x += count
case 'D': // left
info.dwCursorPosition.x -= count
case 'G': // Absolute horizontal position
info.dwCursorPosition.x = count - 1 // windows origin is 0, unix is 1
}
SetConsoleCursorPosition(&info.dwCursorPosition)
return false
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go 1.15
require (
github.com/chzyer/test v1.0.0
golang.org/x/sys v0.0.0-20220310020820-b874c991c1a5
golang.org/x/text v0.3.7
)

require github.com/chzyer/logex v1.2.1
3 changes: 3 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ github.com/chzyer/test v1.0.0 h1:p3BQDXSxOhOG0P9z6/hGnII4LGiEPOYBhs8asl/fC04=
github.com/chzyer/test v1.0.0/go.mod h1:2JlltgoNkt4TW/z9V/IzDdFaMTM2JPIi26O1pF38GC8=
golang.org/x/sys v0.0.0-20220310020820-b874c991c1a5 h1:y/woIyUBFbpQGKS0u1aHF/40WUDnek3fPOyD08H5Vng=
golang.org/x/sys v0.0.0-20220310020820-b874c991c1a5/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
74 changes: 59 additions & 15 deletions operation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ type Operation struct {
errchan chan error
w io.Writer

isPrompting bool // true when prompt written and waiting for input

history *opHistory
*opSearch
*opCompleter
Expand All @@ -39,29 +41,43 @@ func (o *Operation) SetBuffer(what string) {
}

type wrapWriter struct {
r *Operation
t *Terminal
o *Operation
target io.Writer
}

func (w *wrapWriter) Write(b []byte) (int, error) {
if !w.t.IsReading() {
return w.target.Write(b)
return w.o.write(w.target, b)
}

func (o *Operation) write(target io.Writer, b []byte) (int, error) {
o.m.Lock()
defer o.m.Unlock()

if !o.isPrompting {
return target.Write(b)
}

var (
n int
err error
)
w.r.buf.Refresh(func() {
n, err = w.target.Write(b)
o.buf.Refresh(func() {
n, err = target.Write(b)
// Adjust the prompt start position by b
rout := runes.ColorFilter([]rune(string(b[:])))
sp := SplitByLine(rout, []rune{}, o.buf.ppos, o.buf.width, 1)
if len(sp) > 1 {
o.buf.ppos = len(sp[len(sp)-1])
} else {
o.buf.ppos += len(rout)
}
})

if w.r.IsSearchMode() {
w.r.SearchRefresh(-1)
if o.IsSearchMode() {
o.SearchRefresh(-1)
}
if w.r.IsInCompleteMode() {
w.r.CompleteRefresh()
if o.IsInCompleteMode() {
o.CompleteRefresh()
}
return n, err
}
Expand Down Expand Up @@ -234,6 +250,7 @@ func (o *Operation) ioloop() {
o.Refresh()
case CharCtrlL:
ClearScreen(o.w)
o.buf.SetOffset("1;1")
o.Refresh()
case MetaBackspace, CharCtrlW:
o.buf.BackEscapeWord()
Expand Down Expand Up @@ -351,7 +368,7 @@ func (o *Operation) ioloop() {
} else if o.IsInCompleteMode() {
if !keepInCompleteMode {
o.ExitCompleteMode(false)
o.Refresh()
o.refresh()
} else {
o.buf.Refresh(nil)
o.CompleteRefresh()
Expand All @@ -366,11 +383,11 @@ func (o *Operation) ioloop() {
}

func (o *Operation) Stderr() io.Writer {
return &wrapWriter{target: o.GetConfig().Stderr, r: o, t: o.t}
return &wrapWriter{target: o.GetConfig().Stderr, o: o}
}

func (o *Operation) Stdout() io.Writer {
return &wrapWriter{target: o.GetConfig().Stdout, r: o, t: o.t}
return &wrapWriter{target: o.GetConfig().Stdout, o: o}
}

func (o *Operation) String() (string, error) {
Expand All @@ -387,8 +404,29 @@ func (o *Operation) Runes() ([]rune, error) {
listener.OnChange(nil, 0, 0)
}

o.buf.Refresh(nil) // print prompt
// Before writing the prompt and starting to read, get a lock
// so we don't race with wrapWriter trying to write and refresh.
o.m.Lock()
o.isPrompting = true

// Query cursor position before printing the prompt as there
// maybe existing text on the same line that ideally we don't
// want to overwrite and cause prompt to jump left. Note that
// this is not perfect but works the majority of the time.
o.buf.getAndSetOffset(o.t)
Comment on lines +412 to +416
Copy link

Choose a reason for hiding this comment

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

This aspect of the change seems to cause a race condition in my application during asynchronous Close(), where sometimes the response to this DSR query is not read correctly by the library and ends up printed on the screen. It looks like ^[[70;1R or similar.

I'll look further into mitigations from my side, just wanted to note this so I don't forget about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This terminal will be asked for the current offset everytime you call Readline() to ask for text. Normally, since you call readline, input will be taking place so bytes will be read and readline will swallow this offset information from the terminal. However, if you call readline() and then close() in other goroutine (or exit the program abruptly) for example, it can happen that readline won't have time to read it and instead the program will exit and the next thing to read it will be your shell which will just output it. It doesn't happen in my use case because I'm not making exit decisions when readline is waiting for text. However, I can see in your example test case, it might have that issue.

It might be possible for the library to handle this better. Maybe in another PR if this is merged 😉

o.buf.Print() // print prompt & buffer contents
o.t.KickRead()

// Prompt written safely, unlock until read completes and then
// lock again to unset.
o.m.Unlock()
defer func() {
o.m.Lock()
o.isPrompting = false
o.buf.SetOffset("1;1")
o.m.Unlock()
}()

select {
case r := <-o.outchan:
return r, nil
Expand Down Expand Up @@ -501,7 +539,13 @@ func (o *Operation) SaveHistory(content string) error {
}

func (o *Operation) Refresh() {
if o.t.IsReading() {
o.m.Lock()
defer o.m.Unlock()
o.refresh()
}

func (o *Operation) refresh() {
if o.isPrompting {
o.buf.Refresh(nil)
}
}
Expand Down
Loading