270: Option to ignore trailing/inconsistent white space highlighting in diff
- Fixed
- Review Board
| tvol****@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| Sept. 14, 2009 |
What's the URL of the page containing the problem? What steps will reproduce the problem? 1. Make many white space changes to a file (like removing all trailing white space) 2. Make only one substantive change in the file 3. Create a review request containing your changes. What is the expected output? What do you see instead? I'd like to be able to toggle white space differences, so you can see a diff that doesn't show white space changes (diff -bBw). Seeing them all can clutter up the real change that was made. What operating system are you using? What browser? FF2/Linux Please provide any additional information below.
-
- Type-Defect + Type-Enhancement + Component-DiffViewer
This sounds more like a problem with the diff upload tool, are you using post-review?
I'm using post-review, and it isn't a problem with the diff upload tool (though from my wording I suppose it could be interpreted as such). The display of whitespace differences is handled in diffviewer/templatetags/difftags.py in the filter showextrawhitespace(). My request is to make this highlighting of trailing whitespace optional. Right now I have a local modification to my RB installation to turn this off. I know the philosophy within VMWare is to go through and remove all of these trailing whitespaces from their source, but this isn't realistic here.
I have done the same local changes to showextrawhitespace() + changed myersdiff.py from:
if self.ignore_space:
temp = line.lstrip()
# We still want to show lines that contain only whitespace.
if temp != "":
line = temp
to
if self.ignore_space:
line = line.strip()
This removes most of the whitespace-only diffs. We do not really care about these
since removing them is not realistic in a big development group with multiple
operating systems and multiple editors.
This option would be useful to us as well. Thanks, Nithin.
Pretty sure there's another bug with the same request somewhere. We can probably put this in for 1.0.
-
+ Milestone-Release1.0 + Component-Settings -
+ Option to ignore trailing/inconsistent white space highlighting in diff
-
+ Confirmed
I'm a newbie to using review board too. I'd like to 2nd having an option like this to ignore the extra noise of white space. Or perhaps, there is some ability to add additional DIFF options definitions in a user's .reviewboardrc or a the system level. DIFFOPTIONS=xxx DIFFCMD=yyyy/mydiff.exe Again,
This might slip until after 1.0. I'd like to freeze the database schema soon and not change it until after the release.
-
- Priority-Medium + Priority-Low
I would like to second this proposal. Trailing whitespace is pernicious, but in some circumstances it's not feasible to remove it. Having it come up in bright red is also visually distracting and as one poster says it creates clutter and detracts from the actual changes being made.
"Having it come up in bright red is also visually distracting and as one poster says it creates clutter and detracts from the actual changes being made." - Absolutely agree. This is so annoying.
There might be some work related to this post-1.0 for Summer of Code, and I definitely want to introduce a new user prefs overhaul first. For the time being, people can set a custom stylesheet to override in the browser or use one of the GreaseMonkey or Stylish extensions floating around to disable the highlighting. Going to push this out a bit.
-
- Milestone-Release1.0 + Milestone-Release1.5
Is this really best as some kind of special user preference? While sure, there are people who always want it on or always want it off, a lot of people I talk to want to be able to see it but also want it to go away while they are looking at the actual content. Perhaps a better way to handle this is something on the review page that lets you toggle whitespace display on and off.
Paul: We have a Summer of Code student actually working on providing whitespace metadata that's available to the page such that individual users can toggle this on/off when they want to. It'll give us a lot of flexibility on what we can allow users to do here.
I've just upgraded to 1.0 and have made use of the new ignore trailing whitespace option for the diff viewer. Very cool!
I guess that this functionality is pretty stable and works on all browsers. So shouldn't we set it to milestone release 1.1 instead of 1.5?
Probably. I think this can actually be called fixed, too. Don't you think?
-
- Milestone-Release1.5 + Milestone-Release1.1
Well, I don't know the policy for marking something as fixed. Is it the code that fix the issue being on trunk or release? If it's on trunk then this can be marked as fixed.
In response to Christian's comment 14, I'm wondering if the individual diff option toggle is still under development? The global options are a little too broad for me. We're using review board version 1.0.5.1.