Welcome to the Splat alpha!

As much as I hate the way people define tabstops, I think we probably have to support
this.

• Status:

  +	Confirmed

• Tags:

  -	Type-Defect
  +	Type-Enhancement
  +	Milestone-Release1.5

Personally I see this as quite an important features.  

As much as people recommend against using tabs to format text documents, it happens a
lot.  We do all our development in Visual Studio and most people use tabs.

This pretty much makes all our side-by-side diffs harder to read than they should be
since a tab in VS is 4 spaces, and it seems like the diff viewer sees them as 9 spaces.

Is there a "right place" to implement a workaround that replaces all 'tabs' with
appropriate spaces until a configuration option has been implemented?

There's not. Any code going in to work around this might as well turn into a full patch.

The problem is deciding where to support this. Doing it per-review sets a new
precedent for review-specific settings, and I don't know about that just yet.
Per-repository may not be sufficient in some setups.

I'd almost say let's do this in post-review.

• Tags:

  +	Component-DiffViewer

Per repository sounds like a decent place to start, though you're correct it wouldn't 
be perfect in every case (we develop mainly in .Net, but there is code developed in a 
range of other IDE's or tools whose tabs to space ratio is slightly different).

But if it's a per repository setting, then at least the majority of cases could be met, 
without having to do it every time you raise a review (I wouldn't want to have to know 
how many spaces equal a tab every time I post code up for review, and I imagine more 
people would agree).

You could even extend this and have an 'IDE/Tool List' which is configured on a per-
install basis, so various settings (if settings other than tabs were needed) could be 
added easily.  When posting a review, selecting the tool the code was developed in 
could be selected and the correct settings used (this would make it easier to extend in 
the future).

I don't know about post-review, as I'm not sure what that is referring to in the case 
of ReviewBoard.  Do you mean once the review has been posted (so people can modify it 
on a per diff basis if needs be) or does it mean something else?

• Tags:

  +	Milestone-Release1.6

This is biting us too.  I think for now we'll work around by hacking postreview.

FWIW, moz 4.0 has css for modifying tab width (and IIUC it looks like recent versions of opera do too).  https://bugzilla.mozilla.org/show_bug.cgi?id=517882

Yes, we need some way to not have tabstops messed up because it makes reviews difficult. The diffs from svn are offset by one space, so I tried to process the diff using:

expand -t5,9,13,17,21,25,29,33,37,41,45,49,53,57,61,65,69,73,77

But the resulting diff fails to be patched into the source from the repository because of whitespace differences.

Same issue as max9219.  We modified PerforceClient in postreview.py to do tab-expansion before diffing, and got patch failures after/upon upload.  chipx86, when you suggest "do[ing] it in postreview" was there a corresponding server-side change you had in mind to get around the patch failure?

I can confirm that adding

pre {
  -moz-tab-size: 4;
  -o-tab-size: 4;
}

to diffviewer.css gives reasonable results in opera; moz4 is untested.

• Tags:

  +	Priority-Low

Moving some bugs/requests to 1.7. 1.6 is intended to be a short release, and needs to be limited in scope.

• Tags:

  +	Milestone-Release1.7

Given that the purpose of this entire system is to facilitate code reviews I question why this feature/bug is "low" priority. Doing diffs of code where a combination of spaces and tabs is already present in millions of lines of code at four makes it much harder to read the diffs. I love the most recent diff version compared to older one we were using but looking at incorrectly indented code is quite tedious. Please raise this and possibly split into two features. Simple server side tab setting and then the various other options. Would love to see this in the next version and thanks. I will make a donation today just in case it helps make that a reality. We will try and move to tabs as much as possible specifically to make this great tool work for us.

@13, No judgement on your priority argument but modifying the .css is a usable workaround to get server-side tab settings.  I can confirm it works with firefox 4.

I think we can do something for 1.6. I'll bump up the priority too.

• Tags:

  -	Priority-Low
  -	Milestone-Release1.7
  +	Priority-High
  +	Milestone-Release1.6

So unfortunately, the CSS for tab sizes is pretty unsupported in most browsers. Firefox 4 has it, and Opera 10.60, but Chrome/Safari/IE do not yet (nor do they provide alternatives). So we can't really do this through CSS yet. The only other solutions are styling all tabs and attempting to represent that size with a width:, or processing the file and converting to spaces. I'm not really happy with either of those yet, and feel that maybe we'll wait a bit more to see how this is supported in browsers.

I do have a patch that implements the CSS and provides global and per-repository options for it. I just don't want to advertise it too much when it's barely supported.

• Status:

  -	Confirmed
  +	Started

• Tags:

  -	Milestone-Release1.6
  +	Milestone-Release1.7

• Assigned to:

  +	chipx86

I wrote a userscript for google chrome and firefox w/greasemonkey that turns tabs into 4 spaces or anything you want. It integrates with reviewboard (it adds a button next to "Hide Whitespace changes"). Feel free to use or modify it.

• Files:

  +

  replace_tabs.user.js // ==UserScript== // @name Replace Tabs // @namespace https://rwbd.nycapt35k.com // @description Replace tabs with anything you want // @include https://rwbd.nycapt35k.com/* // ==/UserScript== var automatic = false; if (automatic) { var oldupdateAnchors = unsafeWindow.updateAnchors; unsafeWindow.updateAnchors = function(table) { oldupdateAnchors.apply(this, arguments); replaceTabs(); } } var replacement = ' '; //window.addEventListener( // 'load', // function() { var controls = getElementsByClassName('controls')[0]; var newLi = document.createElement('li'); var newA = document.createElement('a'); newA.addEventListener('click', function() { replaceTabs(); }, true); newA.setAttribute('href', '#'); newA.innerHTML = '<img src="/media/rb/images/expand.png?1291062861" width="14" height="14" border="0" alt="" /> Replace tabs with spaces'; newLi.appendChild(newA); controls.appendChild(newLi); // }, // t

I made a change (ReviewBoard-1.0.5.1) in reviewboard\diffviewer\diffutils.py file and everything works:
Line 358-359, you will find:

    old = convert_to_utf8(old, encoding)
    new = convert_to_utf8(new, encoding)

change them to

    old = convert_to_utf8(old, encoding).expandtabs(4)
    new = convert_to_utf8(new, encoding).expandtabs(4)

So write to the end of the lines: ".expandtabs (4)". That's all. Empty all cache (python, server and client), they can foul you.

• Tags:

  -	Milestone-Release1.7
  +	Milestone-Release1.8

To answer comment https://code.google.com/p/reviewboard/issues/detail?id=839#c3 in the above, I have been personally running
$('span.tb').html(' ') in the console of the browser to convert tabs into spaces, until the actual feature comes in the reviewboard.

• Duplicates:

  +	3012 - Manual line-alignment in diff

Comment #18 by Faller.G seems promising. Could this be a place to make a proper option for this? I'm working on an old code base now, where people have been sloppy about spaces/tabs in the past, and would love to be able to set the displayed tab width.

• Tags:

  -	Milestone-Release1.8

I was wondering if there is any progress with this ticket? Do you plan to include it in 2.5.x or later versions?

Thanks.