Infinite loop in tabstop calculations

Logged In: YES
user_id=32170

What do you suggest the correct behaviour is? An error
message? Silently ignoring any smaller tabs?

Logged In: YES
user_id=68433

Raising an error might be a good idea, but it would be
backwards-incompatible. Tk 8.4 appears to use the width of
a space as the minimum distance between chunks if the tab
stop is too far to the left.

Logged In: YES
user_id=32170

Jeff, can I get an opinion on this?

It it ok for us simply to throw an error here. It's clearly
user error (and the documentation makes no mention of the
implicit constraint that tabs must be increasing), but the
only worry is that someone somewhere has written an
application which somehow makes use of this error (by design
or accident).

Joe: how did you encounter this problem?

Logged In: YES
user_id=68433

> Joe: how did you encounter this problem?

'tkdiff' (v3.09) triggers the bug. I haven't figured out
why or how tkdiff specifies nonmonotonically-increasing tab
stops, but it does.

(That's why I'd prefer that the text widget not raise an
error for this condition; it would make (currently nonfatal)
bugs in existing applications hard errors.)

Logged In: YES
user_id=32170

Ok, I took a look at tkdiff and this is the offending code:

# tabstops require a little extra work. We need to
figure out
# the width of an "m" in the widget's font, then
multiply that
# by the tab stop width. For the bottom text widget the
first tabstop
# is adjusted by two to take into consideration the fact
that we
# add two bytes to each line (ie: "< " or "> ").
set cwidth [font measure [$w(LeftText) cget -font] "m"]
set tabstops [expr {$cwidth * $tmpopts(tabstops)}]
...
$w(BottomText) configure\ -tabs [list [expr {$tabstops +($cwidth * 2)}] $tabstops]

To me, the above is clearly a bug. I have no idea what that
that last line is supposed to achieve, but I can't see how
it'll achieve any sensible goal in Tk 8.3/8.4 at all. So,
from that perspective we would be better off throwing an
error here so that this code gets fixed.

Logged In: YES
user_id=79902

It was never entirely clear to me whether the tab spacings
relative to one another, were from the left margin, or from
the left edge of the widget. However, the tkdiff code seems
to be assuming what I always guessed - that tab-stops were
all relative to the previous tab-stop. If that is indeed
the way it worked, then that is the way it must work through
8.5 at least.

The aim of the code is to simulate 8-char tab-stops (well,
the 8 is configurable by altering tmpopts(tabstops) in this
context) but offset the whole thing two characters further
in so that there is an extra "column" (if you see what I
mean) at the left edge that contains the "diff" markings
(which are two characters wide.)

Making all this code return an error is wrong since it will
break much existing code in a minor version. Furthermore,
tkdiff is a relatively high-profile app, so keeping it
working is important.

Logged In: YES
user_id=32170

Tab stops have always been relative to the widget edge, so
tkdiff's code is simply wrong. I'll make non monotonic tabs
throw an error and update the man page to clarify both (i)
everything's relative to the widget and (ii) non increasing
(or negative) tabs are an error.

Logged In: YES
user_id=32170

Fixed documentation and added tests, and fixed code to throw
an error rather than loop infinitely. tkdiff will need to
be fixed, now.

Logged In: YES
user_id=32170

p.s. of course tkdiff should use:

$w(BottomText) configure\ -tabs [list [expr {$tabstops +($cwidth * 2)}] \ [expr {2*$tabstops +($cwidth * 2)}]]

to function according to dkf's description.

Logged In: YES
user_id=79902

Digging with CVS confirms that tabs have always (minimally
since 8.0, though the changes file doesn't indicate any
alteration in tab behaviour since 4.0 when they were
introduced as controllable entitites) been done as offsets
from the left edge of the widget. However, there has also
always been code to make a tab minimally advance by the
width of a space character. If we don't do that, we had
better make sure (minimally) that we mark things as
POTENTIAL INCOMPATABILITY...

Logged In: YES
user_id=68433

> Fixed documentation and added tests, and fixed code to
> throw an error rather than loop infinitely. tkdiff will need
> to be fixed, now.

While I agree that this is probably the Right Thing, it's
also (as dkf points out) an incompatible change and ought
not to be done in a minor release.

This will break existing programs. Reopening.

Logged In: YES
user_id=72656

Especially is such a key program as tkdiff shows the
incompatability.

Logged In: YES
user_id=32170

I'd agree if the code in tkdiff acheived anything even close
to what it is apparently designed to do. But it doesn't at
all. Take this example:

# This is what Tkdiff actually does:
pack [text .t]
.t insert 1.0 "a\tb\tc\td\te\tf\tg\th"
.t configure -tabs {3c 2c}

# This is what Tkdiff actually wants:
.t configure -tabs {3c 5c}

The results are so different that we're better off telling
tkdiff it has a bug, rather than pretending it doesn't.

A key program like tkdiff ought to work correctly, IMHO ;-)

Logged In: YES
user_id=79902

Throwing an error is theoretically the right thing, but
practically the wrong one. This aspect of behaviour
probably ought not be changed until 9.0

Reporting the (long-standing) fault to the tkdiff
maintainers is definitely the right thing.

Clarifying the docs was *absolutely* the right thing. :-)

Logged In: YES
user_id=32170

Alright, I'll check in some changes to disable this until
tk9.0....

Right now I'm working on the performance related bug, so
won't check things in until I've got both straight.