sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.93k stars 227 forks source link

Implement various --border styles #54

Closed dmke closed 5 years ago

dmke commented 5 years ago

This might help to remedy display issues described in #17.

Please note, I'm not a Rust developer, so there might be better ways to solve this. In particular, I'm not a fan of the various matches on the border_style field.

Here's a screenshot of how this looks in my terminal:

image

sharkdp commented 5 years ago

Thank you very much for your contribution!

I didn't have time to look at the code changes, but I have to say that I really like the style without any border. I tend to think that we should make this the default. What do others think?

dmke commented 5 years ago

Currently, without a border, this puts both a leading and a trailing space character on each line. I will try to eliminate that, but I might need some guidance later. Let me come up with the implementation first :-)

dmke commented 5 years ago

I've now spend a good amount of time trying to understand the borrow checker. My initial plan was to move some details into something like

impl BorderStyle {
    // Returns left corner, colum separator, horizontal line and right corner.
    fn header_elems(self) -> Option<(char, char, char, char)> {
        match self {
            BorderStyle::Unicode => Some(('┌', '┬', '─', '┐')),
            BorderStyle::Ascii   => Some(('+', '+', '-', '+')),
            BorderStyle::None    => None,
        }
    }
}

and condense Printer.header to

fn header(&mut self) {
    if let Some((l,c,h,r)) = self.border_style.header_elems() {
        writeln!(self.stdout, "...", ...).ok()
    }
}

but self (the Printer) is in a borrowed context. Moving the if let into Printer's sole consumer (the print_byte function) would (a) not help either, and (b) make that function even more complex.

Another approach would be to move the writeln! into a BorderStyle.print_header() function, but that'd need to borrow the Printer's self.stdout, so that wouldn't work, either.

Do you have suggestions, on how to make this a bit more DRY?

sharkdp commented 5 years ago

I can try to look into this, if you could provide the full code of what you are trying to do.

Would this work?

fn header(&mut self) {
    let header_elems = self.border_style.header_elems();
    if let Some((l,c,h,r)) = header_elems {
        writeln!(self.stdout, "...", ...).ok()
    }
}
dmke commented 5 years ago

Would this work?

No, that was not it. I've found a solution, though, by allowing self.border_style to be borrowed in the BorderStyle.header_elems function (are those the right terms? :-D):

-fn header_elems(self) -> Option<(char, char, char, char)> {
+fn header_elems(&self) -> Option<(char, char, char, char)> {

I've updated the commit (again).

selfup commented 5 years ago

Just chiming in. The border none is neat for sure 👍

sharkdp commented 5 years ago

@dmke Sorry for the delay. I have a few suggestions, but I think it's easier if I just merge this as is and integrate a changes myself. Thank you very much!