Allow to scroll diffs horizontally

[M1cha]

This Pull Request fixes/closes #1017.

This is a simple implementation that works really well according to my testing.

It may need some improvements like an UI indicator for both the current column and the keymap. We should talk about that as part of this PR.

One issue that I've found: It is hard to use keyrepeat on the left key to scroll to the beginning without leaving the diff because starting from column 0 the key suddenly has a different meaning and jumps all the way back up. A solution would be to provide separate keymaps for scrolling.

I followed the checklist:

• [ ] I added unittests
• [x] I ran make check without errors
• [x] I tested the overall application
• [x] I added an appropriate item to the changelog

[extrawurst]

looks nice! lets paint a scrollbar only as long as scroll is not 0

[M1cha]

I'm not sure if that's the best way since you might still wanna scroll vertically while being scrolled horizontally and still see where in the document you you are.

[extrawurst]

Don’t know what the vertical scrolling has to do with it

[M1cha]

well you can scroll both directions at the same time and while being scrolled horizontally I'd still like to know where I am vertically.

[extrawurst]

Still no idea why we worry about the existing scrollbar (vertical). For the horizontal Scrollbar I would try to not convolute the view as long as we did not start scrolling to the right.

[M1cha]

oh for some reason I thought that you mean that we hide the vertical scrollbar as soon as we're scrolled horizontally, sry about that :roll_eyes:

[M1cha]

I've implemented a scroll bar :) A few things:

• it's using the vertical bar symbol. that looks a little weird but I couldn't find a better one
• calculating the longest line is not yet correct. 1) I'd have to convert tabs to spaces (again) before doing the calculation, but also, it only accounts for text, not for any meta data. I tried asking the span list for the longest line but that seems to trip on the selection which always has the full width. any ideas?

[extrawurst]

looks like a good start. but yeah it definitely misses the correct max-scroll pos. I did not have time to check the code yet. you can go for what ever solution as long as it is accurate and cache the result if it is potentially expensive (it would scale with the length of the diff) as long as the diff is not updated

[extrawurst]

are you still going to work on this?

[M1cha]

At least not within the next few weeks. So if anyone else wants to take over feel free. If not and I feel like continuing this I'll say so here.

[cruessler]

@extrawurst, @M1cha I have a few relatively minor changes locally that address the issues I have found so far, but I don’t have push access to this PR. I can open a separate PR from my fork and link to this one there. Is that ok?

cruessler/diff-horizontal-scroll

[M1cha]

that'd make sense. btw the longest_line calculation was insane. It's better to either use for loops like this:

self.longest_line = 0;
if let Some(diff) = self.diff.as_ref() {
	for hunk in &diff.hunks {
		for line in &hunk.lines {
			self.longest_line = self.longest_line.max(line.content.len());
		}
	}
}

or something a little more functional:

self.longest_line = self
	.diff
	.iter()
	.flat_map(|diff| diff.hunks.iter())
	.flat_map(|hunk| hunk.lines.iter())
	.map(|line| line.content.len())
	.max()
	.unwrap_or(0);

I pulled these snippets out of old chats with co-workers about this and didn't verify if those solutions still match what's currently on my branch.

[cruessler]

@M1cha Thanks for the hint, I was able to incorporate your suggestion into the new PR!

[extrawurst]

closed in favour of #1327