-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added text wrapping. (Fixes #54) #102
Conversation
This looks super cool - thank you very much for your contribution! I will only leave a few quick comments for now and add a proper review later when all the checks pass (and the conflicts are resolved). |
src/printer.rs
Outdated
} | ||
}; | ||
|
||
// Generate the panel (gutter) width. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be computed first and then simply passed to the Printer
constructor? This way, we don't need the (mutable) instance
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that it would be a fair tradeoff, making the code easier to maintain by only having a single function (gen_decorations
) for determining which components to display and their sizes.
I could change it with a couple of if
s and consts, if you prefer to avoid the mutable instance
variable though.
src/printer.rs
Outdated
let decorations = instance.gen_decorations(0); | ||
instance.panel_width = decorations.len() | ||
+ decorations.iter().fold(0, |a, x| a + x.size) | ||
+ if config.output_components.grid() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 is added in both cases here? This can't be right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used to count the grid line as part of the gutter's width, but ended up changing it later on to simplify calculations. My bad, it looks like I forgot to remove this leftover code.
src/printer.rs
Outdated
|
||
// Finished. | ||
write!(self.handle, "\n")?; | ||
return Ok(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok(())
is equivalent to return Ok(());
here.
src/printer.rs
Outdated
fn print_git_marker(&self, line_number: usize) -> Option<String> { | ||
|
||
|
||
#[doc = " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply use triple slashes for doc strings:
/// Generates all the line decorations
fn gen_decorations(...)
In general, doc strings are fine but I prefer to use them only if things are not clear from the function name. Things like this Arguments
list here are hardly useful in my opinion and have to be maintained.
src/terminal.rs
Outdated
@@ -26,26 +26,46 @@ fn rgb2ansi(r: u8, g: u8, b: u8) -> u8 { | |||
} | |||
} | |||
|
|||
//pub fn as_terminal_escaped( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I changed the code, I kept comments of the original functions as a reference for how to do certain things. It looks like I forgot to remove that one, sorry.
Also, congratulations on your first Rust code! (could you please run |
- Reformatted code. - Removed leftover code. - Removed leftover comments. - Fixed compiling on Rust 1.24.0
Now bat(1) can be used like cat(1) again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cd26d40: Wow... Uh, you ok git? My local copy definitely didn't look like this, sorry.
I'm not quite sure what was up with git on that last commit, but it's all properly committed now.
src/printer.rs
Outdated
} else { | ||
for &(style, text) in regions.iter() { | ||
let mut chars = text.chars().filter(|c| *c != '\n'); | ||
let mut remaining = text.chars().filter(|c| *c != '\n').count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just clone the chars iterator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that in the initial PR, but it failed to build on Rust 1.24.0.
The chars.clone()
method worked on the on the latest version, however, but I'm not quite sure how to make it work for 1.24.0. If you have any suggestions, I would happily replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh weird
src/terminal.rs
Outdated
s | ||
let mut s: String = String::new(); | ||
write!(s, "{}", style.paint(text)).unwrap(); | ||
return s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove all of this and just return style.paint(text).to_string()
. also, the return statement is not necessary. the last expression in a function will be returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Fixed.
───────┬──────────────────────────────────────────────────────────────────────── | ||
│ File: sample.rs | ||
───────┼──────────────────────────────────────────────────────────────────────── | ||
│ struct Rectangle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the vertical line is gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be part of the resizing gutter change. When there aren't any line decorations, the vertical grid border is removed to save space (since there's nothing to separate).
I could add it back if the old behaviour is preferred, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that the vertical line is part of the grid and should be there if it's specified, but maybe @sharkdp can clarify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change. If there is nothing to display in the side-panel, I don't think there is any need for a vertical grid line.
@@ -1,22 +1,22 @@ | |||
1 │ struct Rectangle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/printer.rs
Outdated
} | ||
|
||
// Grid border. | ||
let border = if gutter_width > 0 && self.config.output_components.grid() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can go inside gen_border
right?
src/printer.rs
Outdated
} | ||
|
||
// It wraps. | ||
if self.config.output_wrap == OutputWrap::Character { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the else
part, so we already know that this will evaluate to true every time. or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it evaluates to true every time. I left it in there in case different wrapping styles were added in the future, but on second thought, it's unnecessary right now.
src/printer.rs
Outdated
Ok(()) | ||
} | ||
|
||
fn print_line_number(&self, line_number: usize) -> Option<String> { | ||
/// Generates all the line decorations. | ||
fn gen_decorations(&self, line_number: usize) -> Vec<PrintSegment> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line_decorations
?
src/printer.rs
Outdated
/// Generates the decoration for displaying the line number. | ||
fn gen_deco_line_number(&self, line_number: usize) -> PrintSegment { | ||
let plain: String = format!("{:width$}", line_number, width = LINE_NUMBER_WIDTH); | ||
let color = self.colors.line_number.paint(plain.to_owned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plain
is already a string, so to_owned
is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing to_owned()
, and ended up with this:
Error[E0382]: use of moved value: `plain`
--> src/printer.rs:217:19
|
213 | let color = self.colors.line_number.paint(plain);
| ----- value moved here
...
217 | size: plain.len(),
| ^^^^^ value used here after move
|
= note: move occurs because `plain` has type `std::string::String`, which does not implement the `Copy` trait
Replacing it with plain.clone()
works, though. Would that be better to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think paint can take a &str
, so you can pass &plain
. also I didn't notice before, but the variable name color
is probably not correct
src/printer.rs
Outdated
} | ||
|
||
/// Generates the decoration for displaying the git changes. | ||
fn gen_deco_line_changes(&self, line_number: usize) -> PrintSegment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it just be line_changes
? two abbreviations in a method name make it confusing. same for the number one. that way, you can remove the doc comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll rename it.
@@ -13,6 +13,12 @@ pub enum OutputComponent { | |||
Plain, | |||
} | |||
|
|||
#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] | |||
pub enum OutputWrap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since there are only two variants, each with no data, maybe this can be represented with a bool
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used an enum in case anybody wanted to add different wrapping styles (e.g. word wrap) in the future, but I could change it to a bool if that's preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I guess you can just go ahead and add that variant then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer the enum here, even if there are just two variants for now ("Boolean blindness"). We shouldn't add the third variant if it is not yet used.
Also ran cargo fmt.
src/printer.rs
Outdated
border.text.to_owned() | ||
)?; | ||
|
||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this continue
statement needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears not. Removing it in the next commit.
src/printer.rs
Outdated
write!(self.handle, "\n")?; | ||
} | ||
|
||
// Finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be safely removed 😃
I just performed some simple benchmarks. As probably expected, the text-wrapping has some performance overhead (up to ~30%):
Unfortunately, the performance drops by the same amount even when calling with |
That would probably be the consequence of the way I rewrote the line decorations. I'll see what I can do to reduce the performance impact. Would you mind if I split the line decorations off into separate objects? There's a lot of caching that could be done if so. |
@sharkdp I split up the decorations into their own file and added caching to them. Here's the results:
Character wrapping
No wrapping
Bat 0.3
|
@eth-p Thank you for looking into the performance-side of this! Is the caching really the important change here? It seems like it would only help for files with really long lines where the line number would not have to be displayed? Sorry that the review process has been dragging on for some time now, but there are (still) a few things that I would like to verify/update before I merge this. To be honest, while I really like the outcome (see screenshots) and the implemented features, I am not quite confident about the changes in the code. Part of the reason might also be that I wasn't really happy with that part of the code (generating the different output elements) to begin with 😄. While it's hard to pinpoint any "bad" changes, here are a few things that we might want to discuss:
Concerning the actual wrapping:
|
The way I implemented the caching, it should increase the performance whether or not the lines are wrapped. Consider the non-cached versions of the git changes and grid border decorations: fn line_changes(&self, line_number: usize) -> PrintSegment {
let color = if let Some(ref changes) = self.line_changes {
match changes.get(&(line_number as u32)) {
Some(&LineChange::Added) => self.colors.git_added.paint("+"),
Some(&LineChange::RemovedAbove) => self.colors.git_removed.paint("‾"),
Some(&LineChange::RemovedBelow) => self.colors.git_removed.paint("_"),
Some(&LineChange::Modified) => self.colors.git_modified.paint("~"),
_ => Style::default().paint(" "),
}
} else {
Style::default().paint(" ")
};
return PrintSegment {
text: color.to_string(),
size: 1,
};
}
fn line_border(&self) -> PrintSegment {
return PrintSegment {
text: self.colors.grid.paint("│ ").to_string(),
size: 2,
};
} In both of them, we were calling
I could definitely change it to use
I don't think
That was my original plan, but I couldn't figure out a sane way to do it since the
I suppose that would depend on whether or not the decorations should be considered as individual modules which are used by the printer, or as a part of the printer itself. I was thinking the former, but I'm likely biased having come from an OOP background where monolithic functions/classes are discouraged. I'll leave it up to you to decide whether or not I should move the decorations back into
Fixed!
That was actually my original idea! For the reason you just mentioned, it ended up not working out too well. I would've had to implement character wrapping on top of region wrapping, and it still would've resulted in funny-looking wrapping like this:
|
Thank you very much for the clarifications!
Ok. Let's stay with your solution for now.
Ok, we can look into this in the future.
Ok, let's keep it for now.
Great, thank you.
Yeah, right 😔 |
Merged the |
It should pass all the checks now.
Thank you very much!! |
I use bat pretty frequently, so I wanted to give back and make it even better by adding text wrapping (issue #54).
I've never touched Rust before this, and I apologize if the code style is weird or if any of my changes have less-than-ideal performance.
Changes:
Text wrapping.
![image](https://user-images.githubusercontent.com/32112321/39954037-7374f742-556c-11e8-9c43-2856b07d3ce6.png)
Lines will now automatically wrap at a character boundary.
Syntax coloring will also wrap.
Resizing gutter (panel).
![image](https://user-images.githubusercontent.com/32112321/39954054-0db9db56-556d-11e8-8557-f6be8d81510f.png)
![image](https://user-images.githubusercontent.com/32112321/39954079-bf96ae44-556d-11e8-88b4-01a18d5db20d.png)
The gutter (area that shows line numbers and changes) will now automatically resize.
This cuts back on the amount of wasted space when not showing everything.
It will also disappear if not needed.