Skip to content
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
Merged

Conversation

tomaswolf
Copy link
Contributor

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.

inFile = false;
}

sb.append(MessageFormat.format("<div class='header'><div class=\"diffHeader\" id=\"{0}\"><i class=\"icon-file\"></i> ", line)).append(line)
Copy link
Contributor Author

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?

@tomaswolf
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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.
To ensure the line number columns never get squashed.
@tomaswolf
Copy link
Contributor Author

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-org:develop Nov 11, 2014
@gitblit
Copy link
Collaborator

gitblit commented Nov 11, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants