Allow to scroll diffs horizontally

[cruessler]

This Pull Request fixes/closes #1017.

This is an attempt to finish the work started by @M1cha. It fixes a few minor issues, makes clippy pass and was rebased onto the latest master.

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 (an item was already added)

[extrawurst]

that looks way better!

one thing is left that bothers: the hunk header also scrolls. can we keep that static?

[M1cha]

I kinda liked that it does so you can see the whole thing in case it doesn't fit the screen.

[cruessler]

I think I agree with @M1cha. I like that you can see each hunk’s header, and I imagine scrolling might feel broken if you weren’t able to do that.

[extrawurst]

ok cool with me. I keep scrolling left and rush up the file tree in the end :( on the other hand I really do not want to introduce a special horizontal scrolling key binding... any ideas?

[M1cha]

can we see the difference between keyrepeat and keypress? if yes, disabling leaving the view through keyrepeat might at least allow to scroll to the very left by holding down the arrow-keys without the risk of accidentally leaving the view.

[extrawurst]

unfortunately this is only supported in a very limited amount of terminals: https://github.com/crossterm-rs/crossterm/pull/688

[extrawurst]

I lean towards making this more explicit and going into a fullscreen diff view (no file tree on the left anymore) when starting to scroll right and only allowing to leave that one via esc

[M1cha]

sounds good to me

[cruessler]

Sounds good to me, too! I’ll try to update this PR next week, probably Tuesday or Wednesday. Would we still have the diff view to the right as long as it doesn’t have focus?

[extrawurst]

Yeah exactly! Thanks ❤️

[cruessler]

DiffComponent is used in 3 places, InspectCommitComponent, CompareCommitsComponent, and FileRevlogComponent. I changed all 3 parent components to have the diff full screen as soon as it gets focus. Is this the right direction? Then we could maybe start fine-tuning and polishing. What do you think?

[extrawurst]

yeah all places behaving the same sounds reasonable. after all we won't change the current behaviour we just add the "press right to open in fullscreen" option from each

[extrawurst]

@cruessler any blockers on this?

[extrawurst]

@cruessler 🥺

[cruessler]

@extrawurst I believe I was waiting for feedback after my last comment. I will check (and rebase in the process)! Sorry for not responding!

[cruessler]

@extrawurst I’ve rebased the PR! I briefly checked that horizontal scrolling works by making my terminal smaller. The diff view is now full-screen, so that scrolling is required in fewer cases. It closes when you’re at scroll position 0 and press left arrow. Let me know what’s missing!

[extrawurst]

this looks good already just a few small improvements:

• lets have the same behaviour in the status tab when moving right on a (horizontally-)scrollable diff
• lets not use the left key to leave the fullscreen, this happens weirdly when holding down the left-arrow
• use esc like in other places to leave the fullscreen again
• lets show the scrollbar already once the scrollable diff is focused

[cruessler]

@extrawurst In the status tab, do we want to keep the current behaviour of having the diff not go fullscreen on focus? I think it makes sense to make it consistent with the other places DiffComponent is used and also go fullscreen.

[extrawurst]

works better now IMO.

in the status tab I would say lets go to fullscreen once the right command is pressed to scroll and there is actually something to scroll.

[cruessler]

works better now IMO.

in the status tab I would say lets go to fullscreen once the right command is pressed to scroll and there is actually something to scroll.

@extrawurst If I understand the architecture correctly, this is not that easy to accomplish. The code that knows whether a scrollbar is necessary, lives in DiffComponent while the code that decides how much space to give to the DiffComponent lives in Status. The state “would a scrollbar be drawn” is currently not easily shareable it seems. Do you have a solution for that issue? Am I overlooking something?

[extrawurst]

@cruessler yeah you are right. lets go to fullscreen on right either way

[cruessler]

@extrawurst I updated the Status tab, rebased and briefly checked that everything still works. As far as I can see, the Status tab was the last thing in your list of feedback.

[extrawurst]

It works really well as far as I can see! Will review later and merge 🥳

[cruessler]

just a very small thing I would prefer to see cleaned up. @cruessler let me know if you got time for it shortly otherwise we can merge and clean them up later

Thanks for the suggestion! I can most likely address it over the weekend.

[extrawurst]

Thank you so much