Allow to scroll diffs horizontally #1327
Conversation
|
that looks way better! one thing is left that bothers: the hunk header also scrolls. can we keep that static? |
|
I kinda liked that it does so you can see the whole thing in case it doesn't fit the screen. |
|
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. |
|
ok cool with me. I keep scrolling left and rush up the file tree in the end :( |
|
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. |
|
unfortunately this is only supported in a very limited amount of terminals: crossterm-rs/crossterm#688 |
|
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 |
|
sounds good to me |
|
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? |
|
Yeah exactly! Thanks ❤️ |
|
|
|
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 |
|
@cruessler any blockers on this? |
|
@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
force-pushed
the
diff-horizontal-scroll
branch
from
7e92601 to
0d20806
Compare
|
@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! |
|
this looks good already just a few small improvements:
|
|
@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 |
|
works better now IMO. in the status tab I would say lets go to fullscreen once the |
cruessler
force-pushed
the
diff-horizontal-scroll
branch
from
45100a0 to
d3dd3a7
Compare
@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 |
|
@cruessler yeah you are right. lets go to fullscreen on |
the diff viewer will need it in a future commit
converting tabs to spaces had to be moved from after the formatting to before it because `trim_offset` seems to remove leading tabs from the output string (not the input string) for some reason. calculating the longtest line is cached to prevent a theoretical decrease in performance.
Consume left arrow in `DiffComponent` even when `scrolled_right` is already 0.
Use Esc to close focused diff
cruessler
force-pushed
the
diff-horizontal-scroll
branch
from
d3dd3a7 to
51d7eb9
Compare
|
@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. |
|
It works really well as far as I can see! Will review later and merge 🥳 |
There was a problem hiding this comment.
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. |
|
Thank you so much |
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:
make checkwithout errorsFeedback 2022-11-27
#1327 (comment)