Gitx dev diff word highlight #243
Conversation
|
There are still some issues with this, which I need to sort out before it's ready to merge... see "Known Issues / Todos" within the description. |
|
👍 That screenshot looks great. How well does it handle indentation changes? It sounds like, after you fix the 2nd to-do item, we'd expect just a green bar showing added indentation, not a pair of red+green lines? |
|
as far as I can tell all displaying issues have been fixed now. @atuttle heres a screenshot showing various changes... unfortunately indention changes (e.g. tabs to spaces) will not yet correctly highlighted because the diff line data I receive in javascript already converted tabs to spaces. so there is no way to detect any changes. |
|
Actually I'm less concerned with conversion between tabs and spaces and more concerned with addition of more tabs/spaces. For example, the case where I decide to wrap a block with an If statement, or a new div - in this case the content inside the new wrapping statement gets indented but doesn't otherwise change. It seems, based on lines 16-18 of your screenshotted diff that the support is more or less what I'm looking for. Although I'd rather it be a nice dark green block for added spaces, ala the reddish block for the removed whitespace at the end of line 21. Otherwise, perfect. |
|
For what it's worth, I also preferred the red-on-red-on-red, and green-on-green-on-green of the original screenshot over this new gray-ish background for changes in the new screenshot. |
|
Yeah, I changed the colors a bit. First Screenshot already changed the Hope this clarifies my intention. But I'm absolutely open to color Btw, the GitHub diff uses black text on all colored backgrounds... How |
|
Looks really interesting! |
|
I'd be all for making it look more like GitHub... black text with 2 shades of red/green. |
|
just fixed the tabs-to-spaces issue |
|
border-radius, eh? Got an updated screenshot? |
|
@atuttle look here: |
|
just updated the screenshot in the description to match the latest version of this. From my point of view this is ready for merge, further improvements can be added later with additional pull requests. Maybe it could make sense to add a User Preference wether to use contrastful or sober diff colors... or just let users define |
|
I don't feel like the border radius is really adding anything. It's not hurting anything either, but meh. Hope this is merged in soon! |
|
I'm using it now for work now. I doesn't crash but I have some commits where the diff view keeps showing "This is a large commit. Click here or press 'v' to view". Guess I have to investigate this before this is ready to merge... |
|
If anyone wants to help test this PR: |
|
Installed. I'll see how it works for me today. |
|
javascript diff algorithm by John Resig ( http://ejohn.org/ )
- make the inline diff aware of whitespace changes - added a test page to verify the diff output stays the same - line selection for stage/unstage in commit dialog
- increase saturation of highlight backgrounds - make highlighted text a bit darker - add tiny border-radius to highlighted blocks *blingbling*
…espace changes have been highlighted
changed whitespaceAwareTokenize to split at line breaks or spaces/tabs so that tokens of preceding whitespace do not include the last line break
it splits on: - every line break - bunch of whitespace - bunch of word characters - every non-word characters (e.g. punctuation)
…only additions or deletions
|
rebased PR: rowanj/master@dda41e4440e549a83d9a9dfcd3a0ded093c8db66 tag: builds/testing/2 |
|
@muhqu any chance of a new binary download with your latest changes? |
|
It does look pretty good; I still need to review it properly but I'm considering merging it and putting out a new testing build soon. Is the "this is a large commit" a bug in these changes, or are those commits over the existing threshold? I've been considering either dropping that code, or hugely changing the threshold anyway; I don't think that loading the larger commits is the problem it may have once been. |
|
Sigh. I would have sworn I only had 1 GitX install, but I was wrong. I removed all existing binaries, reinstalled the latest from @muhqu and reset the CLI command, and now I see word diffs. Thanks! |
|
Now that I've got it working, @muhqu's build needs more work (sorry!)... Sometimes clicking on a modified file in the stage view shows nothing, sometimes it shows the wrong contents (sometimes it's right...). I'm available to demo this via G+ hangout (adam@fusiongrokker.com) if you want to see what I see. |
|
@atuttle thanks for testing! I have just pushed a few changes which might already fix you issues (if they relate to line-break issues...). You can find a fresh binary here: http://muhqu.de/builds/GitX-dev-DiffWordHighlight@3717db3.app.zip If your issue is still present, it would be great if you could send me the corresponding |
|
@muhqu thanks for the new build. So far so good in stage view, but I'm still having trouble with viewing history. Sometimes the history shows up as expected, sometimes the filenames are listed but not their contents, and sometimes I get the "loading" graphic, but it doesn't seem to finish within 5-10 sec so I moved on. |
|
@muhqu Here's an issue with the stage view... Line numbers are wrong. I can illustrate this by changing the amount of context shown: With context set to pretty low, this change appears to be on "line 2": But if I bump up the context, it changes lines: And it gets weirder. If I bump the context any higher than pictured above, the amount of included context goes crazy. This is with the slider just 1 increment higher: (note the visible scrollbar -- there's more above that's out of the viewport!) |
|
@muhqu Also, line numbering seems to continue through hunks. For example: "Line 133" here is actually line 117; and because of this bug, the line numbers displayed in GitX can far exceed the number of lines in the file. This particular file is 191 lines, but with context cranked up to maximum GitX shows up to "line 242" |
|
@atuttle thanks a lot for the details. The underlaying diff data comes directly from a call to |
|
I'm on 1.8.0. Suppose I could upgrade... Adam On Fri, Aug 30, 2013 at 3:22 AM, Mathias Leppich
|
|
Upgraded to 1.8.3.4 and I'm not having any line-numbering or hunk-size Adam On Fri, Aug 30, 2013 at 7:08 AM, Adam Tuttle adam@fusiongrokker.com wrote:
|
|
Oh... thanks a lot for clarifying ... I will try to repro with 1.8.0 to identify the issue. It has to be some difference in the diff output thats generated from the cmd-line |
|
Can you tell I really want this feature? ;) Adam On Fri, Aug 30, 2013 at 9:37 AM, Mathias Leppich
|
|
Heh, chalk up another point in the "reasons not to use command-line git output" column :-) I guess I'll try to parse the Sparkle stats to see how common Git 1.8.0.x is. |
|
awesome, that would be quiet interesting! |
|
@rowanj yesterday I played around with the 'large commit' handling and found a way to make the diff-loading-process more progressively. All diffs are now handled the same way, a progress bar shows and updates only at 100ms. So most of the time commits are loaded fast enough to not show a progress bar at all. Here's a screenshot: Should I add this change to this pull request or do you prefer a separate, rebased pull request? |
|
|
👍 awesome! looking fwd to a new beta build! ;-) |
|
👏 |
|
Wait, does this word diff rendering look right to you:
The screenshot is from Version 0.15.1869 dev (0.15.1869). |
|
so the issue is when text parts just move around... anyway, this is an issue and I will look into it once I find time for it, thanks for reporting |
|
No problem. I created #321 since this has already been merged. Let's continue the discussion there. |
Inspired by GitX. rowanj/gitx#243 Closes #141. Bug found in Sublime, issue filed: sublimehq/sublime_text#766
Integrated an inline word diff algorithm to highlight changed words in diffs. similar to github's hybrid mode.
Addressing #168
The javascript implementation is based on John Resig's javascript diff algorithm described here: http://ejohn.org/projects/javascript-diff-algorithm/
Known Issues
fix diff view keeps showing "This is a large commit..." for some commits... need to figure out what's happening...invalidBonus Todos
allow diff colors to be customized viasuperseded by Customisable CSS #249git config