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

feat: support Stderr for interactive text inputs #529

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

Conversation

lammel
Copy link
Contributor

@lammel lammel commented Jun 27, 2023

Description

This PR adds support for handling interactive text inputs in combination with Stderr.
The work is based on PR atomicgo/cursor#17 for atomicgo/cursor which allows cursor handling on a custom writer.

This PR is in DRAFT state, created to allow referencing in the cursor PR.

Scope

All interactive text inputs are affected.

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Related Issue

To-Do Checklist

@MarvinJWendt MarvinJWendt marked this pull request as draft June 27, 2023 18:24
@MarvinJWendt MarvinJWendt changed the title DRAFT: Support Stderr for interactive text inputs Support Stderr for interactive text inputs Jun 27, 2023
@lammel lammel marked this pull request as ready for review July 28, 2023 16:31
@lammel
Copy link
Contributor Author

lammel commented Jul 28, 2023

With this PR using interactive inputs on STDERR will be possible and also required adjustments for changes with atomicgo/cursor#17 are applied.

This is ready for review now.

@MarvinJWendt
Copy link
Member

MarvinJWendt commented Aug 1, 2023

Hi @lammel, it seems like this branch doesn't build:

image

@MarvinJWendt
Copy link
Member

Oh, I guess this requires the new cursor updates.

@MarvinJWendt
Copy link
Member

Cursor received the update v0.2.0. You can update the library in this PR (and while you're at it, you can also fix the conflict :) ).

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #529 (4b454b7) into master (3466040) will decrease coverage by 0.62%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
- Coverage   82.27%   81.66%   -0.62%     
==========================================
  Files          30       30              
  Lines        3510     3539      +29     
==========================================
+ Hits         2888     2890       +2     
- Misses        564      591      +27     
  Partials       58       58              
Files Changed Coverage Δ
area_printer.go 100.00% <ø> (ø)
color.go 76.42% <ø> (-14.09%) ⬇️
interactive_confirm_printer.go 89.79% <ø> (-2.84%) ⬇️
interactive_textinput_printer.go 47.97% <ø> (-1.45%) ⬇️

@lammel
Copy link
Contributor Author

lammel commented Aug 1, 2023

Cursor received the update v0.2.0. You can update the library in this PR (and while you're at it, you can also fix the conflict :) ).

Done!
Tests using pterm textinput (even multiline) now work with Stderr too.

Should we add an example for Stderr too?
No unit tests have been added, as you stated you are in the process of rewriting those anyway.

So this is ready to merge now! (squash highly recommended)

@MarvinJWendt MarvinJWendt changed the title Support Stderr for interactive text inputs feat: support Stderr (io.Writer) for interactive text inputs Aug 3, 2023
@github-actions github-actions bot added feature and removed feature labels Aug 3, 2023
@MarvinJWendt MarvinJWendt changed the title feat: support Stderr (io.Writer) for interactive text inputs feat: support Stderr for interactive text inputs Aug 3, 2023
@github-actions github-actions bot removed the feature label Aug 3, 2023
@MarvinJWendt
Copy link
Member

MarvinJWendt commented Aug 3, 2023

Awesome!

But I am a little torn apart, that some printers require cursor.Writer and some ìo.Writer.
With #263 I introduced a component, that allows to print multiple live printers at the same time. This is done, by passing a buffer as writer to the printers. We cannot do that with cursor.Writer.

I am not sure, what the best way would be, to make the area accept io.Writer, as Windows needs more than that, to be able to change the cursor position (thus the cursor.Writer).

Maybe we could implement a cursor.Buffer like this:

type Buffer struct {
	*bytes.Buffer
}

func NewBuffer() *Buffer {
	return &Buffer{new(bytes.Buffer)}
}

func (b *Buffer) Fd() uintptr {
	return 0
}

func main() {
	buf := NewBuffer()
	res, _ := pterm.DefaultInteractiveTextInput.WithWriter(buf).Show("Input")

	pterm.Info.Println(res)
}

Then we could require cursor.Writer everywhere and make it possible for every printer to use an area (this would be breaking, tho).

We could migrate the other printers to cursor.Writer in another PR, so that this one isn't blocked.

@lammel
Copy link
Contributor Author

lammel commented Aug 3, 2023

But I am a little torn apart, that some printers require cursor.Writer and some ìo.Writer. With #263 I introduced a component, that allows to print multiple live printers at the same time. This is done, by passing a buffer as writer to the printers. We cannot do that with cursor.Writer.

The multi-progressbar example looks great!

I am not sure, what the best way would be, to make the area accept io.Writer, as Windows needs more than that, to be able to change the cursor position (thus the cursor.Writer).

Maybe we should take another look at cursor if we can support Windows without requiring syscalls and hence the Fd().
Windows Terminal supports xterm-256color mostly, not sure about the old Windows command prompt though.
There were probably reasons why it's implemented that way now.

We might take a closer look at https://github.com/gdamore/tcell which supports Windows by using a pure Go terminfo implementation with reasonable performance and good compatibility.
It might also be possible to use tcell instead of cursor, not sure about your thoughts on that.

This would allow to stick with a simple io.Writer then.

Maybe we could implement a cursor.Buffer like this:

type Buffer struct {
	*bytes.Buffer
}

func NewBuffer() *Buffer {
	return &Buffer{new(bytes.Buffer)}
}

func (b *Buffer) Fd() uintptr {
	return 0
}

func main() {
	buf := NewBuffer()
	res, _ := pterm.DefaultInteractiveTextInput.WithWriter(buf).Show("Input")

	pterm.Info.Println(res)
}

Then we could require cursor.Writer everywhere and make it possible for every printer to use an area (this would be breaking, tho).

So breaking for all code that is using WithWriter actually, which is quite new at least so should not affect all users. But still a breaking change.

We could migrate the other printers to cursor.Writer in another PR, so that this one isn't blocked.

Let's see where the discussion heads and continue then.

@MarvinJWendt
Copy link
Member

Hi, sorry for the delay..

Maybe we should take another look at cursor if we can support Windows without requiring syscalls and hence the Fd().
Windows Terminal supports xterm-256color mostly, not sure about the old Windows command prompt though.
There were probably reasons why it's implemented that way now.

Sadly we cannot switch to a pure io.Writer implementation in cursor. Older Windows terminals / shells, like cmd (which is the default < Win 11), don't support ANSI codes.

Also, we want to make PTerm as compatible as possible, there are many great TUI libraries out there, but we found PTerm to be one of the most cross-platform ones.

I think the custom buffer idea might not be that bad.

Also I worked on #544, which will most likely be merged today. It currently uses io.Writer, but we could change that to cursor.Writer too in the future.

But I am open to different approaches, maybe we can stick to io.Writer in PTerm and convert it to cursor.Writer internally before we pass it to cursor. Something like:

if .Fd() exists
	cast to cursor.Writer
	pass to cursor lib
else
	create internal buffer with empty .Fd() functions
	pipe io.Writer to internal buffer
	pass internal buffer to cursor

@lammel
Copy link
Contributor Author

lammel commented Sep 20, 2023

Guess we have to decide on the approach first before I can continue here.

@k-poppe
Copy link

k-poppe commented Dec 30, 2023

Hi @lammel, is there any update for this feature? It's really sad the progress bar works only with one io.Writer. If you create a pterm.DefaultProgressbar.WithWriter(os.Stderr) and print anything to Stdout using pterm, it will break all layouts and vice versa...

@MarvinJWendt
Copy link
Member

Hi @k-poppe, I don't think @lammel can really continue on this at the moment. He already did a great job, but I do have to make a design decision that I didn't have time to do, yet. I want to continue with PTerm in the next year, as my daily work is now much more organized than the last couple of months. Sorry for the long wait!

@lammel
Copy link
Contributor Author

lammel commented Dec 31, 2023

Sorry for the long wait!

Don't worry, I pretty much ran out of any remaining spare time in the last couple of months too.
So let's get back to this in the next year. See you in 2024, happy new year!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants