[yokurang]: Side-by-side diff view #32

yokurang · 2024-03-07T12:08:37Z

Implemented something like this

We toggle using t. Any thoughts on the design?

@panglesd @Julow

… implemented

panglesd · 2024-03-07T13:44:55Z

That is a very good start!

Some early comments on the usability:

It would be nice if the left and right areas were of always the same size: half of the available size
The common lines should be aligned, or even better, not duplicated!
We need to keep the + and - signs: the goal is to have an accessible interface, so we cannot rely on colors!

yokurang · 2024-03-07T14:44:20Z

Hi,

Can I clarify on what you mean by "The common lines should be aligned, or even better, not duplicated!", as I thought a side by side view still show common lines from both files? Thanks

Julow · 2024-03-11T16:20:53Z

Hi, sorry for the confusion. Yes the common lines should be duplicated at first. Other way of rendering this are for future issues.
However, the common lines should be aligned, for example this diff:

 FIRST
-a
+b
+b
 LAST

Should render to something like:

FIRST       FIRST
a           b
            b
LAST        LAST

In order to do this, it might be needed to make the split_hunk a bit smarter.

Perhaps by this approach: When you encounter a `Mine, collect all the `Mine that follows in a list, then the following `Their in an other list. If one list is smaller than the other, add empty lines to it.
If instead you encounter a `Their first, do the same but collect the `Their first then the `Mine.
The append these lists to mine_acc and their_acc as you are already doing.

You'd need to turn it into a recursive function instead of using a fold_left. You need to write a function that collects consecutive lines of a specified kind (`Mine or `Their). This function must return the remaining of the list to continue the iteration from the right place.

panglesd · 2024-03-18T13:12:49Z

Hello @yokurang !

Could you confirm that @Julow's explanation makes sense to you?

This PR is already pretty advanced!

yokurang · 2024-03-21T03:42:30Z

Hello @yokurang !

Could you confirm that @Julow's explanation makes sense to you?

This PR is already pretty advanced!

Hi, yes, the explanation makes sense. Sorry for the delay, I have been quite busy with school work and some interviews. I'm finishing up soon, so I'll have time to finish this PR soon! Sorry for the delay.

yokurang · 2024-03-22T06:37:38Z

Hi @Julow, I believe I modified split_hunk in the way you explained before. Could I ask you to verify is my solution is indeed correct?

Furthermore, @panglesd, I made the following progress:

I added + and - signs
I wanted to ask if you have any pointers for finding the total width of the viewbox so that I can calculate half the width and wrap the lines appropriately so the left and right areas are always of the same size. I just read up on layout_width from https://ocaml.org/p/nottui/0.3/doc/Nottui/Ui/index.html#val-layout_width. I haven't tried it yet, but wanted to ask if I am in the right direction.
With regards to the point that common lines should be aligned, or even better, not duplicated, can I clarify if we are not duplicating common lines, does this mean that common lines should only be displayed on one side of the side-by-side diff view? On that note, I am also assuming that the following screenshot is wrong?

Thank you

Julow

This is awesome ! Happy to see such a high quality contribution.

I wanted to ask if you have any pointers for finding the total width of the viewbox so that I can calculate half the width and wrap the lines appropriately so the left and right areas are always of the same size

Ui.resize should be used instead. ~w and ~h sets a fixed width and height, while ~sw and ~sh sets how the elements should share the remaining space, see layout_spec.

With regards to the point that common lines should be aligned, or even better, not duplicated, can I clarify if we are not duplicating common lines, does this mean that common lines should only be displayed on one side of the side-by-side diff view?

The common lines should be duplicated ! Sorry for the confusion.

node_modules/.lsp/debug.log

lib/interactive_viewer.ml

.gitignore

Julow · 2024-06-06T09:08:19Z

lib/interactive_viewer.ml

+           | `Their s -> (s, attr_change)
+         in
+         let padded_content =
+           content ^ String.make (max_width - String.length content) ' '


What is the goal of this padding ?

It's currently misbehaving and adding a variable amount of unwanted empty lines between diff lines. Though, I wonder if there's a way not to use padding.

I added padding initially as an attempt to make the spacing look better when text wrapping.. but since we want to forgo text wrapping at this moment I removed this (saved for later) as I don't think I'll be needing this without the text wrap function

Julow · 2024-06-06T09:16:33Z

lib/interactive_viewer.ml

+  | `Their s :: t ->
+      split_and_align_hunk t (`Common "" :: mine_acc) (`Their s :: their_acc)
+
+let wrap_text s =


Is wrapping needed to avoid having horizontal scrolling ? I think that horizontal scrolling is what we want in the end, so I'd suggest not to bother with line wrapping initially.

Currently, if the content is wider than the screen, it's truncated to the right. I think this is reasonable for the first version of the side-by-side view. Horizontal scrolling is something we'll want in the future, a lot of discussion happened in #45

Got it, I'll forgo text wrapping for now. Thank you !

Julow

Looks good to me :)

I think the output could be further improved but if you prefer, that could be done in a later PR.

Julow · 2024-06-06T15:01:00Z

lib/interactive_viewer.ml

-    | `Mine text -> W.string ~attr:Notty.A.(fg green) text
-    | `Common text -> W.string text
-  in
-  Ui.vcat @@ List.map line_to_string hunk.Patch.lines


This diff is an artifact of being based off #57, which has been reverted since. Could you rebase or merge main ?

Julow · 2024-06-06T15:16:39Z

lib/interactive_viewer.ml

+  | (`Common _ as common) :: t ->
+      split_and_align_hunk t (common :: mine_acc) (common :: their_acc)
+  | `Mine s :: `Their s' :: t ->
+      split_and_align_hunk t (`Mine s :: mine_acc) (`Their s' :: their_acc)


This pairs only one added line with one removed line. This might not be the output we expect, for example:

18 11 W.scrollbox 19 12 (W.vbox 20 13 [ 21 - pure_str "Hello world!"; 22 - string_of_counter counter_d; 14 + pure_str (string_of_operation patch.operation); 23 15 Lwd.pure 24 16 @@ Ui.keyboard_area

This diff renders like this in side-by-side:

W.scrollbox W.scrollbox (W.vbox (W.vbox [ [ pure_str "Hello world!"; string_of_counter counter_d; pure_str (string_of_operation patch.operation); Lwd.pure Lwd.pure @@ Ui.keyboard_area @@ Ui.keyboard_area

It would be more intuitive if the whole block of removed line was paired with the added line.
Visually aligning the first removed line of the block with the first added line, leaving the empty line in the right side to be below the green line instead of above.

yokurang · 2024-06-06T15:19:48Z

I think I'll leave that for a separate PR.. permission to merge sir? 🫡

Julow · 2024-06-06T16:32:04Z

You can go ahead. I'd prefer a "Squash and merge" to make the history more regular.

yokurang added 2 commits March 7, 2024 20:05

[yokurang]: implemented something that works, but scrolling still not…

d3c82af

… implemented

[yokurang]: implemented something that works, but scrolling still not…

577f92d

… implemented

panglesd changed the title ~~[yokurang]: implemented something that works~~ [yokurang]: Side-by-side diff view Mar 18, 2024

yokurang added 3 commits March 22, 2024 13:10

resolve merge conflict

0905590

progress

74b46b1

progress

f44310a

Julow mentioned this pull request Mar 22, 2024

Show line numbers in side-by-side diffs #49

Closed

Julow reviewed Mar 22, 2024

View reviewed changes

yokurang added 12 commits March 25, 2024 22:32

progress: issue with I need to wrap text when test becomes too long

c5e2898

dune fmt

95a8882

need to substitute max_width using Ui.layout_spec or Ui.layout_width

2cfbf01

is this a good fix?

d80fb25

Merge branch 'main' into side-by-side-diff-view

abc0236

is this a good fix?

6cd4a6d

adding togglable lines

84fd92a

fixed using Ui resize

a61793a

added colours to normal mode

ba026a6

progress

a02222c

progress

5d7682d

needs work

0f7a3de

Julow reviewed Jun 6, 2024

View reviewed changes

yokurang added 3 commits June 6, 2024 20:45

redid help panel

fb70338

progress

9051a34

please clean up code

d8feae8

yokurang added 2 commits June 6, 2024 22:11

please clean up code

c2d492e

cleaned and fixed code according to discussion

43bffe6

Julow approved these changes Jun 6, 2024

View reviewed changes

yokurang merged commit f7293a6 into panglesd:main Jun 7, 2024
2 checks passed

yokurang deleted the side-by-side-diff-view branch June 9, 2024 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[yokurang]: Side-by-side diff view #32

[yokurang]: Side-by-side diff view #32

yokurang commented Mar 7, 2024 •

edited

Loading

panglesd commented Mar 7, 2024

yokurang commented Mar 7, 2024

Julow commented Mar 11, 2024

panglesd commented Mar 18, 2024

yokurang commented Mar 21, 2024

yokurang commented Mar 22, 2024

Julow left a comment

Julow Jun 6, 2024

yokurang Jun 6, 2024

Julow Jun 6, 2024

yokurang Jun 6, 2024

Julow left a comment

Julow Jun 6, 2024

Julow Jun 6, 2024

yokurang commented Jun 6, 2024

Julow commented Jun 6, 2024

[yokurang]: Side-by-side diff view #32

[yokurang]: Side-by-side diff view #32

Conversation

yokurang commented Mar 7, 2024 • edited Loading

panglesd commented Mar 7, 2024

yokurang commented Mar 7, 2024

Julow commented Mar 11, 2024

panglesd commented Mar 18, 2024

yokurang commented Mar 21, 2024

yokurang commented Mar 22, 2024

Julow left a comment

Choose a reason for hiding this comment

Julow Jun 6, 2024

Choose a reason for hiding this comment

yokurang Jun 6, 2024

Choose a reason for hiding this comment

Julow Jun 6, 2024

Choose a reason for hiding this comment

yokurang Jun 6, 2024

Choose a reason for hiding this comment

Julow left a comment

Choose a reason for hiding this comment

Julow Jun 6, 2024

Choose a reason for hiding this comment

Julow Jun 6, 2024

Choose a reason for hiding this comment

yokurang commented Jun 6, 2024

Julow commented Jun 6, 2024

yokurang commented Mar 7, 2024 •

edited

Loading