Option to set the background extension mode to ANSI or spaces

[th1000s]

(Original topic: Fix right panel background extending beyond the truncation symbol)

In side-by-side mode, if background_color_extends_to_terminal_width
is set, the left panel color is extended via spaces, but the right
one via an ANSI sequence which instructs the terminal emulator to
fill the background color rightwards.

However due to the padding after the right panel the color will
extend beyond a possible truncation symbol:

TEXT#AB→#, not TEXT#AB→ with # being the green background.

This is fixed by using the same logic as in the left panel for the
right panel.

---

Before-After comparison in Konsole:

[th1000s]

The background is now only filled with spaces if any line in a block gets truncated (or, soon, wrapped):

This required moving some code written for wrapping here, I will now rebase the rest onto this.

[dandavison]

@th1000s can you explain why we ever need to fill with spaces? Sorry if I'm being slow! I don't think I've understood the motivation for this PR. I see this problem:

[th1000s]

No problem. Hereafter "ANSI" means the ANSI Sequence which extends the color until the end of the line.

So, ANSI placed after the truncation symbol is the problem [1] (adapted from your screenshot), ANSI placed on lines which are not cut off is fine [2].

But, just selectively skipping ANSI at e.g. [3] but still emitting it where the line is not cut of [5] would result in a jagged right edge, here crossed out in red.

Not using ANSI or spaces doesn't work either, because then the cut off lines are fine [3], but shorter lines are not [4].

This means the only option is to not use ANSI for truncated lines, and filling up lines which are too short with spaces, i.e. [3] and [5] up to the dotted line.

[dandavison]

Thanks very much for explaining that carefully. Would another alternative be to place the continuation character on the green (or whatever) background color instead of on the default (here white) background color? I was thinking that would then allow the code to be almost unchanged, with ANSI sequences used on all lines to fill the background color to the terminal width.

[dandavison]

Continuing my comment above, here are a couple of screenshots (on this branch at 306621b). Please correct me if I'm wrong but what I'm thinking is that even with this plan:

not use ANSI for truncated lines, and filling up lines which are too short with spaces

we are still going to get "jagged right edges" in a sense, because the column to which the right fill extends will vary between files and diff hunks. Hence my question about whether we should consider just placing everything, including the continuation character, on the colored background and always filling rightwards to the maximum extent, using the ANSI sequence in the right panel.

[th1000s]

Yes, making the background of a continuation character (↵ / ↴ / →) the same color as the rest of the line does make it visually more consistent. I will update it to that.

jagged right edges

For larger visual component these are still there, that is true.

But back to the initial issue, for my personal taste the inconsistency of having the background always end with exactly the last character on the red/left side, but having it extend further on the right side is worse - hence my initial fix of always padding with spaces.

And while this ruins terminal resizing/shrinking, in side-by-side mode that is going to happen much sooner anyhow. To help with that, maybe the pager could be started with --chop-long-lines. I also do not thing users change the size that often :). On the long run the only real fix is going to be using some terminal-ui which can redraw on resizes, maybe after some parts have been extracted into libdelta.

[dandavison]

for my personal taste the inconsistency of having the background always end with exactly the last character on the red/left side, but having it extend further on the right side is worse - hence my initial fix of always padding with spaces.

I'm still not sure about this (in the sense that I'm genuinely not sure, not a euphemism for being against!). One consideration is that we haven't had any complaints from other users, and also this was the explicit motivation for the ANSI code in the first place. Is it possible for this decision to be made independently of adding the side-by-side-wrapped feature? Switching back to spaces seems to be quite a large code change whereas I think changing the background color of the continuation character would be a small change. Is it possible for the side-by-side-wrapped branch to not be based on top of the more radical (switching to spaces) change?

[th1000s]

No problem, I'll adjust the background color of the continuation character to match that of the rest of the line. I'd also want to set the foreground color to blue to remove any possible ambiguities and still have it stand out a bit, similar how vim can highlight non-printable characters. Blue seems to be quite readable on both dark and light backgrounds (maybe tab-highlighting can work that way in delta as well).

As for the for the ANSI code motivation, my impression was that these were more relevant for the unified diff view and were then re-used for the right panel of the side-by-side mode.

[dandavison]

the inconsistency of having the background always end with exactly the last character on the red/left side, but having it extend further on the right side is worse -

OK, I think it's important that I understand this exactly. What I had thought was that side-by-side splits the user's terminal window exactly in half (modulo odd number width) and then applies equal (visual) widths of color in each panel. However, I'm looking at this, and I think I understand what you're saying: there seem to be two "extra" characters width of green beyond the truncation symbol on the right. So, why is that? Is the terminal width measurement or splitting calculation wrong?

As for the for the ANSI code motivation, my impression was that these were more relevant for the unified diff view and were then re-used for the right panel of the side-by-side mode.

Yes, I think that's true. And I do agree with you that we want equal width color on each side (filling the full terminal width ideally, and modulo odd-number terminal widths)

[dandavison]

No problem, I'll adjust the background color of the continuation character to match that of the rest of the line.

Great.

Regarding the spaces vs ANSI I am worried about causing you unnecessary work in the event that we both agree in the end that spaces is the right solution! So please, if you feel it makes sense to finish the conversation above before altering code, that would be absolutely fine of course.

I'd also want to set the foreground color to blue to remove any possible ambiguities and still have it stand out a bit, similar how vim can highlight non-printable characters.

That sounds good.

Blue seems to be quite readable on both dark and light backgrounds (maybe tab-highlighting can work that way in delta as well).

Right. Perhaps we'll want to make that color user-customizable? Side-by-side users will see it a lot (and delta seems to have taken it upon itself to be customizable to the last degree!)

[th1000s]

Indeed, the right padding is either 1 or 2, never 0 or 1, I thought it was a stylistic choice and never questioned it :)

If at most 1 line of padding is the goal, maybe that can be inserted in the middle, before the right line-numbers. That would make all the issues here disappear.

unnecessary work

This patchset already makes using spaces or ANSI a function parameter (BgFillWidth: whether to fill the line at all, containing a BgFillMethod: ANSI or via spaces). And spaces are required either way for the left panel.

user-customizable

Of course. The same goes for the continuation/termination chars, maybe someone likes other symbols more or wants to only use 7-bit ASCII.

[dandavison]

If at most 1 line of padding is the goal, maybe that can be inserted in the middle, before the right line-numbers. That would make all the issues here disappear.

This sounds great. There is one thing that we need to bear in mind. Basically, I think that the fact that delta's width calculations are leaving extra columns on the right is due to this line:

delta/src/options/set.rs

Lines 517 to 518 in e7d1e28

	// Allow one character in case e.g. `less --status-column` is in effect. See #41 and #10.
	opt.computed.available_terminal_width = (Term::stdout().size().1 - 1) as usize;

I just wanted to highlight that line of code. Everything that follows in the rest of this comment is detail, just in case it is helpful.

---

The issue being addressed there is that less --status-column inserts two columns on the left, and so if a user is using less --status-column, we have to be careful that delta does not overflow on the right.

To understand the history, the best issue to look at is this one: #115, which led to delta switching to use the ANSI fill-color-right sequence instead of manually padding with spaces. But that still leaves the issue for things like horizontal dividing lines, hence the computed.available_terminal_width subtraction line is still needed even after delta switched to using the ANSI fill-right sequence.

I think it may be relevant to mention that bat has a solution for this that we could consider introducing to delta. Bat's solution is to allow the user to declare how many columns are available to the application (bat/delta) via a +/- offset:

--terminal-width <width>
            Explicitly set the width of the terminal instead of determining it automatically. If
            prefixed with '+' or '-', the value will be treated as an offset to the actual terminal
            width. See also: '--wrap'.

See sharkdp/bat@dda27b2 and sharkdp/bat#376.

[th1000s]

I suspected there was a history to that behavior. Also, well, it is broken again, or maybe never worked with side-by-side mode:
Set the Terminal to any odd width of less than 173 (see echo $COLUMNS) and run git log -p e1ef7f54 | LESS=--status-column target/release/delta -s with exactly that commit.

[dandavison]

Very nice. Thanks, I didn't know about these traits.

[dandavison]

Since Left and Right and LeftRight are used here in line_numbers.rs, I think that raises the questions:

1. Should they be called Minus and Plus rather than Left and Right? (because they are used in contexts other than side-by-side, and because the rest of the codebase uses "minus" and plus" with this meaning)
2. Should they be defined somewhere more central than side-by-side.rs?

However, even if those questions are reasonable, I am happy to leave them unresolved for now and let you decide in the main side-by-side PR. Actually, probably good to resolve this question now.

[dandavison]

If "left" basically means the same thing as "minus" here, then the two sets of equivalent variable names here are making this code harder to understand. I'll wait for your opinion on this topic before merging this PR.

[dandavison]

This looks fantastic, I really appreciate the refactoring and data structures introduced to make the code clearer and more compact.

I'm just marking this "request changes" because I have that question about whether "left/right" and "minus/plus" have genuinely distinct semantics, or whether we can simplify to use "minus/plus" in the situations where we have a data structure holding data from removed and added lines respectively (which, if side-by-side is active will be destined for left and right panels respectively).

[th1000s]

Here is my attempt to have my cake and eat it too: The data structure is now PlusMinus (MinusPlus reads weird), it is indexed by Plus or Minus, but in a side-by-side context (which are actually most of them) it can also be index with PanelSide::{Left, Right}.

I can now squash this as-is into the previous commits or drop the Left/Right indexing.

[dandavison]

The data structure is now PlusMinus (MinusPlus reads weird), it is indexed by Plus or Minus, but in a side-by-side context (which are actually most of them) it can also be index with PanelSide::{Left, Right}.

Great, thanks!

I can now squash this as-is into the previous commits or drop the Left/Right indexing.

I'd like to squash and keep the Left/Right indexing.

Looks like some tests are failing now, but once you've squashed and CI is green, I'll merge this.

failures:

---- features::side_by_side::tests::test_two_minus_lines_truncated stdout ----
thread 'features::side_by_side::tests::test_two_minus_lines_truncated' panicked at 'index out of bounds: the len is 0 but the index is 0', src/features/side_by_side.rs:335:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- features::side_by_side::tests::test_two_minus_lines stdout ----
thread 'features::side_by_side::tests::test_two_minus_lines' panicked at 'index out of bounds: the len is 0 but the index is 0', src/features/side_by_side.rs:335:18

---- features::side_by_side::tests::test_one_minus_one_plus_line stdout ----
thread 'features::side_by_side::tests::test_one_minus_one_plus_line' panicked at 'String mismatch encountered while superimposing style sections: ' ' vs 'b'', src/paint.rs:761:17


failures:
    features::side_by_side::tests::test_one_minus_one_plus_line
    features::side_by_side::tests::test_two_minus_lines
    features::side_by_side::tests::test_two_minus_lines_truncated

[th1000s]

Ah, I mean the other left of course, should be fixed!

[dandavison]

Merged -- thanks very much for this work @th1000s, and I'm really looking forward to having the new wrapping side-by-side in master.