Re-enable ANSI fill by equalizing panel sizes #742

th1000s · 2021-10-18T22:37:22Z

This would turn | 123| foo | 124| bar into

|  123| foo | 124| bar
..^

with 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).

dandavison · 2021-10-21T14:35:59Z

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?

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.

dandavison · 2021-10-21T14:43:56Z

src/format.rs

 pub fn parse_line_number_format<'a>(
    format_string: &'a str,
    placeholder_regex: &Regex,
+    insert_space_before_first_linenr: &mut bool,


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).

Ok, how is this. I may have overdone it, but it IS odd behavior without knowing the context.

th1000s · 2021-10-23T12:19:39Z

I also added a fallback to spaces if the output is not to a terminal (e.g. piping to ansi2html).

space if there are no line numbers

Because currently there are always line numbers (enforced by the side-by-side feature) I just added an assert to warn whoever changes that.

dandavison · 2021-10-23T16:59:36Z

src/config.rs

-                // 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() {


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).

That seems to just cost an ioctl(1, TCGETS, ..) syscall on Linux, but I now store the value (could only find 2 calls btw).

src/tests/mod.rs

dandavison · 2021-10-23T17:33:37Z

@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 ~~, or at the end?~~ EDIT: I realize now that "at the end" isn't an option.

On master:

On this branch:

With first patch below (space at front)

With both patches below (space in middle)

src/features/side_by_side.rs

dandavison

(see comments inline)

th1000s · 2021-10-24T20:45:58Z

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 {odd_fix} option anywhere.

3 one-line changes

1: Move space to the beginning:

--- a/src/format.rs
+++ b/src/format.rs
@@ -111,3 +111,3 @@ pub fn parse_line_number_format<'a>(
         if *insert_space_before_first_linenr {
-            let prefix = SmolStr::new(format!("{}{}", prefix, ODD_PAD_CHAR));
+            let prefix = SmolStr::new(format!("{}{}", ODD_PAD_CHAR, prefix));
             *insert_space_before_first_linenr = false;

2: Move space into the middle (include previous change):

--- a/src/features/line_numbers.rs
+++ b/src/features/line_numbers.rs
@@ -168,3 +168,3 @@ impl<'a> LineNumbersData<'a> {
                     &*LINE_NUMBERS_PLACEHOLDER_REGEX,
-                    &mut insert_space_before_first_linenr,
+                    &mut false,
                 ),
--- a/src/features/side_by_side.rs
+++ b/src/features/side_by_side.rs
@@ -521,3 +521,3 @@ pub mod ansifill {
         fn adapt_sbs_data(mut sbs_data: SideBySideData) -> SideBySideData {
-            sbs_data[super::Left].width += 1;
+            sbs_data[super::Right].width += 1;
             sbs_data

dandavison · 2021-10-24T23:49:00Z

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.

th1000s · 2021-10-25T21:45:08Z

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.

dandavison

Fantastic! LGTM then. I'll merge this when the tests pass. Is there anything else needed before release?

th1000s · 2021-10-25T22:19:36Z

Good, that should be all for line wrapping v1.0.

th1000s added 2 commits October 18, 2021 23:41

Wrap linenumber configs in MinusPlus or LeftRight

7073657

Linenumber test helper added

169e4f9

dandavison reviewed Oct 21, 2021

View reviewed changes

th1000s force-pushed the reansi branch from eddb759 to 5136653 Compare October 23, 2021 12:15

th1000s marked this pull request as ready for review October 23, 2021 12:19

dandavison reviewed Oct 23, 2021

View reviewed changes

src/tests/mod.rs Outdated Show resolved Hide resolved

dandavison reviewed Oct 23, 2021

View reviewed changes

src/features/side_by_side.rs Outdated Show resolved Hide resolved

dandavison requested changes Oct 23, 2021

View reviewed changes

th1000s added 2 commits October 25, 2021 23:29

Remember if stdout is to an interactive terminal

fe0554e

Adapted `set_widths` because that's where there's a Term instance already.

Re-enable ANSI fill by equalizing panel sizes

626473d

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.

th1000s force-pushed the reansi branch from 5136653 to 626473d Compare October 25, 2021 21:40

dandavison approved these changes Oct 25, 2021

View reviewed changes

dandavison merged commit 5762a52 into dandavison:master Oct 25, 2021

Re-enable ANSI fill by equalizing panel sizes #742

Re-enable ANSI fill by equalizing panel sizes #742

Uh oh!

Conversation

th1000s commented Oct 18, 2021

Uh oh!

dandavison commented Oct 21, 2021

Uh oh!

dandavison Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

th1000s Oct 23, 2021

Choose a reason for hiding this comment

Uh oh!

th1000s commented Oct 23, 2021

Uh oh!

dandavison Oct 23, 2021

Choose a reason for hiding this comment

Uh oh!

th1000s Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dandavison commented Oct 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dandavison left a comment

Choose a reason for hiding this comment

Uh oh!

th1000s commented Oct 24, 2021

Uh oh!

dandavison commented Oct 24, 2021

Uh oh!

th1000s commented Oct 25, 2021

Uh oh!

dandavison left a comment

Choose a reason for hiding this comment

Uh oh!

th1000s commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dandavison commented Oct 23, 2021 •

edited

Loading