rietveld: correct tab handling

This fixes the buggy tab rendering that can be seen at
http://codereview.appspot.com/116075/diff/1/2

The fundamental problem was that the tab code was
not being told what column the text began in, so it
didn't know where to put the tab stops.  Another problem
was that some of the code assumed that string byte
offsets were the same as column offsets, which is only
true if there are no tabs.

In the process of fixing this, I cleaned up the arguments
to Fold and ExpandTabs and renamed them Break and
_ExpandTabs so that I could be sure that I found all the
call sites.  I also wanted to verify that ExpandTabs was
not being used from outside intra_region_diff.py.

[rsc_swtch, 2009-10-03 23:02:17]

(I assume the description will be sent as part of the mail.)

[rsc_swtch, 2009-10-22 08:23:38]

ping

[GvR, 2009-10-24 17:50:03]

Hmmm... There are several things not right. I patched this into Rietveld and you
can preview it at http://11.latest.codereview.appspot.com/.

(1) If you view your original sample with this version, the tabs are expanded
correctly but the red ">>" marker is missing.

(2) Intra-line diffs seem to be broken, e.g. look at this very change with that
version (the darker highlights are missing).

[rsc_swtch, 2009-10-24 18:11:24]

On 2009/10/24 17:50:03, GvR wrote:
> Hmmm... There are several things not right. I patched this into Rietveld and
you
> can preview it at http://11.latest.codereview.appspot.com/.
> 
> (1) If you view your original sample with this version, the tabs are expanded
> correctly but the red ">>" marker is missing.

I looked at this when I was working on the patch and
figured the online server must have some special config.
If RenderIntraRegionDiff gets called with mark_tabs=True, I believe
that the markers will be displayed.  But I couldn't figure 
out how to make it get called that way: the call sites don't
specify a value, and the default, which I didn't change, is False.
When I was testing I just explicitly changed the default
in _ExpandTabs.

> (2) Intra-line diffs seem to be broken, e.g. look at this very change with
that
> version (the darker highlights are missing).

I'll look at that now.

[rsc_swtch, 2009-10-24 18:58:34]

On 2009/10/24 18:11:24, rsc wrote:
> On 2009/10/24 17:50:03, GvR wrote:
> > Hmmm... There are several things not right. I patched this into Rietveld and
> you
> > can preview it at http://11.latest.codereview.appspot.com/.
> > 
> > (1) If you view your original sample with this version, the tabs are
expanded
> > correctly but the red ">>" marker is missing.
> 
> I looked at this when I was working on the patch and
> figured the online server must have some special config.
> If RenderIntraRegionDiff gets called with mark_tabs=True, I believe
> that the markers will be displayed.  But I couldn't figure 
> out how to make it get called that way: the call sites don't
> specify a value, and the default, which I didn't change, is False.
> When I was testing I just explicitly changed the default
> in _ExpandTabs.

I'll look more into this.  I tried putting mark_tabs=True on
the RenderIntraRegionDiff call sites, and that certainly
made the markers come back.  I'll see if I can find why the
trunk is putting them in.

http://swtch.com/~rsc/codereview1.png
my original tab example.  the three windows are,
top to bottom, current codereview, code with patch,
code with patch + mark_tabs=True on the 
RenderIntraRegionDiff call sites.
(column width=40)

> > (2) Intra-line diffs seem to be broken, e.g. look at this very change with
> that
> > version (the darker highlights are missing).
> 
> I'll look at that now.

This should be fixed in patch #2.

http://swtch.com/~rsc/codereview2.png
the file from this change.  top is current codereview,
bottom is with patch.
(column width=40 to cause line wraps)

[rsc_swtch, 2009-10-24 19:52:06]

> > I looked at this when I was working on the patch and
> > figured the online server must have some special config.
> > If RenderIntraRegionDiff gets called with mark_tabs=True, I believe
> > that the markers will be displayed.  But I couldn't figure 
> > out how to make it get called that way: the call sites don't
> > specify a value, and the default, which I didn't change, is False.
> > When I was testing I just explicitly changed the default
> > in _ExpandTabs.
> 
> I'll look more into this.  I tried putting mark_tabs=True on
> the RenderIntraRegionDiff call sites, and that certainly
> made the markers come back.  I'll see if I can find why the
> trunk is putting them in.

[The first attempt at posting this reply sent the mail but got
a 500 Server Error and did not save it to the issue log. 
This is a second attempt.]

One of the reasons I rewrote the code instead of trying to make
a smaller change is that the conventions about the relationship
between indent, limit, offset, and text did not agree at the call
sites and inside Fold.

In r476 rietveld, these are the calls to Fold to format the left half
of the pane for line 33 in http://swtch.com/~rsc/codereview1.png:

Fold: text=u'\tint' limit=40 indent=0 offset=0 tabsize=8 mark_tabs=False
Fold: text=u'\tr;\t\t\t/* % format Rune */\n' limit=40 indent=0 offset=4
tabsize=8 mark_tabs=False

The first call to fold does the right thing: it replaces the \t with spaces.

The second call to Fold expects that text[:indent-offset] has already
been processed and can be passed through unchanged.  This is not
true the way Fold gets called.  The offset=4 is taken from len('\tint'),
the size of the string passed to the previous Fold, but indent=0,
so indent-offset=-4, so this piece:

  if "\t" in text:
    # If mark_tabs is true, we retain one \t character as a marker during
    # expansion so that we later replace it with an HTML snippet.
    tab_marker = mark_tabs and "\t" or None
    rest = text[indent-offset:]
    text = text[:indent-offset] + ExpandTabs(rest, tabsize, tab_marker)

passes '\tr;\t\t\t/* % format Run' through unchanged and calls
ExpandTabs on just 'e */' (the \n has been dropped already).

In the image linked above, the three panes show:

top (current codereview):
	no » at the beginning of line 33 (correct Fold)
	visible » in the rest of line 33 despite mark_tabs=False (one of the Fold bugs)

middle (patch):
	no » anywhere on line (correct behavior for mark_tabs=False)

bottom (patch+explicit mark_tabs=True calling RenderIntraRegionDiff):
	visible » everywhere on line 33 (correct behavior for mark_tabs=True)

If you want to preserve (most of) the current behavior,
the fix is to add mark_tabs=True to the RenderIntraRegionDiff
call sites.

[GvR, 2009-10-24 23:39:10]

Live, with some docstring tweaks and mark_tabs=True.

[GvR, 2009-10-24 23:40:50]

On 2009/10/24 23:39:10, GvR wrote:
> Live, with some docstring tweaks and mark_tabs=True.

(And submitted as r478.)

I'll entertain a fix to add new Account setting to disable the tab marking
behavior (which I'm guessing you're not too fond of :-).

[rsc_swtch, 2009-10-24 23:47:12]

> (And submitted as r478.)

Thanks.

> I'll entertain a fix to add new Account setting to disable the tab marking
> behavior (which I'm guessing you're not too fond of :-).

They're fine with me.  Just as you can use them to say
"hey, don't use tabs here", I can use (the lack of) them
to say "hey, don't use spaces here".