Image diffs #229
Image diffs #229
Conversation
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.
|
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: |
|
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. |
|
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: 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.) |
|
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. |
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.
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.
* 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.
|
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. |
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.
|
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. |
|
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:
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? |
|
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? |
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.)
|
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! |
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.
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.
|
Done from my point of view. (Pending a quick test on IE...) |
|
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. |
|
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.) |
This is a GitBlit 1.7.0 preview: essentially * gitblit-org/gitblit#229 * gitblit-org/gitblit#230 * gitblit-org/gitblit#231 * gitblit-org/gitblit#232 Replace the CSS-based slider by a JavaScript-based one, plus blink comparator, plus pixel difference on browsers that support it.
Ticket 88
UI based on Lea Verou's pure CSS slider.
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.