Add side-by-side line wrapping mode

[th1000s]

If the current line does not fit into the panel, then
it is not truncated but split into multiple lines. A
wrapping symbol is placed at the end of the line.

Wrapping is limited to a certain number of lines, if this
is exceeded the line is truncated by a now highlighted
truncation symbol.

Commandline argument -S / --side-by-side-wrapped.

Also adapted --keep-plus-minus-markers logic, required
to calculate the exact remaining panel width.

---

Updated* Example:

In the current state is is perfectly functional, however the limit of 5 wrapped lines is currently hard coded and I have not tested the behavior with --tabs yet. Also documentation beyond the stub in --help has yet to be written, plus more tests.

Ping #359 #299 #498

Let me know what you think!

[Kr1ss-XD]

Thank you, this looks ace !

I've just checked out your PR and am building it right now to do some testing :)

[Kr1ss-XD]

So, one thing I noticed is, that on wrapped lines, the plus-style/minus-style is only applied until the EOL while otherwise it's applied to the while line. Please see the screenshot, and compare source code lines 5 and 183 in the right pane.

[dandavison]

Wow, thanks very much for this PR and #512 @th1000s. I'll review as soon as I can. Meanwhile, I see that they merge together without conflicts, so that combined branch will be what I'm running locally, in side-by-side-wrapped mode!

[dandavison]

And @Kr1ss-XD, thanks for reviewing and testing also.

[th1000s]

@Kr1ss-XD Thank you for testing, I somehow managed to overlook that. It is fixed now.

@dandavison Yes, the other PR is a stand-alone bug fix, though I noticed the issue while working on this one.

[dandavison]

@th1000s thanks again for the very significant contribution you've made. This question applies to both #512 and this PR, but maybe it'll help to keep the discussion here in the big PR.

---

I was thinking that right-filling with a terminal emulator instruction was an improvement because the user can resize the screen width and the color will expand/contract automatically. If I'm understanding correctly, in #512 you switch to hard-padding with spaces in the right panel, and side-by-side-wrapped also hard-pads on the right with spaces. This of course results in effects like below (where I issued the command with a larger width and then afterwards made my terminal narrower).

I may not have fully understood the nature of the problem you're addressing in #512 but here's my question with my current level of understanding: is it possible to have the best of both worlds, i.e. fill right with terminal emulator colors instead of hard spaces, and yet avoid the problem you're addressing in #512? (Would explicitly painting the background color of the continuation character do the trick?)

master

this branch

[dandavison]

By the way, did you consider ↴ as an alternative to ↵ for the continuation character? I like the longer horizontal stem of ↵ but on the other hand I find that it sort of "points backwards" whereas ↴ perhaps points more in the direction of where the text is going? Anyone got any views on this?

[th1000s]

#512

Ah, thank you, now I understand the reasoning behind the ANSI sequence.

Only emitting color-filling when the line is not wider than the panel would help with the green background highlighting extending beyond it.

However this would result in shorter lines still having their highlighting extend beyond the panel, resulting in a jagged right edge:

But since these +/- lines are processed as one block, padding with spaces or with color-filling could depend on there being one or more lines which touch the right edge. Any plus-block containing a right-edge-touching line would stand out a bit (feature!) and not wrap nicely on terminal resizes, but otherwise have a consistent look.

↵ / ↴

I haven't given this detail much thought, I just picked the first angled arrow I found :)

But this might combine nicely with adding alignment to the wrapped line. Most of the time just a few characters do not fit onto the line (as visible in Kr1ss' screenshot). These could be right aligned and be marked with ↴ - as in "the rest is right down here".

When the wrapped line is wider, then it starts all the way on the left again, indicated by e.g. ↵.

Two different wrapping symbols are a less obtrusive way to indicate a different alignment than writing .../>> or so into the line itself.

Also and FYI, the truncation symbol is inverted to indicate that some information was lost, which wrapping should actually prevent.

[dandavison]

Hi @th1000s, I just want to check that you're not waiting for anything from me. I'm thinking that the next step here is that you're planning to rework the #512 changes to use the ANSI sequence, does that sound right? And possibly also some work on the alignment details and continuation characters as you describe above. For my part, I will review the code more, and at some point start a discussion in this thread about what the defaults should be and whether the new code can basically replace the old (but that needn't block the ANSI sequence and alignment details work).

[dandavison]

I haven't reviewed all the code yet but here are some thoughts on the bit that I've been through so far. It's really nice work, and I really appreciate you having grappled with one of the most complex parts of the codebase.

[dandavison]

I'd vote for renaming features/wrap.rs => features/side_by_side_wrap.rs.

[dandavison]

I think that users will basically prefer the new behavior as the default, rather than the old. If we expose wrap_max_lines as a command line option side-by-side-max-wrapped-lines, then am I right in thinking that we can have side-by-side invoke the new code, and a user who wants the old behavior can get that with side-by-side-max-wrapped-lines = 0?

[dandavison]

I think this is going to sound a bit odd, but would you mind using unwrap_or_else(|| panic!(...)) instead of expect()? It's a local style rule that I've been following.

I'm prepared to be shouted down :), but the reason is that I personally really dislike the idiom .expect("something is wrong"). It seems to me like the only bad design decision I've encountered so far in Rust -- that clearly reads as "I expect something is wrong". It seems that the intention was for it to be understood as "I expect this operation to succeed but, if not, then something is wrong", but it seems clear to me that the name doesn't work.

[th1000s]

Thank you for the kind words :)
I worked off all your review notes, and regarding:

I believe that the two calls below to wrap::minus_plus_too_long will always return the same values.

I thought so, too, but wanted to be on the safe side. So now the raw string is used to calculate the length, not one of the Style vectors derived from it.

The config options are unchanged so far, however I agree that wrap_max_lines should be exposed. Maybe 1 here would call the old behavior, and 0 means unlimited wrapping (and also set max-line-length to 0)?

The right-align wrapping has been added. As such a line now does not start on the left I added an unstyled cell before it so it can be distinguished from wrapped whitespace. However this is not visible in the context/zero lines and a bit jarring in changed lines and I am open to suggestions.

expect()

Yes, it is weird, it took me a white to finally think of this as .expect_success_otherwise_panic_with("message")

Also, more arrows, the first in the rows are ones currently used:

↵ ↲ ⮨ ⮰ ⤶ ⮐ ↩ / ↵ ↲ ⮨ ⮰ ⤶ ⮐ ↩

↴ ⮷ ⮯ 🢱 ⤵ ⤸ ⬎ / ↴ ⮷ ⮯ 🢱 ⤵ ⤸ ⬎

(oh, some are blue here, in the terminal they are all monochrome as on the right)

[dandavison]

Hi @th1000s, I'm still very excited about the work here of course. Can you tell me where things stand and how I can help? By the way, using delta for its side-by-side mode was recently suggested in the GitHub blog here: https://github.blog/2021-03-11-scripting-with-github-cli/#make-ghyour-own so that's a nice additional context for your improvements to land in.

[dandavison]

I need to spend more time with the diff here but I've taken another look and it's looking fantastic to me. It's very impressive; there's a lot for me to learn from it, it improves on the previous implementation, and I know it involved grappling with a lot of details of the codebase!

I think I can see a couple of bugs. You're probably aware of this one (the single space with no background colour before the ol):

Also, I can crash delta on this branch by resizing my terminal to (e.g.) 80 characters and issuing git log -p in the delta repo at 8ec99b8.

:thread 'main' panicked at 'String mismatch encountered while superimposing style sections: '↵' vs '↴'', src/paint.rs:790:17
stack backtrace:
   0: _rust_begin_unwind
   1: std::panicking::begin_panic_fmt
   2: delta::paint::superimpose_style_sections::superimpose_style_sections
   3: delta::paint::Painter::paint_line
   4: delta::paint::Painter::paint_zero_line
   5: delta::delta::StateMachine::handle_hunk_line
   6: delta::delta::delta
   7: delta::run_app
   8: delta::main

[th1000s]

The space in front of the the right-aligned text was actually on purpose, a first draft to indicate where the line continues. I replaced it with … and keep the background. For now these inserted symbols are not following any theme and are just blue. But for some reason later on the color is removed in the the zero hunks, but kept correctly in the diff hunks. Any idea why that might be?

And the other obvious improvement would be to not wrap lines by cutting up words so eagerly (see: bo↴ol) , but let's leave that for later :)

[dandavison]

Hi @th1000s,

The space in front of the the right-aligned text was actually on purpose, a first draft to indicate where the line continues.

Ah, OK great, I thought it was unlikely that you'd be unaware of it.

For now these inserted symbols are not following any theme and are just blue. But for some reason later on the color is removed in the the zero hunks, but kept correctly in the diff hunks. Any idea why that might be?

This is just a quick guess but this code comes to mind:

delta/src/paint.rs

Lines 536 to 572 in e7d1e28

	/// There are some rules according to which we update line section styles that were computed
	/// during the initial edit inference pass. This function applies those rules. The rules are
	/// 1. If there are multiple diff styles in the line, then the line must have some
	/// inferred edit operations and so, if there is a special non-emph style that is
	/// distinct from the default style, then it should be used for the non-emph style
	/// sections.
	/// 2. If the line constitutes a whitespace error, then the whitespace error style
	/// should be applied to the added material.
	fn update_styles(
	style_sections: &mut Vec<Vec<(Style, &str)>>,
	whitespace_error_style: Option<Style>,
	non_emph_style: Option<Style>,
	) {
	for line_sections in style_sections {
	let line_has_emph_and_non_emph_sections =
	style_sections_contain_more_than_one_style(line_sections);
	let should_update_non_emph_styles =
	non_emph_style.is_some() && line_has_emph_and_non_emph_sections;
	let is_whitespace_error =
	whitespace_error_style.is_some() && is_whitespace_error(line_sections);
	for section in line_sections.iter_mut() {
	// If the line as a whole constitutes a whitespace error then highlight this
	// section if either (a) it is an emph section, or (b) the line lacks any
	// emph/non-emph distinction.
	if is_whitespace_error
	&& (section.0.is_emph || !line_has_emph_and_non_emph_sections)
	{
	*section = (whitespace_error_style.unwrap(), section.1);
	}
	// Otherwise, update the style if this is a non-emph section that needs updating.
	else if should_update_non_emph_styles && !section.0.is_emph {
	*section = (non_emph_style.unwrap(), section.1);
	}
	}
	}
	}
	}

[dandavison]

@th1000s I've pushed a commit coalescing the two features into a single feature named side-by-side (thus making the new wrap code paths take over), and adding the following (provisional names and defaults) options to the CLI:

--side-by-side-wrap-max-lines <side-by-side-wrap-max-lines>
    Maximum number of wrapped lines to display in side-by-side mode. Any content that still does not fit will be
    truncated [default: 3]
--side-by-side-wrap-symbol <side-by-side-wrap-symbol>
    Symbol indicating that a line has been wrapped in side-by-side mode [default: ↵]

--side-by-side-wrap-right-percent <side-by-side-wrap-right-percent>
    Threshold for right-aligning wrapped content in side-by-side mode. If the length of remaining wrapped
    content, as a percentage of the panel width, is less than this quantity then it will be displayed right-
    aligned [default: 37.0]
--side-by-side-wrap-right-symbol <side-by-side-wrap-right-symbol>
    Symbol indicating that a line has been wrapped and that the subsequent content is displayed right-aligned
    [default: ↴]
--side-by-side-wrap-right-align-symbol <side-by-side-wrap-right-align-symbol>
    Symbol displayed in front of right-aligned wrapped content [default: …]

[dandavison]

Any thoughts on the option names and defaults? On reflection side-by-side-wrap-right-align-symbol maybe isn't the right name. Maybe something like side-by-side-wrap-right-prefix-symbol?

[th1000s]

Here are my suggestions, using the same prefix for all 3 right-align arguments:

--side-by-side-wrap-max-lines (keep)
--side-by-side-wrap-symbol (keep)
--side-by-side-wrap-right-align-percent (was --side-by-side-wrap-right-percent)
--side-by-side-wrap-right-align-wrap-symbol to mirror the name of the 2nd argument (was --side-by-side-wrap-right-symbol)
--side-by-side-wrap-right-align-prefix-symbol (was --side-by-side-wrap-right-align-symbol)

[dandavison]

I like your consistency improvements to the option names. One thought -- do you think perhaps the word "align" is superfluous? We could still have a shared prefix but keep it shorter as

--side-by-side-wrap-max-lines
--side-by-side-wrap-symbol
--side-by-side-wrap-right-percent
--side-by-side-wrap-right-wrap-symbol
--side-by-side-wrap-right-prefix-symbol

[dandavison]

Ok, I've done a review pass and I'll hand over to you now. Really exciting! I think delta might be about to possess humanity's most sophisticated side-by-side diff view!

[th1000s]

I think you've mentioned this elsewhere -- is respecting word boundaries in wrapping computations something that you feel should be a goal for future PRs (worth creating an issue for)?

Absolutely, --wrap-at-chars is a must. Plus a limit as with right align and other possible customization options.

git -c delta.side-by-side=true -c delta.wrap-right-symbol='<<' diff master...

It works on the command line, it seems as though parse(try_from_str ..) is ignored when reading values from the git config, is there some make_feature() magic missing? I'll leave that for you do debug :)

[th1000s]

And more arg parsing issues: Adding support for signed integer / isize leads to an error inside the builtin_feature! macro where an Into<OptionValue> for i32 is missing, see my isize branch - note that i32s are practically unused in delta!

Or, instead of a magic -1 value for --wrap-max-lines this could become a string argument and "unlimited" the explicit name for never ending wrapping.

[dandavison]

It works on the command line, it seems as though parse(try_from_str ..) is ignored when reading values from the git config, is there some make_feature() magic missing? I'll leave that for you do debug :)

Ok! That's fine then, we can ignore that. Discussed offline, the try_from_str logic needs to be moved to a code path called on values from gitconfig.

Or, instead of a magic -1 value for --wrap-max-lines this could become a string argument and "unlimited" the explicit name for never ending wrapping.

That would be fine with me, I'm certainly not attached to the magic -1. Maybe we can allow ∞ also.

[th1000s]

Validation moved to config.rs, which required a few more changes. wrap-max-lines now refers to the "extra" lines in the UI, internally 0 is used as-is. Also renamed the new file to wrapping.rs. However I left the wrap_line() logic untouched because the next improvements regarding alignment and configurability will probably entagle the "wrap" and "align" sections in there.

[dandavison]

I've made two minor requests for changes -- I could do them but I don't want to make a mess of the history that you've been preserving.

Other than that, LGTM!! I'm not aware of anything else that needs addressing? Let me know if I've forgotten an outstanding issue that needs solving before merge but, otherwise, I'll merge this as soon as you tell me what you want to do with my two minor requests (i.e. you make them, or tell me to).

[th1000s]

A few touch-ups, simplified Left/Right more, hit a nightly rustc crash, and it looks good to me as well! :)

[dandavison]

Thanks very much for this fantastic contribution @th1000s. It adds a feature that will significantly improve delta, and that many people have been looking forward to, it sets us up for additional wrapping-related improvements in the future, and it improves the codebase in many places, including via the addition of new patterns and techniques to the codebase that will be the source of still further codebase improvements in the future.

Can you advise on which further changes you'd like to see merged before the next release? (#727?)

[th1000s]

Very nice, and thank you for your help!

Before the next release the background color should be extended via ANSI codes again, see #727 and #740.

[fakhrulhilal]

Any idea when will it be released? This the feature that I've been waiting for. Delta is nice tool to compare many files in a scrollable screen. At least tell me how to build the cli tool.

Cheers.

[dandavison]

Hi @fakhrulhilal, I agree, this is a very exciting addition to delta. We're pretty close to release I think. If you'd like to try out the unreleased master branch that would be great. The instructions to build are in the README; it's just the standard process for building a Rust executable. If you have any feedback on released or unreleased features, please do let us know.

[fakhrulhilal]

Hi @fakhrulhilal, I agree, this is a very exciting addition to delta. We're pretty close to release I think. If you'd like to try out the unreleased master branch that would be great. The instructions to build are in the README; it's just the standard process for building a Rust executable. If you have any feedback on released or unreleased features, please do let us know.

Just realize that it's in readme file already. I'll try in PC and I'll let you know when I have feedback. Thank you very much.

[dandavison]

This has now been released (delta v0.9.1). Thanks very much @th1000s for the huge amount of work put into this, improving the codebase in many ways as well as adding this eagerly-awaited feature. 🚀

[Kr1ss-XD]

Thanks very much @th1000s for the huge amount of work put into this

+1 👍