hud: add a prompt package for the new terminal mode#3464
Conversation
|
ack, i forgot what a pain dealing with TTY APIs is |
| serverStatus := "(without browser UI)" | ||
| if hasBrowserUI { | ||
| if p.host == "0.0.0.0" { | ||
| serverStatus = fmt.Sprintf("on %s (listen on 0.0.0.0)", p.url) |
There was a problem hiding this comment.
nit
| serverStatus = fmt.Sprintf("on %s (listen on 0.0.0.0)", p.url) | |
| serverStatus = fmt.Sprintf("at %s (listening on 0.0.0.0)", p.url) |
| switch r { | ||
| case ' ': | ||
| _, _ = fmt.Fprintf(p.stdout, "Opening browser: %s\n", p.url.String()) | ||
| _ = p.openURL(p.url.String()) |
There was a problem hiding this comment.
I don't know under what cases this might return an error (too many files, can't start a new process, I dunno), but it seems worth it to log any error, so at least it's not silently doing nothing
| } | ||
| }() | ||
|
|
||
| for ctx.Err() == nil { |
There was a problem hiding this comment.
IIUC, at the moment, this is just holding onto the subscriber's notify goroutine forever, which ~means its OnChange can only ever be called once (which then might surprise someone trying to edit this subscriber). It seems to work, but also seems different from how our other subscribers work, and potentially accidental (like, does this mostly make p.printed redundant?)
would it be better to put this for loop in a new goroutine? am I missing something?
There was a problem hiding this comment.
yep! i've been moving towards subscribers that just hold onto their lock until they are done. And using goroutines less often. they're a ton easier to test.
though i think in this case it makes sense to do the input polling on a goroutine, done!
2 participants
Hello @landism, @jazzdan,
Please review the following commits I made in branch nicks/browser:
e503e18 (2020-06-15 16:00:46 -0400)
hud: add a prompt package for the new terminal mode
Code review reminders, by giving a LGTM you attest that: