Skip to content
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

Implement various --border styles #54

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Implement various --border styles #54

merged 1 commit into from
Apr 8, 2019

Conversation

dmke
Copy link
Contributor

@dmke dmke commented Mar 5, 2019

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

src/main.rs Outdated Show resolved Hide resolved
@sharkdp
Copy link
Owner

sharkdp commented Mar 6, 2019

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
Copy link
Contributor Author

dmke commented Mar 6, 2019

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
Copy link
Contributor Author

dmke commented Mar 6, 2019

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
Copy link
Owner

sharkdp commented Mar 8, 2019

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
Copy link
Contributor Author

dmke commented Mar 9, 2019

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
Copy link
Contributor

selfup commented Mar 14, 2019

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

@sharkdp
Copy link
Owner

sharkdp commented Apr 8, 2019

@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!

@sharkdp sharkdp merged commit 07c2229 into sharkdp:master Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants