search

peterh / liner

Pure Go line editor with history, inspired by linenoise
MIT License
1.05k stars 132 forks source link

Renamed termios type to TerminalMode. Added GetTerminalMode, GetOriginal... #15

Closed michaelmacinnis closed 10 years ago

michaelmacinnis commented 10 years ago

Hi Peter,

I exposed the liner.termios type and renamed it TerminalMode. I added a TerminalSupported function and GetTerminalMode/SetTerminalMode methods. I also added a GetOriginalMode method.

GetTerminalMode returns a boolean status in addition to the TerminalMode. This is because it was being checked in input_windows.go. To maintain the current behavior the status is currently ignored in input.go. For symmetry, SetTerminalMode also returns a boolean status (currently it always returns true in input_windows.go). Again to maintain the current behavior the status is ignored in both input.go and input_windows.go.

Let me know what you think of these changes.

Thanks,

Michael.

peterh commented 10 years ago

Thanks for the patch.

Are you sure you want to expose get/set mode functions? I'm sure I don't want to expose all the terminal mode bits, so the only thing a user can do is pass us back a mode from CurrentMode/OriginalMode. I'm not sure if we want to include an unexported bool in the structure so we can detect and prevent SetTerminalMode of a zero value. Either way, get/set seems rather more complex than, for example, a single reopen function. Reopen seems more restrictive, but if we can only have two terminal states anyway, three functions is too many. See https://github.com/peterh/liner/tree/reopen , which needs testing and comments, but it should give you the general idea. Or in the alternative, GrabTerminal(false); GrabTerminal(true) instead of Close(); Reopen() or the three get/set functions. What are your thoughts on this?

There are a few other rough edges that need to be smoothed before a pull:

michaelmacinnis commented 10 years ago

I should have made it clear that my pull request was a sketch (much like your ReOpen sketch). The Windows changes should be considered even more of a sketch and the method/type names should be considered placeholders - I am fine using whatever names you feel fit with your project.

I would like to expose get/set mode methods. I need to launch/suspend other programs. Those programs can (and will) make changes to the terminal mode. I need to be able to save the terminals current mode when suspending a program and restore that saved mode when resuming. I need the terminal to start in cooked mode when launching a new program (I cheat and assume that the original mode was cooked) and I need the terminal in the mode required by liner before issuing a prompt. These are very simple syscall wrappers but since you are already making these syscalls in other parts of your library I thought it would make sense for these methods to be part of your library.

TerminalSupported was included because it was such a trivial change. Feel free to push it or wait until the get/set mode changes are sorted.

If you feel that these changes pull your library in a direction you do not intend it to go, just let me know.

On Tuesday, May 20, 2014, Peter Harris notifications@github.com<javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Thanks for the patch.

Are you sure you want to expose get/set mode functions? I'm sure I don't want to expose all the terminal mode bits, so the only thing a user can do is pass us back a mode from CurrentMode/OriginalMode. I'm not sure if we want to include an unexported bool in the structure so we can detect and prevent SetTerminalMode of a zero value. Either way, get/set seems rather more complex than, for example, a single reopen function. Reopen seems more restrictive, but if we can only have two terminal states anyway, three functions is too many. See https://github.com/peterh/liner/tree/reopen , which needs testing and comments, but it should give you the general idea. Or in the alternative, GrabTerminal(false); GrabTerminal(true) instead of Close(); Reopen() or the three get/set functions. What are your thoughts on this?

There are a few other rough edges that need to be smoothed before a pull:

  • Please only one concept per commit. TerminalSupported is a separate concept. If 7d43897https://github.com/peterh/liner/commit/7d43897c17f4e3234864d008ba04c358710ac7a5is acceptable to you, I can push it now, before we decide on get/set vs reopen.
  • If you do want to expose get/set mode functions, they should return error, not bool. (See http://golang.org/pkg/syscall/#Errno )
  • Git convention is to have a short summary as the first line of the commit message, a blank line, and then a paragraph describing the change. The paragraph should be word wrapped to something sensible like 78 characters. Please follow this convention.
  • Every exported type and function needs a description. I presume you were going to write these after we discussed the above. https://github.com/golang/lint can help find the ones you missed.
  • Go convention is to omit the "Get" on getter functions.
  • There are a number of typos and omissions that break compiling on non-Linux platforms. It only takes about 10 minutes to install a complete set of cross compilers. I use https://github.com/laher/goxc . After the initial goxc -t, you can use goxc xc instead of go build to check syntax on all platforms simultaneously.

— Reply to this email directly or view it on GitHubhttps://github.com/peterh/liner/pull/15#issuecomment-43642755 .

peterh commented 10 years ago

I should have made it clear that my pull request was a sketch

I suspected it might have been, which is why I separated the rough edges into a separate section.

I would like to expose get/set mode methods. I need to launch/suspend other programs.

It's the suspend part that I missed previously.

Given that the terminal state isn't really attached to the liner State in any meaningful sense (at least for your application), what do you think of a standalone TerminalMode function that returns an interface to restore whatever the mode was when you initially called it? Something like ad5ff6c26423dae63d9f28b003bafa653eec7dbf (which still needs testing)?

It's a few more lines than your patch, but a lot of that is in comments and error reporting, and I think it's worth a few lines to reduce the number of API calls (from three to two in this case). In particular, since it's not attached to State, you can call TerminalMode before you call NewLiner, which eliminates the need for GetOriginalMode.

michaelmacinnis commented 10 years ago

That looks great! I really like that TerminalMode is standalone and can be called before NewLiner.

michaelmacinnis commented 10 years ago

Two nitpicks:

Something like this:

type ApplyModer interface { ApplyMode() error }

would address those two nitpicks and follow what I believe to be the Go convention for interface names.

peterh commented 10 years ago

You're right. That is a much better name. I've force-updated the altmode branch; see 9d6d70cc78faeef1161c3e81e652cd4b6be99f4f. Please give it some testing and let me know if anything breaks.

Aside: I originally added Input to the name because it operates on stdin, and I wanted to leave space for a function that operates on stdout. But you're right — despite acting on a handle named stdin, it affects the output mode too.

michaelmacinnis commented 10 years ago

I did some quick tests and it looks great. Thanks!

peterh commented 10 years ago

Thanks for testing. Pushed.