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

Merged
merged 6 commits into from Nov 11, 2014

Conversation

Projects
None yet
3 participants
@tomaswolf
Contributor

tomaswolf commented Oct 27, 2014

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.

@tomaswolf tomaswolf referenced this pull request Oct 27, 2014

Closed

Improve the commitdiff. #225

Show outdated Hide outdated src/main/java/com/gitblit/utils/GitBlitDiffFormatter.java
inFile = false;
}
sb.append(MessageFormat.format("<div class='header'><div class=\"diffHeader\" id=\"{0}\"><i class=\"icon-file\"></i> ", line)).append(line)

This comment has been minimized.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Doesn't line need to be run through StringUtils.escapeForHtml() here?

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Doesn't line need to be run through StringUtils.escapeForHtml() here?

Show outdated Hide outdated src/main/java/com/gitblit/utils/GitBlitDiffFormatter.java
String path = StringUtils.escapeForHtml(s.getKey(), false);
String comment = s.getValue();
if (comment != null) {
sb.append("<span id='" + path + "'>" + path + ' ' + StringUtils.escapeForHtml(comment, false) + "</span>");

This comment has been minimized.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Just noticed that StringUtils.escapeForHtml() does not replace ' by &apos;. So either escapeForHtml() should do so (after all, the pages are declared as XHTML, so using &apos; should be fine), or I need to change the quoting for the id attribute here to use double quotes.

Which do you prefer?

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Just noticed that StringUtils.escapeForHtml() does not replace ' by &apos;. So either escapeForHtml() should do so (after all, the pages are declared as XHTML, so using &apos; should be fine), or I need to change the quoting for the id attribute here to use double quotes.

Which do you prefer?

Show outdated Hide outdated src/main/java/com/gitblit/utils/GitBlitDiffFormatter.java
String path = StringUtils.escapeForHtml(s.getKey(), false);
String comment = s.getValue();
if (comment != null) {
sb.append("<span id=\"" + path + "\">" + path + ' ' + StringUtils.escapeForHtml(comment, false) + "</span>");

This comment has been minimized.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

I'm not too happy about fragment generation in CommitDiffPage.java and the id generation here. In CommitDiffPage, you just use '#' + entry.path for the href of the link. So what does Wicket do with this if the path contains critical characters like ", ', or a blank? In HTML 5, ids must not contain blanks, and in XHTML, they're even more constrained (and in particular slashes are not allowed). Perhaps one should better use 'n' + DiffEntry.getNewId().name() (or getOldId() for deletions) in both places as fragments and ids; that would be guaranteed not to contain any problematic characters and to be a valid fragment or id according to any HTML specification.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

I'm not too happy about fragment generation in CommitDiffPage.java and the id generation here. In CommitDiffPage, you just use '#' + entry.path for the href of the link. So what does Wicket do with this if the path contains critical characters like ", ', or a blank? In HTML 5, ids must not contain blanks, and in XHTML, they're even more constrained (and in particular slashes are not allowed). Perhaps one should better use 'n' + DiffEntry.getNewId().name() (or getOldId() for deletions) in both places as fragments and ids; that would be guaranteed not to contain any problematic characters and to be a valid fragment or id according to any HTML specification.

This comment has been minimized.

@gitblit

gitblit Oct 28, 2014

Owner

Hmm. I'm not looking at the code right now, but off the top of my head...
doesn't DiffEntry.getNewId() return a SHA? That would break the relative
links for jumping to a specific path in the commitdiff. That's the reason
they are there. I'm ok with escaping the paths as needed - but the
escaping has to match in both places; the file list at the top & the diff
entry.

@gitblit

gitblit Oct 28, 2014

Owner

Hmm. I'm not looking at the code right now, but off the top of my head...
doesn't DiffEntry.getNewId() return a SHA? That would break the relative
links for jumping to a specific path in the commitdiff. That's the reason
they are there. I'm ok with escaping the paths as needed - but the
escaping has to match in both places; the file list at the top & the diff
entry.

This comment has been minimized.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Of course one would need to adapt also CommitDiffPage and ComparePage to use 'n' + entry.objectId for the fragment (that's two one-liner changes in each page). As far as I see those are the only places where page-internal links to the file diffs are generated.

When I wrote "in both places" I meant in those two pages, plus here in GitBlitDiffFormatter for the ids. I've actually tried this locally, and it works nicely, and simply side-steps all problems with strange characters in paths.

@tomaswolf

tomaswolf Oct 28, 2014

Contributor

Of course one would need to adapt also CommitDiffPage and ComparePage to use 'n' + entry.objectId for the fragment (that's two one-liner changes in each page). As far as I see those are the only places where page-internal links to the file diffs are generated.

When I wrote "in both places" I meant in those two pages, plus here in GitBlitDiffFormatter for the ids. I've actually tried this locally, and it works nicely, and simply side-steps all problems with strange characters in paths.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Oct 29, 2014

Contributor

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.

Contributor

tomaswolf commented Oct 29, 2014

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.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Oct 29, 2014

Owner

This all looks really good. Using the blob id is a good idea 👍 .

  • Let's change the background css color on the left gutter from f0f0f0 to fbfbfb.
  • Let's put some space between the diff text and the gutter's right border, 2-3 px maybe.

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.

Owner

gitblit commented Oct 29, 2014

This all looks really good. Using the blob id is a good idea 👍 .

  • Let's change the background css color on the left gutter from f0f0f0 to fbfbfb.
  • Let's put some space between the diff text and the gutter's right border, 2-3 px maybe.

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.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Oct 29, 2014

Contributor

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.

Contributor

tomaswolf commented Oct 29, 2014

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.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Oct 29, 2014

Owner

Ok, no problem. I'll take whatever help I can get. :) How about fefefe
for the context lines? I don't want to go pure white.

Owner

gitblit commented Oct 29, 2014

Ok, no problem. I'll take whatever help I can get. :) How about fefefe
for the context lines? I don't want to go pure white.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Oct 29, 2014

Contributor

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!

Contributor

tomaswolf commented Oct 29, 2014

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!

tomaswolf added some commits Oct 26, 2014

Improve the commitdiff.
* 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.)
Further diff improvements
- Add the new settings to gitblit.properties
- Highlight trailing whitespace
More diff page improvements
- 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.
CSS changes.
- 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.
Add min-width in .diff-line CSS
To ensure the line number columns never get squashed.
@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 8, 2014

Contributor

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:

  • image diffs (that would be your ticket 88), based on an improved version of Lea Verou's pure CSS image slider. That one is actually ready; I'm already using it in-house in production.
  • a table-less layout for diff listings that allows selecting only code with all indentation intact (in this patch here, indentation is still screwed up a bit because of the table rows and cells) and that introduces anchors for the diff lines. It also avoids the strange HTML combination of a pre-element containing divs and tables that exists currently on the diff pages and that is not allowed by no HTML definition I know. That this works at all is a minor miracle. I've done a fully functional mock-up of that table-less design. Interested?

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.

Contributor

tomaswolf commented Nov 8, 2014

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:

  • image diffs (that would be your ticket 88), based on an improved version of Lea Verou's pure CSS image slider. That one is actually ready; I'm already using it in-house in production.
  • a table-less layout for diff listings that allows selecting only code with all indentation intact (in this patch here, indentation is still screwed up a bit because of the table rows and cells) and that introduces anchors for the diff lines. It also avoids the strange HTML combination of a pre-element containing divs and tables that exists currently on the diff pages and that is not allowed by no HTML definition I know. That this works at all is a minor miracle. I've done a fully functional mock-up of that table-less design. Interested?

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.

@gitblit gitblit merged commit 6e55f53 into gitblit:develop Nov 11, 2014

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Nov 11, 2014

Owner

Thanks for your patience and the improvements! Fire away on any PR you think would be generally useful upstream.

Owner

gitblit commented Nov 11, 2014

Thanks for your patience and the improvements! Fire away on any PR you think would be generally useful upstream.

@tomaswolf tomaswolf deleted the tomaswolf:issue-450-develop branch Nov 11, 2014

@fzs fzs modified the milestone: 1.7.0 Mar 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment