Add (optional) status bar where a message can be shown#126
Add (optional) status bar where a message can be shown#126AngelEzquerra wants to merge 1 commit intoarxanas:mainfrom
Conversation
AngelEzquerra
force-pushed
the
statusbar
branch
3 times, most recently
from
31bc0fc to
b58eeb0
Compare
The status message can also be set through the command line with the optional --status-message argument.
AngelEzquerra
force-pushed
the
statusbar
branch
from
b58eeb0 to
e579c86
Compare
emesterhazy
left a comment
emesterhazy
left a comment
There was a problem hiding this comment.
Thanks for mailing this, it seems nice to have. I think we should do some cleanup first though. I left a few comments on the diff.
| fn example_contents() -> RecordState<'static> { | ||
| RecordState { | ||
| is_read_only: false, | ||
| // status_message: Some("Test status message".to_string()), |
There was a problem hiding this comment.
Should this be deleted or restored?
| let recorder = Recorder::new(state, &mut input); | ||
| recorder.run()?; | ||
|
|
||
| insta::assert_snapshot!(initial, @r###" |
There was a problem hiding this comment.
Can we add or replace this test with a test that has a short viewport and too many changes to fit it so that we can demonstrate that the status bar renders properly when there's potential overlap?
| output: _, | ||
| read_only: _, | ||
| dry_run: _, | ||
| status_message: _, |
There was a problem hiding this comment.
Can we clean these match cases in a parent commit (in a different PR as well for easier review)? That will avoid the need to update all of the match cases for this PR, and in the future as well if we add more fields.
For example, this can be rewritten as
Opts {
dir_diff: false,
left,
right,
Base: None,
..
}| impl RecordState<'_> { | ||
| /// Create a new RecordState for testing purposes. | ||
| #[cfg(test)] | ||
| pub fn for_test( |
There was a problem hiding this comment.
Why do we need this? It's not adding any additional functionality and just delegates directly to struct initialization. It looks like this isn't used either, but maybe I'm missing it.
| base: None, | ||
| output: None, | ||
| read_only: false, | ||
| status_message: None, |
There was a problem hiding this comment.
I feel like we can do a cleanup of the struct initialization as well (also in a separate commit). If we derive Default for Opts then we can rewrite this to:
Opts {
left: PathBuf::from("left"),
right: PathBuf::from("right"),
..Default::default()
}I don't think writing out every field necessarily makes the tests clearer, and it makes the diff large for changes like this where the optional field isn't even used in the test.
2 participants
This is an attempt to implement #125.
Note that the status message can also be set through the command line with the optional --status-message argument.
If this is OK we could also add a --title-message that shows a message at the top of the screen (above the menubar).