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

allow custom input/output and handle eof for os.input properly #39

Merged
merged 2 commits into from
Dec 30, 2020

Conversation

chris-rock
Copy link
Contributor

This PR allows the user to set a custom input and output. In addition it handles the case where the input returns an io.EOF error. There are cases eg. piping where os.Stdin may be empty when bubbletea starts.

An alternative (or even additional) implementation would be to allow rendering without reading from the input at all.

Signed-off-by: Christoph Hartmann chris@lollyrock.com

@chris-rock chris-rock force-pushed the custom-input branch 2 times, most recently from f6fb5c1 to 7108b24 Compare December 27, 2020 15:42
@meowgorithm
Copy link
Member

meowgorithm commented Dec 28, 2020

Thanks for the PR. This was something we've been meaning to do, so it's nice to see it happening. The functional arguments are a nice touch.

I believe we also need to check and see if output is a TTY before entering raw mode, hiding the cursor and so on (see tty.go and tty_windows.go). We typically use mattn/go-isatty for this. If that sounds right to you, do you mind making that update?

I'm also open for disabling input completely as an additional implementation, if that's something you're interested in doing.

Just curious, what's your personal use case with this?

@chris-rock
Copy link
Contributor Author

chris-rock commented Dec 28, 2020

That makes a lot of sense, from your description I see the following additions:

  • validate output for tty to ensure it working with raw mode
  • change options to allow error handling
  • allow disabling the input via separate option (default should stay active)

My main use case is command piping: cat config.json | command. I detect the pipe and read the config from stdin before bbubbletea is even initialized. Therefore bubbletea runs into EOF panic errors. I mainly use bubbletea to render progress bar and data. At this point I do not need interactive input.

For additional error handling we have the following options:

1.) use type ProgramOption func(*Program) error. The additional error handling for constructor values would be a breaking change, since we would need to change the method signature from:func NewProgram(model Model, opts ...ProgramOption) (*Program) to: func NewProgram(model Model, opts ...ProgramOption) (*Program, error). It also has the disadvantage that the nice short syntax would not be possible:

if err := tea.NewProgram(example{progress: progress}).Start(); err != nil {
  fmt.Println("Oh no!", err)
  os.Exit(1)
}

2.) func WithOutput(output *os.File) (ProgramOption,error). This has the disadvantage that you cannot easily pass the option as argument since the user needs additional error handling.

3.) We could apply all options when the user calls Start(). The pro is that its easy to write, not breaking. The disadvantage is that we defer error handling.

I am going with option 3, since the error handling defer is minimal and its easier to write and read.

@chris-rock chris-rock changed the title allow custom input andn handle eof for os.input properly allow custom input and handle eof for os.input properly Dec 28, 2020
@meowgorithm
Copy link
Member

Thanks for the quick, thoughtful updates — and sorry, I could have been clearer. Rather than erroring out if output's not a terminal I believe we can simply skip over initTerminal and restoreTerminal and carry on as normal. There are some cases where Bubble Tea can still be useful without a TTY.

I can make the change if you'd like. Either way, it would mean reverting the error handling in your last couple revisions. Let me know.

@chris-rock
Copy link
Contributor Author

Oh even better, this would make the implementation also more straight-forward. I'll update the PR.

@chris-rock chris-rock force-pushed the custom-input branch 2 times, most recently from d1124ac to 1e21f42 Compare December 29, 2020 08:23
@chris-rock chris-rock changed the title allow custom input and handle eof for os.input properly allow custom input/output and handle eof for os.input properly Dec 29, 2020
@chris-rock
Copy link
Contributor Author

I updated the PR, I hope this matches your thinking.

@meowgorithm
Copy link
Member

This looks great. Thanks for your work on this one. I'm going to test a little as well as add some comments and make a few minor stylistic changes before merging, but I'll aim to get this one in quickly.

@meowgorithm
Copy link
Member

meowgorithm commented Dec 30, 2020

I'm playing with this PR a bit and I'm wondering if it's wise to disable input for your personal use case because users can't abort the application at will. If the progress bar took a long time to finish for whatever reason the user wouldn't be able exit it. Bubble Tea doesn't listen for SIGINT so ctrl+c doesn't exit by default.

For cases where you're piping something in (cat file | command) you could can enable it with something like the following, courtesy this new PR:

tty, err := os.Open("/dev/tty")
defer tty.Close()
if err != nil {
	fmt.Println("could not open tty:", err)
	os.Exit(1)
}

if err := tea.NewProgram(model{}, tea.WithInput(tty)).Start(); err != nil {
	fmt.Println("could not run program:", err)
	os.Exit(1)
}

So now I'm wondering if we should be doing this for input by default so that in cases where pipes and redirection are present keyboard and mouse input just works as expected.


On the other hand, after talking to my colleagues about it a bit, it probably makes more sense to simply add an option to listen for SIGINT and cleanup appropriately.

@chris-rock
Copy link
Contributor Author

Thank you @meowgorithm for the great additions. I like the new interface to configure input/output. I think we should remove the sugar option func WithInputDisabled() ProgramOption and call this iteration complete. Since input can still be set to nil, we have two options:

1.) err when Start is called
2.) keep the handling and just do not read, similar to EOF (as implemented right now)

I agree that there is a feeling that we may want to add an option for tty. At least I am not 100% sure how to write the default behavior that it makes sense for most users/command line tools. Right now, its very explicit and bubbletea stays very flexible.

There are three cases that come to my mind:

  • standard command
  • input pipe: cat file.json | command
  • output pipe: command | more

In various cases it would be interesting to pass in options (eg. tty detected) to the model to render accordingly. A use case that I have in mind is to write different renderer for user input and piping. I am not sure yet if I want this to be done magic in the model or before I pass the model to bubbletea.

@meowgorithm
Copy link
Member

meowgorithm commented Dec 30, 2020

Good call about removing WithInputDisabled. For now, at least let's keep your implementation of simply not reading if we get an EOF. I'll make that update and then merge this PR.

I also agree we leave the TTY stuff in the user's hands for now, per the example. However, I will add a WithInterrupt option in a near-future release that will listen for SIGINT and cleanup before exiting accordingly.

And yeah, to your last point, @muesli and I were talking about something along the lines of switching renderers if output isn't a TTY so that Bubble Tea programs could work as a daemon/non-TUI as well as a TUI. In that case we could probably disable the renderer altogether and let the user simply print to stdout. That's just off the top of my head though.

I'm thinking we'd probably pass a special message to Update on first run, similar to the way tea.WindowSizeMsg works. That way we wouldn't have to make any assumptions or place any restrictions on the model type. Open to suggestions of course, though.

And thank you again for the PR!

@meowgorithm
Copy link
Member

@chris-rock just a small note that we're now listening for SIGINT in master, so if input is not a terminal users can press ctrl-c to quit.

We're planning on doing a release in a few days, which will contain the stuff in this PR, the SIGINT behavior, and some other small but helpful improvements.

@chris-rock
Copy link
Contributor Author

That is awesome @meowgorithm Thank you

@meowgorithm
Copy link
Member

Hey there, @chris-rock. Just a note that we've merged a PR that automatically opens a TTY for input if input is not a TTY, which means if you pipe or redirect data into a Bubble Tea application input will "just work". This works on both unix and Windows systems in well-researched, cross-platform way.

All that said, if you explicitly set a custom input (per this excellent PR) Bubble Tea will respect that input and not open a TTY.

@chris-rock
Copy link
Contributor Author

@meowgorithm that is very cool. You guys are great.

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