On 03/25/2013 05:04 PM, Christian Hammond wrote: > Hi Stephen, > > Whitespace for indentation has never been shown for most files, > regardless of that setting. That's just due to how we've always > generated diffs. > > During diff generation, indentation changes are ignored. The intent was > to prevent a block of indentation changes from showing up as a bunch of > modified lines or, worse, deleted/added lines, distracting from the > content of those reviews.
Sure, I understand that, but I would have figured that's what the "Hide whitespace changes" button would have been for. As it stands right now, those two buttons don't appear to do anything at all. > > All other whitespace changes are then let through (trailing whitespace, > whitespace within the line), and lines containing just that are assigned > a class. The "Show/Hide Extra Whitespace" toggle then just changes the > visibility of any chunks containing these classes. It doesn't regenerate > the diff or anything. The goal of this feature is to make code review > easier when the file contains lots of whitespace cleanup, but not to > show indentation. > > So, they're noticing this for the first time. It has never been any > different, out of the box. > Well, this instance has only been up for about a week, so "for the first time" happened pretty quickly. > This can be disabled in an installation, though. You can include *.py > files in "Show all whitespace for:" list in the Diff Viewer Settings in > the Admin UI, and we'll stop ignoring leading whitespace on lines. > I couldn't get this to work for https://reviewboard-openlmi.rhcloud.com/r/54/diff/ I added "*.py, openlmi-doc-class2*" to the ignore list in the Diff Viewer settings (and restarted the server), but it still does not display leading whitespace changes on line 124 of that diff. > Can this all be better? Yes. What I'd love is to have the diff > generation be much smarter and say "This line hasn't changed except for > the indentation," and then mark that up differently. There's a lot of > work that would be needed, though. Something to put on the increasingly > large TODO list. > The diff generation seems to be able to identify the specific characters that have changed within a line other than whitespace, so I'm not sure why we can't just use that same logic. But I'll admit to having no clear view of the internal details here. -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~----------~----~----~----~------~----~------~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
