[WIP] add line highlight #346
Conversation
There was a problem hiding this comment.
Thank you very much!
The code looks good. I think we should somehow extend the highlighting a little bit, though. Currently, empty lines can not be highlighted at all, for example. We should think about either extending the highlighting-range to the right side of the screen or by extending it into the side panel on the left(?). What do you think?
src/printer.rs
Outdated
| @@ -296,6 +301,18 @@ impl<'a> Printer for InteractivePrinter<'a> { | |||
| } | |||
| } | |||
|
|
|||
| // Line highlighting | |||
| let background = match self.config.highlight_line { | |||
There was a problem hiding this comment.
This whole block could be something like:
let background = self.config.highlight_line
.filter(|line| line == line_number)
.map(|_| self.background_highlight);Granted, I'm not sure if that is easier to read. Soon, there will be Option::replace which could be used instead of .map(|_| ..).
src/clap_app.rs
Outdated
| .long("highlight-line") | ||
| .overrides_with("highlight-line") | ||
| .takes_value(true) | ||
| .value_name("n") |
There was a problem hiding this comment.
Can we use a captital N here to be consistent with the --line-range option?
src/clap_app.rs
Outdated
| .value_name("n") | ||
| .help("Highlight a line.") | ||
| .long_help( | ||
| "Highlight the nth line. The background color is changed to create contrast.", |
There was a problem hiding this comment.
this would be "the N-th line" then.
|
I agree! Right when I started testing the changes I thought this kind of sucks if it doesn't change more that just the background of the text. |
| let background = self.config.highlight_line | ||
| .filter(|line| *line == line_number) | ||
| .map(|_| self.background_highlight) | ||
| .unwrap(); |
There was a problem hiding this comment.
This is undesirable right? I was getting and Option<Option> situation.
There was a problem hiding this comment.
Yes. We want to avoid unwrap. In this case, it should be and_then instead of map.
If you have any ideas, feel free to experiment here. |
|
@tskinn Any updates on this? How do you think we should proceed here? |
|
Sorry, no updates! I haven't looked very deeply into it yet. I've bin a little strapped for time lately but I will try to find some time this week. |
|
closing this in favor of #453 |
There might be a better place to get the background highlight color. Not sure.