@andreynering could you pull the latest and see if the problem is still there?
technically it was still side-by-side, just that the line was to wide and made it scroll (almost) forever ^.^ anyhow, I've made it line-wrap instead since it makes more sense for split-views :)

Now working as intended

@unknwon thoughts?

I think the button should be called Unified instead of inline.

@bkcsoft Did you commit this change? Because for me it is as before.

I think you should rebase (or squash?) your commits, since some commits from @unknwon is showing in this pull requests.

But I'd say this is working as expected.

@unknwon yeah, I think that makes most sense on second thought

@andreynering I've commited the changes, which web-browser are you testing in? (I'm currently only testing on firefox)
about the leaking commits, I rebased on develop and that seems to have borken something -. - I'll see if I can make it cleaner

Which web-browser are you testing in?

Chrome

@andreynering yeah, not getting that on firefox or chrome. sure you checked it out cleanly and built it correctly? (make build and all that...)

@unknwon Renamed and cleaned up a bit :)

@bkcsoft I tried in Firefox and it's OK. In Chrome it's still as before.

@andreynering which version if chrome? Since I canct reproduce it :/

47.0.2526.106

I'll have a look later today :)

@andreynering currently testing in "Google Chrome 47.0.2526.80" and it "Works On My Machine" ^.^
Can't really see how .80 and .106 breaks it, I'll try 48.x or 49.x and see if it's still working there :)

Thanks @bkcsoft !

Just get back from trip. Will start reviewing today. (I'm on +8 CST right now)

@andreynering odd, I tried it in "Google Chrome 47.0.2526.80" and there it worked as expected, but in "Google Chrome 47.0.2526.106" I see what you're seeing...

Anyhow, the issue is with word-wrapping... I wrap on whitespace, but there's no whitespace to wrap on :/

I am playing a little here, trying to highlight what changed in each line, like GitHub and GitLab do:

There are some few bugs, but I hope I can get this working 100%.

@andreynering well, I can make it either break in the middle of words, OR break on whitespace... apparently not both -.-

about highlighting changes, that might be really hard... for simple stuff it easy enough:

- foo()
+ bar()

but for a bit more complex stuff it is harder:

- foo()
- bar(1)
+ foo()
+ baz()
+ bar(2)

^ this one should highlight only the numbers (and perhaps baz()) 😄

@andreynering well, I can make it either break in the middle of words, OR break on whitespace... apparently not both -.-

unless one uses an undocumented css-value... namely word-break: break-word...
I'll commit and push the fix so you can try it 😄

I will try to get it right and maybe send a PR, but will wait this one be
merged first.

I am using a third party package that does the diff of the strings.

Em dom, 3 de jan de 2016 22:14, Kim Carlbäcker notifications@github.com
escreveu:

@andreynering https://github.com/andreynering well, I can make it
either break in the middle of words, OR break on whitespace... apparently
not both -.-

about highlighting changes, that might be really hard... for simple
stuff it easy enough:

• foo()
• bar()

but for a bit more complex stuff it is harder:

• foo()
• bar(1)
• foo()
• baz()
• bar(2)

^ this one should highlight only the numbers (and perhaps baz()) [image:
😄]

—
Reply to this email directly or view it on GitHub
#2296 (comment).

@andreynering yeah, I looked at diff-packages when I started this but it seemed like they wanted rad .patch-files (which I didn't wanna push to the frontend...) so I gave up on that...

Verified to work in the following browsers:
Chrome stable 47.0.2526.106
Firefox 43.0.1
Microsoft Edge 20.10240.16384.0 (that version-number xD
Internet Explorer 11.0.10240.16431
Opera 33.0.1990,115

@bkcsoft It's OK now. 👍

Oops! There are conflicts on bindata.go

PS: A rebase is recommended to keep commit history clean.

Thanks agin for the PR!

I checked out locally and works great AFA I can tell.

Some comments are made, also I see a lot duplicated content between diff_box_split.tmpl and diff_box_unified.tmpl, is it worth doing it or should we reduce the duplicated part?

@unknwon I've missed to remove to old code, I'll remove that and squash some commits to make it cleaner. Also resolve the bindata.go-issue

About the duplicate code, I made that decision because it'll only do 1 bool-check (and not 1 per line). My reasoning for that is that it should be faster to render, I can do some benchmarks if you'd like to see how much slower it'd be to do it inline. (just need to find a big commit first 😄)

About the duplicate code, I made that decision because it'll only do 1 bool-check (and not 1 per line). My reasoning for that is that it should be faster to render, I can do some benchmarks if you'd like to see how much slower it'd be to do it inline. (just need to find a big commit first 😄)

I don't mean to check bool per line, but reduce the check to a smaller range of content. As you can see, things are getting different inside tbody tag: https://github.com/gogits/gogs/pull/2296/files#diff-9d1f05e66c594694d453ec18a343782dR35

doing it on the tbody-tag would make it check IsSplitStyle on each listed file. but if that's acceptable I'll do that :)

merging them and having it check per-file (on tbody) didn't even make a dent in the render-time even for some of the larger commits I could find 😄

Oh, I completely forgot to mention that this PR also includes the fold-icon (opticon-fold)
Which is what this is doing: https://github.com/gogits/gogs/pull/2296/files#diff-f0b73ac65b702903d90a117166a9b06bR75 (down to row 88)

Like this:

Not sure if I should leave it there or have it in a separate PR?

Never mind, I'll do that after merge.

And you need at least comment out fold icon, it's not working yet.

Already pushed the template-merging :)

The icon is more of a visual thing for me. but I'll remove it and see if I can make it work in a separate PR :)

Oh, I see. I'll checkout again later today. 😀

LGTM.

Is there a flag to enable split-view by default?

@mastorm currently, no..