Handle cursor area with custom writer #17

lammel · 2023-06-19T22:16:55Z

With this merge request using a custom writer is now possible by using a instanced cursor area.
Moving the cursor will be restricted to the size of the area. Multiline is also supported.

Marked as draft, as tests have only been conducted under Linux and probably some discussion is needed.

MarvinJWendt · 2023-06-20T07:45:51Z

Hi, awesome PR, thanks!

I am unable to compile it under Linux currently:

I like the idea, and I think implementation wise, there is not much to discuss (looks good to me).

cursor.go

MarvinJWendt

I assume that you do not have a Windows environment? If so, what would be the most appropriate way for you to implement it for Windows? Let me know if you need any help with that :)

lammel · 2023-06-20T13:30:45Z

Hi, awesome PR, thanks!

I am unable to compile it under Linux currently:

I like the idea, and I think implementation wise, there is not much to discuss (looks good to me).

OMG! I built my application with clil based on pterm with interactive text input and multiline text-input over StdErr yesterday. Still wonder how I managed to push a broken version :)

I assume that you do not have a Windows environment? If so, what would be the most appropriate way for you to implement it for Windows? Let me know if you need any help with that :)

I have a setup for Windows 10 with Powershell running in a VM, so probably some first steps are possible.
But a little help to verify this PR would be very welcome.

MarvinJWendt · 2023-06-20T16:37:24Z

OMG! I built my application with clil based on pterm with interactive text input and multiline text-input over StdErr yesterday. Still wonder how I managed to push a broken version :)

Haha no worries ^^

I have a setup for Windows 10 with Powershell running in a VM, so probably some first steps are possible.
But a little help to verify this PR would be very welcome.

Sounds good. If you're stuck at some point, give me a ping :)

codecov · 2023-06-26T08:01:54Z

Codecov Report

Merging #17 (1286808) into main (00834e0) will increase coverage by 1.01%.
The diff coverage is 10.00%.

@@           Coverage Diff            @@
##            main     #17      +/-   ##
========================================
+ Coverage   3.84%   4.86%   +1.01%     
========================================
  Files          4       6       +2     
  Lines        130     185      +55     
========================================
+ Hits           5       9       +4     
- Misses       124     176      +52     
+ Partials       1       0       -1

Files Changed	Coverage Δ
area.go	`0.00% <0.00%> (ø)`
area_other.go	`0.00% <0.00%> (ø)`
cursor.go	`0.00% <0.00%> (-33.34%)`	⬇️
cursor_other.go	`28.57% <28.57%> (ø)`
utils.go	`13.15% <31.25%> (+13.15%)`	⬆️

MarvinJWendt

Hi, I have left some comments.

Just for reference:

We need to use the internal Writer interface everywhere, instead of io.Writer. Windows handles cursor movement via syscalls, which means we need a file descriptor. If you would do this:

func main() {
	var buffer bytes.Buffer
	a := cursor.NewArea().WithWriter(&buffer)
	a.Update("Hello World!")
}

We would get this panic on Windows:

We have to do this for every operating system, as otherwise, Linux / macOS users would be able to do the example above, which would then panic on Windows. By accepting the internal Writer interface, we make sure that there has to be a file descriptor.

The only downside to this is, that the area cannot write to a buffer, but that isn't really relevant, as there is no cursor to move in a buffer anyway.

The rest looks good to me :)

// PS:

Sorry for the delay, I was on vacation.

area.go

cursor.go

cursor_windows.go

lammel · 2023-06-27T16:13:32Z

Hi, I have left some comments.

Just for reference:

We need to use the internal Writer interface everywhere, instead of io.Writer. Windows handles cursor movement via syscalls, which means we need a file descriptor.

Done. pterm also requires a PR to make use of cursor.WithWriter which will be handled by a followup PR.

The rest looks good to me :)

Great!

Sorry for the delay, I was on vacation.

Nice, will do the same next week. So in case we haven't merged then, we might need some more days then 🆗

lammel · 2023-07-03T23:02:39Z

I ran the area tests on my virtualized Windows 10 VM with good results so far.
Behaves exactly the same on Linux and Windows.

But would be good if you give it a try. Thanks for the support here.

lammel · 2023-07-17T22:38:18Z

Just fixed the compile error on windows, caused by a missing commit.
Please let the workflow run again (PR for CI lint is also available now)

MarvinJWendt · 2023-07-18T18:00:43Z

Hi @lammel, sorry for the late response, I was on vacation.
I have implemented an important fix that caused the area to be messy on Windows. Could you please merge the changes of #20 into this PR? Thanks! (On Update Windows should use Fprint and everything else Fprintln and print line by line)

// Update overwrites the content of the Area.
func (area *Area) Update(content string) {
	oldWriter := target

	SetTarget(area.writer) // Temporary set the target to the Area's writer so we can use the cursor functions
	area.Clear()
	lines := strings.Split(content, "\n")
	fmt.Fprintln(area.writer, strings.Repeat("\n", len(lines)-1)) // This appends space if the terminal is at the bottom
	Up(len(lines))
	SetTarget(oldWriter) // Reset the target to the old writer

	// Workaround for buggy behavior on Windows
	if runtime.GOOS == "windows" {
		for _, line := range lines {
			fmt.Fprint(area.writer, line)
			StartOfLineDown(1)
		}
	} else {
		for _, line := range lines {
			fmt.Fprintln(area.writer, line)
		}
	}

	height = 0
	area.height = len(strings.Split(content, "\n"))
}

MarvinJWendt · 2023-07-19T14:25:07Z

Hi @lammel, there are some linting errors, and they (finally) are reported correctly. You can see them here: https://github.com/atomicgo/cursor/pull/17/files

Or if you have docker installed, you can use this command locally: docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.53.1 golangci-lint run.

Btw: You don't need to update the README.md files in AtomicGo packages. They are generated automatically when the PR is pushed to master 😉

lammel · 2023-07-19T22:20:12Z

Hi @lammel, there are some linting errors, and they (finally) are reported correctly. You can see them here: https://github.com/atomicgo/cursor/pull/17/files

Or if you have docker installed, you can use this command locally: docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.53.1 golangci-lint run.

I'm using a locally installed golangci-lint now.
The rules seem a little too harsh (exhaustive struct, cuddled blocks), but I've fixed those linter warnings now.

Btw: You don't need to update the README.md files in AtomicGo packages. They are generated automatically when the PR is pushed to master wink

Thanks for noting, good to know and saves some time :)

lammel · 2023-07-19T22:22:20Z

Hi @lammel, sorry for the late response, I was on vacation. I have implemented an important fix that caused the area to be messy on Windows. Could you please merge the changes of #20 into this PR? Thanks! (On Update Windows should use Fprint and everything else Fprintln and print line by line)

// Update overwrites the content of the Area.
func (area *Area) Update(content string) {
	oldWriter := target

	SetTarget(area.writer) // Temporary set the target to the Area's writer so we can use the cursor functions
	area.Clear()
	lines := strings.Split(content, "\n")
	fmt.Fprintln(area.writer, strings.Repeat("\n", len(lines)-1)) // This appends space if the terminal is at the bottom
	Up(len(lines))
	SetTarget(oldWriter) // Reset the target to the old writer

	// Workaround for buggy behavior on Windows
	if runtime.GOOS == "windows" {
		for _, line := range lines {
			fmt.Fprint(area.writer, line)
			StartOfLineDown(1)
		}
	} else {
		for _, line := range lines {
			fmt.Fprintln(area.writer, line)
		}
	}

	height = 0
	area.height = len(strings.Split(content, "\n"))
}

Actually I removed the newline for linux on purpose, as single line updates would otherwise leak into the next line, although this is not required and requires more cursor movement.
So I'd like to keep the minimal cursor movement and fix the cursor/area usage in pterm.
Or do you see compatibility issues for other software using cursor?

MarvinJWendt · 2023-07-22T22:46:24Z

I think that should work, the linux implementation didn't had any bugs. For Windows the StartOfLineDown(1) part is important. Otherwise, it would result in this:

idea64_uDtapqB6WY.mp4

Also, I couldn't reproduce the bug on Windows consistently. The select and multiselect printers were affected all the time, but I couldn't really isolate the problem. Sometimes the Windows terminal is just a little weird.

…r/area

into feature/custom-writer

lammel · 2023-07-28T07:43:23Z

I think that should work, the linux implementation didn't had any bugs. For Windows the StartOfLineDown(1) part is important. Otherwise, it would result in this:

idea64_uDtapqB6WY.mp4
Also, I couldn't reproduce the bug on Windows consistently. The select and multiselect printers were affected all the time, but I couldn't really isolate the problem. Sometimes the Windows terminal is just a little weird.

As this is a pterm test that might also cause unexpected behaviour I've added some tests under _examples here now to reproduce the issues.

I could reproduce 1 windows issue and one generic issue you were facing.
For windows the cursor package now ensures \r\n is used for area.Update() which fixes the cursor not moving to the start of the line. This is done in a platform dependent helper in area_windows.go now. Furthermore this problem will probably be resolved by future Windows updates (hopefully).

The second issue was reproduced by _example/movement/main.go which showed a jumpy behavior of the area in case content length differs (e.g. when searching in pterms interactive_select` example)

Both of these issues are resolved now. The only remaining problem seems to be moving the cursor Up() out of bounds of the area, which causes the next update to garble output.

Your tests with pterm should run a lot smoother now.

MarvinJWendt · 2023-07-28T08:26:43Z

Looks great!

Both of these issues are resolved now. The only remaining problem seems to be moving the cursor Up() out of bounds of the area, which causes the next update to garble output.

Could this be fixed by preventing to call Up() when the cursor is in line 1 of the area?

lammel · 2023-07-28T10:15:20Z

Both of these issues are resolved now. The only remaining problem seems to be moving the cursor Up() out of bounds of the area, which causes the next update to garble output.

Could this be fixed by preventing to call Up() when the cursor is in line 1 of the area?

Should be, just ran out of time yesterday, I'll look into it soon.
Once merged this should allow pterm to have a quite reliable cursor area for the interactive inputs which also handle single line inputs.

lammel · 2023-07-28T15:30:52Z

After making the linter happy again, the above issues are fixed now.

So only the pterms multiline input has issues with the custom cursor positioning,
The y-axis for cursorPosY is a little awkward, I'd suggest to correct it to 0x0 being the upper left (not lower left) of the terminal area (already that way in pterm). Any objections?

lammel · 2023-07-28T16:24:52Z

This PR is now ready to merge. All added examples here and for pterm work as expected on Linux and Windows.
For pterm please be sure to test with PR pterm/pterm#529

lammel · 2023-07-28T16:30:29Z

Ah one more note... This PR may break current behavior depending on the use of the cursor area. You might consider that when merging/releasing.

MarvinJWendt · 2023-08-01T14:46:54Z

After making the linter happy again, the above issues are fixed now.

So only the pterms multiline input has issues with the custom cursor positioning, The y-axis for cursorPosY is a little awkward, I'd suggest to correct it to 0x0 being the upper left (not lower left) of the terminal area (already that way in pterm). Any objections?

Just to clarify, you mean the position in the Area right? If so, no objections from my side :)

MarvinJWendt

LGTM! Thanks for contributing 🚀

PS: Your examples look awesome!

MarvinJWendt · 2023-08-01T15:07:55Z

Quick addition, I have invited you to this repo as collaborator (which gives you some more rights). If you want, you can accept the invitation here: https://github.com/atomicgo/cursor/invitations.

Don't feel pressured by this to do more, but take it as appreciation :)

lammel · 2023-08-01T18:50:06Z

Quick addition, I have invited you to this repo as collaborator (which gives you some more rights). If you want, you can accept the invitation here: https://github.com/atomicgo/cursor/invitations.

Don't feel pressured by this to do more, but take it as appreciation :)

Thanks! I feel a little pressured ;-)
But I'm already looking into adding missing pieces to pterm (cursor here was just a requirement to get Stderr working there), so why not help out a little more 🆗

Support fix multiline text input area cursor

83eb5c3

lammel force-pushed the feature/custom-writer branch from 15981f0 to 83eb5c3 Compare June 19, 2023 22:40

MarvinJWendt reviewed Jun 20, 2023

View reviewed changes

cursor.go Outdated Show resolved Hide resolved

MarvinJWendt reviewed Jun 20, 2023

View reviewed changes

lammel added 3 commits June 20, 2023 22:21

Fix compile error due to merge

da0dea7

Use wrapped Writer interface, refactor global cursor helpers to utils

935a25a

Update README with gomarkdoc

e4735ab

lammel requested a review from MarvinJWendt June 20, 2023 20:51

lammel added 2 commits June 21, 2023 14:29

Refactor cursor for windows

993e54d

Fix handling of cursors internal Writer interface

392d539

lammel marked this pull request as ready for review June 21, 2023 21:41

MarvinJWendt requested changes Jun 26, 2023

View reviewed changes

lammel added 3 commits June 27, 2023 17:35

Consistently use cursor.Writer interface

f37bfde

Change area update to not add unnecessary newline

b82f3e7

Fix using cursor.Writer for Windows

8418eaf

lammel mentioned this pull request Jun 27, 2023

feat: support Stderr for interactive text inputs pterm/pterm#529

Open

11 tasks

lammel requested a review from MarvinJWendt July 15, 2023 21:53

Fix bad import causing compile error on windows

05fedf2

lammel added 4 commits July 18, 2023 21:43

Support fix multiline text input area cursor

4f8caac

Use wrapped Writer interface, refactor global cursor helpers to utils

33f5e35

Update README with gomarkdoc

beabaa7

Refactor cursor for windows

4adbf51

lammel added 2 commits July 19, 2023 19:46

Fix golint warnings

3ffca3f

Fix more linter warnings

e8fa94d

lammel added 7 commits July 27, 2023 18:26

Add examples for cursor and area

25fb942

Slightly improve examples

ca3b9a0

Fix single line clear not moving back to start of line

6ede95c

Fix newline handling for windows, refactor platform support for curso…

8b6acf0

…r/area

Add example to test varying content sizes

7738fd3

Fix jumpy content in area caused by wrong cursorPosY

d5f4f59

Fix jumpy content in area caused by wrong cursorPosY

c886465

lammel force-pushed the feature/custom-writer branch from d5f4f59 to c886465 Compare July 28, 2023 00:01

Merge branch 'feature/custom-writer' of https://github.com/lammel/cursor

a864b73

into feature/custom-writer

Fix compile error on windows

a1df9ff

lammel added 3 commits July 28, 2023 14:45

Fix area up/down to respect bounds

ec37172

Final cleanup for examples

009272c

Make linter happy again

1286808

MarvinJWendt approved these changes Aug 1, 2023

View reviewed changes

MarvinJWendt merged commit 0754e36 into atomicgo:main Aug 1, 2023

Uh oh!

Handle cursor area with custom writer #17

Handle cursor area with custom writer #17

Uh oh!

Conversation

lammel commented Jun 19, 2023

Uh oh!

MarvinJWendt commented Jun 20, 2023

Uh oh!

Uh oh!

MarvinJWendt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lammel commented Jun 20, 2023

Uh oh!

MarvinJWendt commented Jun 20, 2023

Uh oh!

codecov bot commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MarvinJWendt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lammel commented Jun 27, 2023

Uh oh!

lammel commented Jul 3, 2023

Uh oh!

lammel commented Jul 17, 2023

Uh oh!

MarvinJWendt commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarvinJWendt commented Jul 19, 2023

Uh oh!

lammel commented Jul 19, 2023

Uh oh!

lammel commented Jul 19, 2023

Uh oh!

MarvinJWendt commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lammel commented Jul 28, 2023

Uh oh!

MarvinJWendt commented Jul 28, 2023

Uh oh!

lammel commented Jul 28, 2023

Uh oh!

lammel commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lammel commented Jul 28, 2023

Uh oh!

lammel commented Jul 28, 2023

Uh oh!

MarvinJWendt commented Aug 1, 2023

Uh oh!

MarvinJWendt left a comment

Choose a reason for hiding this comment

Uh oh!

MarvinJWendt commented Aug 1, 2023

Uh oh!

lammel commented Aug 1, 2023

Uh oh!

Uh oh!

MarvinJWendt left a comment •

edited

Loading

codecov bot commented Jun 26, 2023 •

edited

Loading

MarvinJWendt commented Jul 18, 2023 •

edited

Loading

MarvinJWendt commented Jul 22, 2023 •

edited

Loading

lammel commented Jul 28, 2023 •

edited

Loading