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

An initial engine terminal UI and demo. #6223

Merged
merged 2 commits into from Jul 25, 2018

Conversation

Projects
None yet
4 participants
@kwlzn
Copy link
Member

kwlzn commented Jul 24, 2018

to run the demo binary:

$ (cd src/rust/engine/ui && cargo run -p ui --release)

@kwlzn kwlzn requested review from stuhood , jsirois and illicitonion Jul 24, 2018

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented Jul 24, 2018

demo of signal-free resize handling:

lightning-logger-live-resizing

@illicitonion
Copy link
Contributor

illicitonion left a comment

This is super awesome to play around with, thanks for putting it together!

A few behaviour questions, and a pile of comments for small optimisations :)

sigil: '⚡',
divider: "▵".to_string(),
poll_interval_ms: Duration::from_millis(55),
padding: (0..indent_level)

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

padding: " ".repeat(indent_level.into())

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

// Gets the current terminal's cursor position, if applicable.
fn get_cursor_pos(&mut self) -> (u16, u16) {
match self.terminal {
Console::Terminal(ref mut t) => match &t.cursor_pos() {

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

Console::Terminal(ref mut t) => t.cursor_pos().unwrap_or((0, 0)),

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed


// Gets the current terminal's width and height, if applicable.
fn get_size() -> (u16, u16) {
match termion::terminal_size() {

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

termion::terminal_size().unwrap_or((0, 0))

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed


// Sets the terminal size for signal-free resize detection.
fn get_max_log_rows(&self) -> usize {
self.terminal_size.1 as usize - self.action_map.len() - 1

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

If my terminal is fewer lines high than the action_map, this panics with a subtraction overflow... I'm not sure what the correct behaviour here is... I guess maybe degenerating to a single "X workers exist" line?

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

yeah.. hopefully this case is rare by nature, but I suspect we'd want to fall back to non-tty mode and just scroll a summary as if a tty weren't there (since a tty with few lines is effectively useless anyway). added a TODO.

}

// Writes output to the terminal.
fn write(&mut self, msg: String) -> Result<usize> {

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

This should probably take a &str

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

// Renders one frame of the log portion of the screen.
fn render_logs(&mut self, max_entries: usize) -> usize {
let cursor_start = self.cursor_start;
let printable_logs: Vec<String> = self.logs.clone().into_iter().take(max_entries).collect();

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

I'm not sure about this line; is the intent that self.logs will always contain all logs, or that we'll pop them out of the VecDequeue as we display them?

If the former, I suspect this should be a self.logs.iter().take(max_entries).cloned().collect(), to just copy out the things we care about. If the latter, I suspect this should either be a self.logs.drain or a self.logs.split_off.

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

it'll always contain enough logs to be able to fill on re-painting if the screen is resized larger - currently unbounded (but I'll cap this arbitrarily to 500 for now).

when I attempt to s/.clone().into_iter()/.iter()/ I get:

error[E0502]: cannot borrow `*self` as mutable because `self.logs` is also borrowed as immutable
   --> ui/src/display.rs:168:7
    |
155 |     let printable_logs: Vec<&String> = self.logs.iter().take(max_entries).collect();
    |                                        --------- immutable borrow occurs here
...
168 |       self
    |       ^^^^ mutable borrow occurs here
...
183 |   }
    |   - immutable borrow ends here

error[E0502]: cannot borrow `*self` as mutable because `self.logs` is also borrowed as immutable
   --> ui/src/display.rs:179:7
    |
155 |     let printable_logs: Vec<&String> = self.logs.iter().take(max_entries).collect();
    |                                        --------- immutable borrow occurs here
...
179 |       self.render_divider(counter as u16);
    |       ^^^^ mutable borrow occurs here
...
183 |   }
    |   - immutable borrow ends here

error: aborting due to 2 previous errors

and am unable to satisfy the borrow checker via standard tricks.

fn render_actions(&mut self, start_row: usize) {
let mut worker_states: Vec<(String, String)> = self.action_map.clone().into_iter().collect();

worker_states.sort_by_key(|k| k.clone().0);

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

worker_states.sort_by would avoid having to clone the strings for comparisons (this is a piece of rust that definitely annoys me :()

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

pub fn add_worker(&mut self, worker_name: String) {
self.update(
worker_name.clone(),
String::from(format!("booting {}", worker_name)),

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

format! returns a String, so you shouldn't need the String::from wrapper here. (There are a few more of these throughout the PR, too)

(Also, if you do the format! before the self.update call, you should be able to avoid the worker_name clone; i.e. let action = format!("booting {}", worker_name) ; self.update(worker_name, action))

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

}

// Removes a worker/thread from the visual representation.
pub fn remove_worker(&mut self, worker_id: String) {

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

Arg should probably be &str not String, as only the reference is used

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

poll_interval_ms: Duration,
padding: String,
terminal: Console,
action_map: HashMap<String, String>,

This comment has been minimized.

@illicitonion

illicitonion Jul 24, 2018

Contributor

Should this be a BTreeMap? It looks like when we consume from it, we always want it to be sorted; just storing it sorted is probably cheaper than sorting every time we consume?

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

works for me! fixed

@jsirois
Copy link
Member

jsirois left a comment

Excellent!

I'll bow out of this review with little context. One feature suggestion though which I'm sure is already on the roadmap and is clearly out of scope at the moment!


// Prep the screen for painting by clearing it from the cursor start position.
fn clear(&mut self) {
let cursor_start = self.cursor_start;

This comment has been minimized.

@jsirois

jsirois Jul 24, 2018

Member

It would be really nice to be able to scroll back to view older rendered log lines that are currently lost, potentially pausing the render loop so that you didn't have to wait until the end of the session to do the scrollback without interruption.

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

yeah. I think facilitating scrollback on exit should be easily accomplishable by dumping the full log buffer on exit. but it's not super clear how to achieve that during an active run. we could implement up/down scrolling in the display logic itself or employ some raw vs non-raw mode switching tricks, perhaps. added a TODO.

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good to me.

I'd like to bias toward focusing on integration with the engine this week... if we're able to defer any performance-related optimizations here, that would be my preference.

@@ -39,6 +39,7 @@ members = [
"resettable",
"testutil",
"testutil/mock",
"ui"

This comment has been minimized.

@stuhood

stuhood Jul 24, 2018

Member

This should also be in default-members I think, as it doesn't have any system requirements.

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

fixed

terminal_size: (u16, u16),
}

// TODO: Prescribe a threading/polling strategy for callers - or implement a built-in one.

This comment has been minimized.

@stuhood

stuhood Jul 24, 2018

Member

Before landing this, please take a look at which TODOs deserve more fleshed out github issues vs just inline expansion for a reader who might not have all of the context.

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

will do.

}
}

fn render_for_pipe(&self) {

This comment has been minimized.

@stuhood

stuhood Jul 24, 2018

Member

This one could definitely use some more fleshing out. I think I had been thinking that in the absence of a TTY we would not launch this UI at all, and simply render log messages. Perhaps there is an advantage to having this codepath involved in both modes...?

This comment has been minimized.

@kwlzn

kwlzn Jul 24, 2018

Member

yeah, I was originally thinking that we would also not launch this UI for pipe mode - but in terms of "concerns", having the EngineDisplay handle all rendering seems more ideal than having this + a sidecar pipe impl, I think. handling failure modes also becomes way easier - can't pop the terminal into raw mode for some reason? fallback to pipe mode, etc.

but would be totally open to going either way.

This comment has been minimized.

@stuhood

stuhood Jul 24, 2018

Member

Fine with me.

@kwlzn kwlzn merged commit 9e30b20 into pantsbuild:master Jul 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

An initial engine terminal UI and demo. (pantsbuild#6223)
to run the demo binary:

$ (cd src/rust/engine/ui && cargo run -p ui --release)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment