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

Image diffs #229

Merged
merged 9 commits into from Nov 17, 2014

Conversation

Projects
None yet
3 participants
@tomaswolf
Contributor

tomaswolf commented Nov 11, 2014

Ticket 88

UI based on Lea Verou's pure CSS slider.

  • Add a callback interface, pass it through DiffUtils to the GitBlitDiffFormatter. Is needed because the rendering needs access to the repositoryName and other things that are known only at higher levels.
  • New class ImageDiffHandler responsible for rendering an image diff. Called for all binary diffs, doesn't do anything if it's not an image. HTML is generated via JSoup: no worries about forgetting to close a tag, nor about HTML escaping, nor about XSS.
  • The 3 diff pages set up such an ImageDIffHandler and pass it along.
  • CSS changes: from Lea Verou, with some minor improvements (ability to slide all the way to the left).

I think in the long run there'll be no way around rewriting the HTML diff formatter from scratch, not using the standard JGit DiffFormatter at all. For now, it's still manageable (even though the binary file interception is a bit a of a dirty trick), but if one ever wanted intraline diffs, there'll be no way around that.

Image diffs
Ticket 88: https://dev.gitblit.com/tickets/gitblit.git/88

Based on Lea Verou's pure CSS slider:
http://lea.verou.me/2014/07/image-comparison-slider-with-pure-css/

* Add a callback interface, pass it through DiffUtils to the
  GitBlitDiffFormatter. Is needed because the rendering needs access
  to the repositoryName and other things that are known only at higher
  levels.
  
* New class ImageDiffHandler responsible for rendering an image diff.
  Called for all binary diffs, doesn't do anything if it's not an
  image. HTML is generated via JSoup: no worries about forgetting to
  close a tag, not about HTML escaping, nor about XSS.
  
* The 3 diff pages set up such an ImageDIffHandler and pass it along.

* CSS changes: from Lea Verou, with some minor improvements.

I think in the long run there'll be no way around rewriting the
HTML diff formatter from scratch, not using the standard JGit
DiffFormatter at all.
@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Nov 11, 2014

Owner

Really nice! Is there a way to control the opacity of the overlay? I have an older image which is smaller than a new image. The older image is overlayed on top of the newer image with the new image peeking through beneath the old new image.

You are probably well aware of how Github displays this, but here is the commit I am using to test and compare with Github:
gitblit/ninja@948eeb4?short_path=04a27bd#diff-6

Owner

gitblit commented Nov 11, 2014

Really nice! Is there a way to control the opacity of the overlay? I have an older image which is smaller than a new image. The older image is overlayed on top of the newer image with the new image peeking through beneath the old new image.

You are probably well aware of how Github displays this, but here is the commit I am using to test and compare with Github:
gitblit/ninja@948eeb4?short_path=04a27bd#diff-6

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 11, 2014

Contributor

Only with Javascript, AFAIK. Not with CSS alone. We'd want a second slider to control the opacity, but unless one starts using HTML5 range inputs, I don't readily see how to read its setting (CSS resize does not trigger proper events, apparently). I haven't found yet a small, self-contained JavaScript implementation. Or do you happen to know one? Maybe there's some jQuery plugins; GitBlit already does have jQuery, but we'd probably want one that doesn't drag in all of jQuery UI. I'll look around a bit.

Also, got any idea how to best deal with really small images? (And how to get sizes of images in pixels without reading the whole file, so that one can know quickly whether an image is small?)

In any case, it's rather easy with this refactoring to replace the UI by something more sophisticated, as it's wholly contained in that ImageDiffHandler which lives at a level where you can actually add scripts to the page and do other fancy stuff.

Contributor

tomaswolf commented Nov 11, 2014

Only with Javascript, AFAIK. Not with CSS alone. We'd want a second slider to control the opacity, but unless one starts using HTML5 range inputs, I don't readily see how to read its setting (CSS resize does not trigger proper events, apparently). I haven't found yet a small, self-contained JavaScript implementation. Or do you happen to know one? Maybe there's some jQuery plugins; GitBlit already does have jQuery, but we'd probably want one that doesn't drag in all of jQuery UI. I'll look around a bit.

Also, got any idea how to best deal with really small images? (And how to get sizes of images in pixels without reading the whole file, so that one can know quickly whether an image is small?)

In any case, it's rather easy with this refactoring to replace the UI by something more sophisticated, as it's wholly contained in that ImageDiffHandler which lives at a level where you can actually add scripts to the page and do other fancy stuff.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 11, 2014

Contributor

Haven't found anything so far that I really liked. But... with a few lines plain JavaScript (even less if I use jQuery) and a little more CSS I can produce this: screenshot_imagediff

Is that about what you had in mind? It isn't the same as GitHub's, but the implementation is also much simpler and doesn't look too bad, I think. (P.S.: the Javascript triggers on plain old scroll events of that slider, so no CSS resize stuff needed for that.)

Contributor

tomaswolf commented Nov 11, 2014

Haven't found anything so far that I really liked. But... with a few lines plain JavaScript (even less if I use jQuery) and a little more CSS I can produce this: screenshot_imagediff

Is that about what you had in mind? It isn't the same as GitHub's, but the implementation is also much simpler and doesn't look too bad, I think. (P.S.: the Javascript triggers on plain old scroll events of that slider, so no CSS resize stuff needed for that.)

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 11, 2014

Contributor

P.S.: a red-green gradient may not be ideal for people with red-green blindness, but similar problems exist in the web UI in several places already. (For instance, lines added/removed.) An alternative might be a red to white gradient, but that's somewhat less indicative for people with normal eyesight.

Contributor

tomaswolf commented Nov 11, 2014

P.S.: a red-green gradient may not be ideal for people with red-green blindness, but similar problems exist in the web UI in several places already. (For instance, lines added/removed.) An alternative might be a red to white gradient, but that's somewhat less indicative for people with normal eyesight.

tomaswolf added some commits Nov 12, 2014

Support for adding bottom scripts
Needed if we want to have opacity changes in image diffs because
jQuery is bottom-loaded, so we must ensure that any scripts using
jQuery are run later.

I'm not a Wicket expert; maybe there's a cleverer or cleaner way to do
this. There is a JavascriptUtils class in Wicket, but that writes to
the response -- I don't quite see how that would give me control over
the precise placement of the scripts to ensure they come after that
bottom-loaded jQuery.
HTML bug fix on the blob page
That blob page sent *two* body tags. Now that we have bottom scripts,
we can fix that easily: don't try to set body.onload, but run the
prettyprinting through a bottom script on jQuery's document.ready.
Opacity adjustments for image diffs
* ImageDiffHandler adds the slider; styled in gitblit.css
* imgdiff.js is a little bottom-loaded Javascript that adjusts the
  opacity on sliders' scroll events.
* The three diff pages add this bottom script to the page if needed
* GitBlitDiffFormatter: center image diffs.
@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 13, 2014

Contributor

It appears that this opacity adjustment is not quite correct yet. Worked for me yesterday, today on another machine with Firefox 33.1, this doesn't work at all anymore. Will have to devise something else for that opacity bit.

Contributor

tomaswolf commented Nov 13, 2014

It appears that this opacity adjustment is not quite correct yet. Worked for me yesterday, today on another machine with Firefox 33.1, this doesn't work at all anymore. Will have to devise something else for that opacity bit.

Fix that opacity slider
Using the browser's built-in slider doesn't work if the browser hides
scrollbars (like Firefox on Mac). So,construct our own slider with
three divs and some CSS. Event-handling Javascript changed to match
this new implementation.
@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 13, 2014

Contributor

Now that works better. Too stupid, I had forgotten about some browsers hiding scrollbars, which of course breaks any attempt to use a built-in scrollbar as a kind of range input. I hate that behavior and have set my browser to always show them, which is why I didn't notice that right away.

Anyway, I replaced that first attempt by a CSS-resize based slider. Otherwise, a more complete and more complicated Javascript based slider would need to be used. Or some completely different kind of control.

Contributor

tomaswolf commented Nov 13, 2014

Now that works better. Too stupid, I had forgotten about some browsers hiding scrollbars, which of course breaks any attempt to use a built-in scrollbar as a kind of range input. I hate that behavior and have set my browser to always show them, which is why I didn't notice that right away.

Anyway, I replaced that first attempt by a CSS-resize based slider. Otherwise, a more complete and more complicated Javascript based slider would need to be used. Or some completely different kind of control.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 14, 2014

Contributor

I'm still not completely happy with this. Opacity complicates matters quite a bit. What are your plans for Gitblit -- remain basically static, only very few Javascript, or would it be OK to do both sliders completely in Javascript (only styling in CSS)? If that was fine, I could re-build something more like GitHub's. I have a working prototype for that. Looks like this:

screen shot 2014-11-14 at 16 42 55

The vertical bar sliding horizontally is initially at the far left, so the new image is shown. If the bottom slider is moved while the old image is not visible at all, that vertical bar is moved all the way to the right (thus showing the old image) and opacity is adjusted. If both images are visible (as in the image above), the bottom slider only adjusts the opacity of the overlay. A bit like GitHub's, but in one widget and no need to switch "tabs".

It's about twice as much CSS as now, and about 100 lines jQuery JavaScript with no other dependencies.

So... stay basically CSS-based relying on CSS-resize and use only a tiny little script, or be bolder and do the sliding via Javascript?

Contributor

tomaswolf commented Nov 14, 2014

I'm still not completely happy with this. Opacity complicates matters quite a bit. What are your plans for Gitblit -- remain basically static, only very few Javascript, or would it be OK to do both sliders completely in Javascript (only styling in CSS)? If that was fine, I could re-build something more like GitHub's. I have a working prototype for that. Looks like this:

screen shot 2014-11-14 at 16 42 55

The vertical bar sliding horizontally is initially at the far left, so the new image is shown. If the bottom slider is moved while the old image is not visible at all, that vertical bar is moved all the way to the right (thus showing the old image) and opacity is adjusted. If both images are visible (as in the image above), the bottom slider only adjusts the opacity of the overlay. A bit like GitHub's, but in one widget and no need to switch "tabs".

It's about twice as much CSS as now, and about 100 lines jQuery JavaScript with no other dependencies.

So... stay basically CSS-based relying on CSS-resize and use only a tiny little script, or be bolder and do the sliding via Javascript?

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Nov 14, 2014

Owner

I'm ok with your bolder proposal. I may rely on you to maintain it. :) Can you get by with the jquery version in Gitblit?

Owner

gitblit commented Nov 14, 2014

I'm ok with your bolder proposal. I may rely on you to maintain it. :) Can you get by with the jquery version in Gitblit?

Javascript-based sliders styled with CSS
This works better for small images. The previous CSS-resize based
attempt worked reasonably well, but had two problems on WebKit
(Safari):

1. For very small images the red resize handle would overlap the image
   itself. In that case, the image became un-draggable as soon as the
   opacity was reduced below 1.0.
   
2. Safari apparently doesn't send mousemove events during a CSS
   resize, so the opacity was changed only on mouseup.
   
Both observed on Safari 6.1.6 and 7.1. FF 33.1 had no problems.

Therefore I've switched to a Javascript slider. Since I didn't find
any that was simple, did not require HTML 5, appeared to be well
maintained, had a bug tracker and not too many outstanding bug reports,
didn't pull in umpteen other dependencies, didn't suffer from feature
bloat, was compatible with jQuery 1.7.1, and was freely licensed, I
ended up writing my own.

imgdiff.js contains a small Javascript slider (only horizontal) that is
styled completely in CSS. It reports ratios in the range [0..1] and
fires nice jQuery events 'slider:pos' on value changes. Base element
is a plain div that is positioned. It's not a general-purpose do-it-all
slider, but it's small, simple, and works for what we need it.

(imgdiff.js also sets up the ese sliders on the diff pages.)
@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 14, 2014

Contributor

Barely :-) (I mean the jQuery 1.7.1. Actually, it's no problem so far.) Do you have a PC available to test? I don't, so I cannot test on IE. I did test with all the browsers I have at my disposition: FF 33, Safari 6.1.6 and 7.1, Chrome 38. But those are the "good guys" that mostly do things right. Who knows what'll happen on IE!

Contributor

tomaswolf commented Nov 14, 2014

Barely :-) (I mean the jQuery 1.7.1. Actually, it's no problem so far.) Do you have a PC available to test? I don't, so I cannot test on IE. I did test with all the browsers I have at my disposition: FF 33, Safari 6.1.6 and 7.1, Chrome 38. But those are the "good guys" that mostly do things right. Who knows what'll happen on IE!

tomaswolf added some commits Nov 15, 2014

Minor corrections in Javascript
1. Stop running animation before starting a new one.
2. Fix ratio in animation
3. Fix div width

None of these change have any visible effect in the current use of
this script. (1) is just being safe, in (2) , the wrongly calculated
value was never used,and in (3), the div was a little too wide before.
Make the sliders clickable.
Always having to drag is cumbersome. Now the slider's handle can also
be set by clicking on the slider.

Heh :-) I see the GitHub UI designers hadn't thought of that.
@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 15, 2014

Contributor

Done from my point of view. (Pending a quick test on IE...)

Contributor

tomaswolf commented Nov 15, 2014

Done from my point of view. (Pending a quick test on IE...)

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Nov 16, 2014

Owner

That is looking very awesome! Haven't tested on IE yet. Any possibility of separating the modes - swipe vs opacity - like Github does? They use some sort of tabbed-like experience.

Owner

gitblit commented Nov 16, 2014

That is looking very awesome! Haven't tested on IE yet. Any possibility of separating the modes - swipe vs opacity - like Github does? They use some sort of tabbed-like experience.

@tomaswolf

This comment has been minimized.

Show comment
Hide comment
@tomaswolf

tomaswolf Nov 16, 2014

Contributor

About separating: of course that's possible. Nearly anything is. :-) But I wouldn't want to do so. I actually think this is more user-friendly than the GitHub widget. It gives you both controls without the need to switch tabs, and since it combines the functionality in one widget gives you possibilities you just don't have in GitHub. For instance moving the top slider to the middle and then reducing opacity of the overlay. The only thing missing would be a side-by-side view, but personally I can live without that, and if at all, it should be done in some later change.

I also think the UI is pretty self-explanatory. A new user might at first be puzzled, but after the first click on any slider, he'll immediately realize what this about and how it works. And there's no need for new localizable labels. (I've always found "Onion skin" a very strange name.)

Contributor

tomaswolf commented Nov 16, 2014

About separating: of course that's possible. Nearly anything is. :-) But I wouldn't want to do so. I actually think this is more user-friendly than the GitHub widget. It gives you both controls without the need to switch tabs, and since it combines the functionality in one widget gives you possibilities you just don't have in GitHub. For instance moving the top slider to the middle and then reducing opacity of the overlay. The only thing missing would be a side-by-side view, but personally I can live without that, and if at all, it should be done in some later change.

I also think the UI is pretty self-explanatory. A new user might at first be puzzled, but after the first click on any slider, he'll immediately realize what this about and how it works. And there's no need for new localizable labels. (I've always found "Onion skin" a very strange name.)

@gitblit gitblit merged commit 4fdbcf5 into gitblit:develop Nov 17, 2014

@tomaswolf tomaswolf deleted the tomaswolf:ticket-88 branch Nov 17, 2014

tomaswolf added a commit to tomaswolf/gerrit-gitblit-plugin that referenced this pull request Nov 21, 2014

Improve image diffs
This is a GitBlit 1.7.0 preview: essentially

* gitblit/gitblit#229
* gitblit/gitblit#230
* gitblit/gitblit#231
* gitblit/gitblit#232

Replace the CSS-based slider by a JavaScript-based one, plus blink
comparator, plus pixel difference on browsers that support it.

@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