Summary

Add a toggle to hide / show trailing or extra whitespaces

Review Request #1458 — Created March 5, 2010 and submitted

Information

Owner

lonico

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Our users could not decide whether they wanted extra whitespaces to be highlighted or not.  Some people love it, others find it too distracting, the more for legacy code.  This change provides a button, modeled on the collapse change button.  The choice is also saved in a cookie, so that each user can have her own preference.

This is different from Show/Hide Whitespace changes, and the 2 options cohabit well, I verified that after upgrading to 1.5 beta1.  (I had to change gt(0) to gt(1) in the javascript.  I would rather use a class name associated to the li tag, but I was concerned about a potential performance impact.)

I was also wondering if I should make it more dynamic, ala Show/Hide Whitespace changes, but I see it as a more static choice, depending on the code you're working on.

Testing Done

This has been in production in our local installation for over 6 weeks.

Screenshots

showing extra white spaces

hiding extra white spaces

Given that this doesn't really need to do anything inside of Review Board to generate a new form of diff, I'd love to see this done solely in JavaScript. You'd need only to toggle the CSS rule. You could also set the cookie dynamically in JavaScript.

dbuch-agi March 8, 2010, 9:32 a.m.

I've been getting requests from coworkers for such a feature.  +1 for js-only implementation, though.

lonico March 9, 2010, 9:11 a.m.

I agree.  I'll give a try at JS, unless somebody beats me to it.

Change Summary:

New revision, based on javascript.

It provides a new toggle, and I tested it does not interfere with the other buttons.

Not sure it needs to be enabled/disabled based on the site configuration, but since the code was there in the template, I kept it.

Diff:

Revision 2 (+78 -4)

Show changes

	reviewboard/htdocs/media/rb/js/diffviewer.js
	reviewboard/templates/diffviewer/view_diff.html

Change Summary:

Minor update, replaced toggleExtraWhitespace(1) with toggleExtraWhitespace('init') to make the intent more obvious.

Diff:

Revision 3 (+78 -4)

Show changes

	reviewboard/htdocs/media/rb/js/diffviewer.js
	reviewboard/templates/diffviewer/view_diff.html

Ship it!

There was one bug (it wouldn't show the toggle button if the siteconfig item was set to the default, only if it was explicitly true). Fixed that, and submitted as r5b3925c. Thanks!