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
Re-enable ANSI fill by equalizing panel sizes #742
Conversation
I think I'd vote for the middle, but I haven't experimented. You've played around with this more than me, is middle what you're inclined towards? My guess is it will be less noticeable. |
src/format.rs
Outdated
@@ -99,13 +101,25 @@ pub fn make_placeholder_regex(labels: &[&str]) -> Regex { | |||
pub fn parse_line_number_format<'a>( | |||
format_string: &'a str, | |||
placeholder_regex: &Regex, | |||
insert_space_before_first_linenr: &mut bool, |
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 you add a docstring / comment to this function explaining the problem that is motivating this space insertion? And perhaps another comment somewhere explaining the need to introduce a space at all (whether in line numbers or elsewhere).
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, how is this. I may have overdone it, but it IS odd behavior without knowing the context.
I also added a fallback to spaces if the output is not to a terminal (e.g. piping to ansi2html).
Because currently there are always line numbers (enforced by the side-by-side feature) I just added an assert to warn whoever changes that. |
src/config.rs
Outdated
// an odd width then extending the background color with an ANSI sequence | ||
// would indicate the wrong width and extend beyond truncated or wrapped content, | ||
// thus spaces are used here by default. | ||
line_fill_method: if !Term::stdout().is_term() && !testing() { |
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.
Do you have a sense whether Term::stdout()
might be expensive? With this we call it twice now in the "init sequence" (cli.rs, config.rs, set.rs etc).
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 seems to just cost an ioctl(1, TCGETS, ..)
syscall on Linux, but I now store the value (could only find 2 calls btw).
@th1000s I'm sorry to say this so late, but I'm not sure about using the line numbers for the extra space. You've worked really hard to make the visual appearance perfect, and it is seeming to me that the asymmetry between the two line number columns is jarring, by the high standards that you have set. I think we can assume that people will have odd terminal widths approximately 50% of the time so this is going to be seen a lot. Should we consider placing that space in the "middle", i.e. just before the right line number column On master: On this branch: With first patch below (space at front) With both patches below (space in middle) |
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.
(see comments inline)
Indeed, "at the end" means keep using spaces. But moving the space to the beginning or into the middle is easy and requires almost no changes, see the patches below. This could be made configurable as well and wouldn't have any runtime cost, but I'm fine with either location, let me know what you prefer. Interesting that you notice the larger line number field, I moved it there because to me that was the most inconspicuous location. Also because the number padding currently is not configurable so nobody gets the idea of adding an 3 one-line changes1: Move space to the beginning:
2: Move space into the middle (include previous change):
|
Thanks for those patches! I've updated my screenshots in the comment above. Personally I'm liking the "space in the middle", i.e. with both those patches applied. I'm finding that I can think of that as "perfectly symmetrical, just with extra separation in the middle", and there's none of the "why is the left line number column indented like that?" feeling I get with just the first patch. I'd be happy for it to be customizable though, especially if you or anyone else has a different preference. |
Adapted `set_widths` because that's where there's a Term instance already.
Make the two panels in side-by-side use the full terminal width by inserting an extra space in the center between the panels if the width is odd and ANSI filling is enabled. Fall back to spaces when the output is not to a terminal.
Then "space in the middle" it is! Lets leave this as the default, if anyone wants to customize some part of it they can easily add this later. |
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.
Fantastic! LGTM then. I'll merge this when the tests pass. Is there anything else needed before release?
Good, that should be all for line wrapping v1.0. |
This would turn
| 123| foo | 124| bar
intowith an extra space there. Most noticeable with two digit line numbers. Any preference where to place the space if there are no line numbers (not possible without editing the source at the moment, but still): In front, or in the middle? If the latter, then maybe always go from 2 to 1 spaces there, not from 0 to 1.
(Nevermind the failing tests, will need to add
--line-fill-method=spaces
to them).