diff view on commit with parallel diff view

[Popl7]

in answer to request:
http://feedback.gitlab.com/forums/176466-general/suggestions/3788060-side-by-side-diff-view

I have made an parallel diff view on the commit show page.
Below are 2 screenshots.
It works with the big commit message.
Notes per line are still too wide; If someone has tips for this, please let me know.

Please let me know if this is what the view should look like.

[bringhurst]

I like it -- great work! A modification to this for improving readability might be to place line numbers next to each other. Here's an image to show you what I mean -- see how the left and right line numbers are both located in the center column? It makes it slightly easier for your eyes to compare the line numbers.

[Popl7]

thx.
Currently is only show the diffs, not the whole file.
I am working on that right now.
The center column is something for later ;-)
I agree this looks nice, but I am not sure how to make that right now.

[bringhurst]

Sounds good to me -- it's definitely a great start. Once the bulk of the work is committed, it makes it much easier to iterate changes like this.

[Popl7]

I have updated the branch to show full files.
Line comments should display correct also.

Hmm, it seems line comments placed aren't shown in the parallel view.

[Popl7]

ok. showing of the comments working too.

[coveralls]

Coverage remained the same when pulling 499ae4541079dea5fe416b2f2a06d40104380ff6 on Popl7:parallel-diffs-side-by-side into 4123c76 on gitlabhq:master.

[coveralls]

Coverage remained the same when pulling 499ae4541079dea5fe416b2f2a06d40104380ff6 on Popl7:parallel-diffs-side-by-side into 4123c76 on gitlabhq:master.

[Popl7]

hmm, the tests seem to give an error.
fixing...

[Popl7]

because tests failed in javascript, I have removed the line notes from the parallel view.
There was a selector used in a feature test which was duplicate.

Also I can't get the line numbers working in the center.
When lines are long they push the line numbers to the right.

[coveralls]

Coverage remained the same when pulling 78d6fd853c3c7cf6ba1c658468fcd5e8ef2558f7 on Popl7:parallel-diffs-side-by-side into 4123c76 on gitlabhq:master.

[Popl7]

Can I use a Html5 canvas in GL?

[MrKeiKun]

Can you include to the diff on right side the SHA-1 for it?

[Popl7]

Yes I think so.
I'll it and post it here

Edit:
hmm, I don't see where I can place this.
Isn't this the same as the parent_id that's at the top of the page?

Can you show me where this would fit in, in your opinion?

[Popl7]

I have -some- working lines, but I am not sure if this works ok.

This is how it looks:

[mmoll]

@Popl7 could ypu push your current code for testing? awesome that someone is working on this, BTW :)

[Popl7]

I have updated the code.
Please let me know how this works for you.
Currently there are no added tests for the view; I am not sure which to add.

The lines are optional.
They can be switched off in the _text_file.html.html file.
Please let me know what you find of these lines, and if these should be included or left out.

@mmoll thx. I started picking up some minor feature requests to get to know the code and opensource development in general.

[mmoll]

@Popl7 OK, in general it's working, but there are some glitches, have a look at those lines in the middle:

[Popl7]

Thx for the feedback!

hmm, that looks interesting ;-)

I am thinking of leaving the lines out for now, and focussing on tests for the views and working comments.
Due to the way these comments are implemented and tested, this is more difficult than I thought.

Is there anything that you would change or would like to see?

[mmoll]

One thing is what is displayed when a file gets deleted:

It would be a good idea, also for the unified diff, to just display "file was deleted" here.

The second thing is a bit related to the lines. I would like to see a offset for added/delete lines to get non-changed parts of the changed file aligned at the same height:

[Popl7]

Ok. Thx
Great comments.
Do you think line comments are important in this view?
They are nasty to fit in for me.
Without them the offset part is easier also.

Op 23 okt. 2013 om 23:23 heeft Michael Moll notifications@github.com het volgende geschreven:

One thing is what is displayed when a file gets deleted:

It would be a good idea, also for the unified diff, to just display "file was deleted" here.

The second thing is a bit related to the lines. I would like to see a offset for added/delete lines to get non-changed parts of the changed file aligned at the same height:

—
Reply to this email directly or view it on GitHub.

[mmoll]

Hm, comments would be especially nice if you use this view for reviewing merge requests, but I can see how this is making it difficult. IMHO, the GitLab devs should decide, which features are required for them to get it merged.

[Popl7]

I have the line numbers working.
Comments work partially:

• you can add comment to a commented line
• you can delete the comment (but it doesn't get deleted from unified view without reload)
• you can edit a comment (but it gets updated on the unified view)

I will push this version when my tests pass.
The comment link will follow soon hopefully.

Maybe someone can test the codeview to see if any quirks come up?
Any comment / tips are appreciated!

[coveralls]

Changes Unknown when pulling 6a73d1d41160c72ada05a2ffff2a5c8d738b08c8 on Popl7:parallel-diffs-side-by-side into * on gitlabhq:master*.

[mmoll]

The line offsets and "File was created/delete" notes work great here, this is on the right track 👍

[Popl7]

ah, nice to hear that! thx.

[natefinch]

my 2 cents - comments are critical in the diff view. That's where the money is - using this during a review of a pull request. Not being able to use this with comments really hamstrings the usefulness of the feature

[Popl7]

thx for the feedback Nate.
I will soldier on to get the comments working.
Is there anything else you are missing, or could be different / better?

[natefinch]

I'd prefer that if there are lines added or deleted, that they be lined up with white space the other side. Keep lines that match on the same line.

Something like this:

This is a screenshot from Beyond Compare.

I also think that showing file context would be really helpful. Often times you can't really understand a change unless you see it in the context of the whole file, or at least plus or minus a few lines. Making that optional is fine, letting people choose no context, partial context (+- N lines) or full file context. If it were me, I'd just make it full file by default, and only implement partial context if pushed on it. You can always ignore the extra info, but you can't get the extra context if it's not displayed.

[dzaporozhets]

@Popl7 looks good. Is it ready for review/merge?

[coveralls]

Coverage decreased (-15.03%) when pulling 856d408 on Popl7:parallel-diffs-side-by-side into c938833 on gitlabhq:master.

[dosire]

@Popl7 If I understand correctly the code as it is now in this MR has comments that do respect the left and right columns?

[Popl7]

no, comments don't (fully) work yet.
They are being shown either left or right and replying works.
Adding comments per line doesn't work, so this is disabled on this view.

[dosire]

@Popl7 Thanks for the comment and the screenshot. I'll ask @randx to have a look.

[dosire]

@Popl7 Dmitriy will have a look at this after 6.3 is out.

[dzaporozhets]

@Popl7 thank you!

[dzaporozhets]

@Popl7 fill name is Steven Thonus, right?

[Popl7]

thx.

yes, that's me.

[dzaporozhets]

@Popl7 good. Because of 8c7d342 :)

[dosire]

@Popl7 Congratulations on getting your second major feature merged!

[Popl7]

thank you!

[soukron]

Any chance to get it for 5.4 also?

[m4tthumphrey]

@soukron Definitely not. Why haven't you upgraded to 6.* ?

[soukron]

I wish I could upgrade, but we can't lose our projects in "root level". Maybe I have missed some changelog where it's possible to have it again.

[m4tthumphrey]

IIRC, lack of root was only removed recently..

Could you not move them to /root. Why do they need to stay as root?

[soukron]

We use GitLab in a big development group and all scripts, commands, documents are referring to git@git.localdomain.net:.git. It's impossible to move to a different schema for us. In the other side, new projects are created inside a group, but old ones need to stay as is.

I'll review if I can upgrade to any 6.x which allows lack of root. Any help is appreciate.

[soukron]

Hi @m4tthumphrey in the upgrade guide from 5.4 to 6.0 it mentions the deprecation of global namespaces and the need to move all projects to any namespace. This made me keep in 5.4.

I'll try to hack routes.rb manually to map those projects to non-root projects.

[dosire]

@soukron You are right, there is no way to keep the root namespace when upgrading.

[soukron]

Yeah @dosire thanks for the double-checking. I've checking routes.rb file and I'll to make some map for well-known projects to add the group before enabling Grack support. They're only 5.

[dosire]

@soukron OK, goodluck.

[the8472]

Nice feature, but with the default layout it's not really usable due to the fixed with. side by side halves the already restricted width even further -> long lines of code need lots of sidescrolling.

To compare with side by side diffs in an IDE: I can fullscreen them there.

[ghost]

nice features, but it doesn't work when the diff is too large

[pengbins]

can we try to embed reviewboard to gitlab?
The diff view of reviewboard is really powerfull.

[dosire]

@ottersxx Reviewboard has support for GitLab. Please describe what reviewboard feature you would like to see in GitLab.

[roygao94]

@Popl7 I'm using the current release(8.10.2), in the changes(diff) view, can I configure to view more lines above and below the changes?

[Popl7]

@roygao94 I worked on this feature some time ago.
It has been merged into GitLab and it is being maintained by the development team.
If you have any questions about it, please contact GitLab support directly.