-
Notifications
You must be signed in to change notification settings - Fork 670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the commitdiff. #226
Conversation
inFile = false; | ||
} | ||
|
||
sb.append(MessageFormat.format("<div class='header'><div class=\"diffHeader\" id=\"{0}\"><i class=\"icon-file\"></i> ", line)).append(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't line
need to be run through StringUtils.escapeForHtml()
here?
ada195e
to
3868411
Compare
I've rebased this onto the latest head from gitblit:develop, and have also changed the fragment/id generation to use the SHA instead of the path. |
This all looks really good. Using the blob id is a good idea 👍 .
It looks like you've started working on in-line diff highlighting. That is much bueno. Feel like trying to complete that work? That has long been on my todo list and would make the diff page much more useful. |
Thanks! About the background color: that would give the gutter (I presume that's the columns with the line numbers and +/-) the same background as the code column (context lines). Is that what you intend? I'm all for making the gutter a bit lighter (improves the contrast with the text color), but I'd then simply give the code column a white background. What do you think? A little padding on the code column is a good idea. No, I had not planned to do full intraline diffs; it's more work than I have time for. Doing that is not trivial; just take a look at what Gerrit does. Highlighting trailing whitespace is. |
Ok, no problem. I'll take whatever help I can get. :) How about fefefe |
Ok, fefefe it is. Though I can't see the difference from white. Can you really see a 1-bit difference in each channel? You must have eagle eyes! |
* Optimize CSS: simplify selectors. That alone cuts rendering time in half! * Adapt HTML generation accordingly. * Change line number generation so that one can select only code lines. Also move the +/- out of the code column; it also gets in the way when selecting. * Omit long diffs altogether. * Omit diff lines for deleted files, they're not particularly interesting. * Introduce a global limit on the maximum number of diff lines to show. * Supply translations for the languages I speak for the new messages. https://code.google.com/p/gitblit/issues/detail?id=450 was about a diff with nearly 300k changed lines (with more then 3000 files deleted). But one doesn't have to have such a monster commit to run into problems. My FF 32 become unresponsive for the 30+ seconds it takes it to render a commitdiff with some 30000 changed lines. (90% of which are in two generated files; the whole commit has just 20 files.) GitHub has no problems showing a commitdiff for this commit, but omits the two large generated files, which makes sense. This change implements a similar thing. Files with too many diff lines get omitted from the output, only the header and a message that the diff is too large remains. Additionally, there's a global limit on the length of a commitdiff; if we exceed that, the whole diff is truncated and the files not shown are listed. The CSS change improves performance by not using descendant selectors for all these table cells. Instead, we assign them precise classes and just use that in the CSS. The line number generation thing using data attributes and a :before selector in the CSS, which enables text selections only in the code column, is not strictly XHTML 1.0. (Data attributes are a feature of HTML 5.) However, reasonably modern browsers also handle this correctly if the page claims to be XHTML 1.0. Besides, the commitdiff page isn't XHTML compliant anyway; I don't think a pre-element may contain divs or even tables. (Note that this technique could be used on other diff pages, too. For instance on the blame page.)
- Add the new settings to gitblit.properties - Highlight trailing whitespace
- Use git object ids as fragments and HTML element ids - Simplify generation: don't parse the diff line, instead generate the table header from the DiffEntry when we process it, and just skip the diff lines.
- As discussed: - gutter a little lighter, context lines nearly but not quite white. - 2px left (and right) padding in the code column. - I also noticed that somehow all lines were spaced vertically a little wider than on dev.gitblit. Added cellpadding='0' to get the old line height again.
31b1442
to
8fa8cbc
Compare
To ensure the line number columns never get squashed.
From my point of view this change is done and complete. I don't want to pack more into this, but if you see things that should still be fixed in this, I can do so, of course. I have two further improvements in the works that build upon this one:
If you're interested, I can create PRs for those, too, but I'd like to see this one resolved first; otherwise I'll end up in rebase hell because they all touch common files. |
Thanks for your patience and the improvements! Fire away on any PR you think would be generally useful upstream. |
Same as PR-225, but against gitblit:develop.
Width of the line number columns reduced to 2px padding and 3em width. On my screen the three columns (two line numbers and +/-) are 95px, vs. the two line number columns at dev.gitblit at 82px.
Sorry for the duplicate PR, but GitHub didn't let me rewrite the original PR (which was based on master). I just hope I didn't mess up somewhere in this rebase confusion.