tablelist 6.3 bug

[Ralf Fassel]

tablelist 6.3 introduced some changes in the form of

if {$updateListVar} {
=>
if {$updateListVar && [info exists ::$data(-listvariable)]} {

One of these changes opens a code path where I get an TCL error:

tablelist6.3/scripts/tablelistWidget.tcl

proc tablelist::insertRows {win index argList updateListVar parentKey \
childIdx} {
--<snip-snip>--

if {$updateListVar && [info exists ::$data(-listvariable)]} {
(1) upvar #0 $data(-listvariable) var
trace vdelete var wu $data(listVarTraceCmd)
}

#
# Insert the items into the internal list
#
set result {}
set appendingItems [expr {$index == $data(itemCount)}]
set appendingChildren [expr {$childIdx == $childCount}]
set row $index
foreach item $argList {
#
# Adjust the item
#
set item [adjustItem $item $data(colCount)]

#
# Insert the item into the list variable if needed
#
(2a) if {$updateListVar} {
if {$appendingItems} {
lappend var $item ;# this works much faster
} else {
(2) set var [linsert $var $row $item]
}
}
--<snip-snip>--

The mapping of var to the listvariable at (1) is only done if the
variable exists, but the derefencing at (2) is done regardless of the
variable existing.

Unfortunately I cannot isolate the offending code in my app, but I think
the analysis is clear here. If the variable does not exist, either
'updateListVar' should not be true at all, or it should get reset to
false if (1) fails, or the [info exists] test at (1) needs to be
re-evaluated at (2) or (2a).

TNX
R'

[Ralf Fassel]

* Ralf Fassel <ral...@gmx.de>

| Unfortunately I cannot isolate the offending code in my app, but I think
| the analysis is clear here. If the variable does not exist, either
| 'updateListVar' should not be true at all, or it should get reset to
| false if (1) fails, or the [info exists] test at (1) needs to be
| re-evaluated at (2) or (2a).

Further analysis shows that the error is triggered when tablelist is

- used as a component of an Itk-Widget, where itcl is still the 'old'
variant (not the one included in tcl 8.6)
- AND -listvariable is coupled to a class variable.

In this case, the class variable when used via itcl::scope has the form

data(-listvariable) => {@itcl ::.foo0 ::foo::myvar}

which when queried via

[info exists ::$data(-listvariable)]

produces 0, whereas the 'old' variant

[info exists $data(-listvariable)]

produced 1:

% set foo {@itcl ::.foo0 ::foo::myvar}
@itcl ::.foo0 ::foo::myvar

% info exists $foo
1

% info exists ::$foo
0

Not-so-small sample code to reproduce:

$ cat t.tcl
package require Tablelist
package require Itk
::itk::usual Tablelist { }

itcl::class foo {
inherit itk::Widget
constructor {args} {
itk_component add tl {
tablelist::tablelist $itk_interior.tl -listvariable [itcl::scope myvar] \
-columns {0 col1 0 col2 0 col3}
}
pack $itk_component(tl) -fill both -expand yes
}
public {
method errcmd {} {
$itk_component(tl) insert 1 {four five six}
}
}
private {
variable myvar [list {one two three} {four five six}]
}
}

set obj [foo .\#auto]
pack $obj -fill both -expand yes

# make it err out
$obj errcmd
# EOF


$ wish
% source t.tcl

% set errorInfo
can't read "var": no such variable
while executing
"linsert $var $row $item"
(procedure "insertRows" line 47)
invoked from within
"insertRows $win $index [lrange $argList 1 end] $data(hasListVar) root $index"
(procedure "insertSubCmd" line 13)
invoked from within
"${cmd}SubCmd $win [lrange $args 1 end]"
(procedure "tablelist::tablelistWidgetCmd" line 8)
invoked from within
"$itk_component(tl) insert 1 {four five six}"
(object "::.foo0" method "::foo::errcmd" body line 2)
invoked from within
"$obj errcmd"
(file "t.tcl" line 41)
invoked from within
"source t.tcl"


% package versions Itcl
3.4

% package versions Itk
3.4

% info tclversion
8.5

% info patchlevel
8.5.19

I guess this opens up a can of worms, so I'll wait until Csaba
answers...

R'

[Ralf Fassel]

* Ralf Fassel <ral...@gmx.de>
| * Ralf Fassel <ral...@gmx.de>

| Further analysis shows that the error is triggered when tablelist is
>
| - used as a component of an Itk-Widget, where itcl is still the 'old'
| variant (not the one included in tcl 8.6)
| - AND -listvariable is coupled to a class variable.
>
| In this case, the class variable when used via itcl::scope has the form
>
| data(-listvariable) => {@itcl ::.foo0 ::foo::myvar}
>
| which when queried via
>
| [info exists ::$data(-listvariable)]
>
| produces 0

Perhaps a better way to query the existence of the variable would be
using 'uplevel 0' instead of blindly prefixing with "::"

if {$updateListVar && [info exists ::$data(-listvariable)]} ...
=>
if {$updateListVar && [uplevel \#0 [list info exists $data(-listvariable)]]} ...

since most of the times the very next lines is

upvar #0 $data(-listvariable) var

?
R'

[nemethi]

Many thanks for your bug report!

You are right that the code above contains an inconsistency
(unfortunately, it escaped my attention when making the change in
Tasblelist 6.3). To eliminate it, the [info exists] test at (1) must be
re-evaluated at (2a).

On the other hand, as shown in your 2nd posting within this thread,
using [info exists ::$data(-listvariable)] is not safe in the rather
special case of your application (using Itk 3.4). While IMHO this is
actually a bug in Itcl 3.4, Tablelist should behave the same, regardless
of whether Itk 3.4 or 4.1.0 is being used (your sample script works as
expected when using Itk 4.1.0). I think, the right solution to this
problem is to use [uplevel #0 [list info exists $data(-listvariable)]],
as proposed in your 3rd posting within this thread. The Tablelist code
contains 9 occurrences of [info exists ::$data(-listvariable)] -- all
these need to be replaced with [uplevel #0 [list info exists
$data(-listvariable)]].

--
Csaba Nemethi http://www.nemethi.de mailto:csaba....@t-online.de

[nemethi]

Since I have found one more occurrence of "if {$updateListVar} {" in the
procedure tablelist::insertRows, a better way to eliminate the two
inconsistencies is:

set updateListVar [expr {$updateListVar &&
[uplevel #0 [list info exists $data(-listvariable)]]}]
if {$updateListVar} {

upvar #0 $data(-listvariable) var
trace vdelete var wu $data(listVarTraceCmd)
}

and letting the two "if {$updateListVar} {" occurrences unchanged.

[Ralf Fassel]

* nemethi <csaba....@t-online.de>

| While IMHO this is actually a bug in Itcl 3.4, Tablelist should behave
| the same, regardless of whether Itk 3.4 or 4.1.0 is being used (your
| sample script works as expected when using Itk 4.1.0).

This is due to the internal representation of variable scope in the two
underlying Itcl versions, as can be seen in this simpler example using
only Itcl, not Itk:

package require Itcl
package require Tablelist 6.3

itcl::class foo {
public {
method connect_listvar {w} {
$w configure -listvariable [itcl::scope myvar_]
puts stderr "$w configure -listvariable [itcl::scope myvar_]"
}
}
private {
variable myvar_ ""
}
}

set tbl [tablelist::tablelist .t -columns {0 one 0 two}]

set obj [foo \#auto]
$obj connect_listvar $tbl

With Itcl 3.4, it prints

.t configure -listvariable @itcl ::foo0 ::foo::myvar_

With Itcl 4.1.0, it prints

.t configure -listvariable ::itcl::internal::variables::oo::Obj34::foo::myvar_

And this 4.1 version of the itcl variable representation is an
'ordinary' namespace, as opposed to the 'old' Itcl 3.4 representation.
With that, simply prefixing it with another "::" just works.

| I think, the right solution to this problem is to use [uplevel #0
| [list info exists $data(-listvariable)]], as proposed in your 3rd
| posting within this thread.

I don't know what those [info exist...] code passages were meant to
achieve, since usually all other widgets simply create the global
variables they are connected to if they don't exist:

entry .e
info exist foo
=> 0
.e configure -textvariable foo
info exist foo
=> 1

And in fact looking at the code in tablelistConfig.tcl, proc
tablelist::makeListVar (which is called in the process of
"configure -listvariable"), there is no way how the variable could not
exist: it is set there if it does not exist:

proc tablelist::makeListVar {win varName} {
...
upvar #0 $varName var
...
if {[info exists var]} {
#
# Invoke the trace procedure associated with the new list variable
#
listVarTrace $win $name1 $name2 w
} else {
#
# Set $varName according to the value of data(itemList)
#
set var {}
foreach item $data(itemList) {
lappend var [lrange $item 0 $data(lastCol)]
}
}

And a trace for 'unset' is added which recreates the variable should it
ever become unset...

However the solution, in addition to your proposed changes involving
uplevel, IMHO the following procs also need attention with this:

- proc tablelist::listVarTrace
- in the 'unset' branch, it uses to recreate the variable:
set ::$varName {}
lappend ::$varName
trace variable ::$varName wu $data(listVarTraceCmd)
(should better use code similar to makeListVar involving upvar and
accessing the upvar'd variable)

- proc tablelist::makeListVar
- uses
if {[array exists ::$varName]} {
to detect if the variable is an array
(also should use 'upvar' and array exists on the upvar'd name)

And last but not least: many thanks for Tablelist, it is a very valuable
package!

R'

[nemethi]

I thought, too, that, due to the measures taken by the procedure
tablelist::makeListVar, there is no way how the list variable could not
exist. But back in 2006 I received a bug report along with a sample
script using snit::widgetadaptor, demonstrating that it can happen that
$data(hasListVar) is true but the variable data(-listvariable) no longer
exists. For this reason, as proposed in that bug report, I changed the
procedure tablelist::cleanup in the file tablelistBind.tcl to use

if {$data(hasListVar) && [info exists $data(-listvariable)]} {

However, a bug report from this year revealed that this only works as
expected if the list variable begins with "::" (quite logical, isn't
it?). Since this is not always the case, in Tablelist 6.3 I changed the
above code to become

if {$data(hasListVar) && [info exists ::$data(-listvariable)]} {

And, at the same time, I inserted the additional query [info exists
::$data(-listvariable)] at other places of the code, too.

I thought that this covers all the use cases, but your postings from
yesterday and today demonstrate that there are counter-examples based on
Itk 3.4 or Itcl 3.4. For this reason, the next Tablelist version 6.4
will use your valuable proposal

if {$data(hasListVar) &&
[uplevel #0 [list info exists $data(-listvariable)]]} {

>
> However the solution, in addition to your proposed changes involving
> uplevel, IMHO the following procs also need attention with this:
>
> - proc tablelist::listVarTrace
> - in the 'unset' branch, it uses to recreate the variable:
> set ::$varName {}
> lappend ::$varName
> trace variable ::$varName wu $data(listVarTraceCmd)
> (should better use code similar to makeListVar involving upvar and
> accessing the upvar'd variable)
>
> - proc tablelist::makeListVar
> - uses
> if {[array exists ::$varName]} {
> to detect if the variable is an array
> (also should use 'upvar' and array exists on the upvar'd name)

Thanks, I will examine and improve the code accordingly.

>
> And last but not least: many thanks for Tablelist, it is a very valuable
> package!
>
> R'
>

[Ralf Fassel]

* nemethi <csaba....@t-online.de>

| > However the solution, in addition to your proposed changes involving
| > uplevel, IMHO the following procs also need attention with this:
| >
| > - proc tablelist::listVarTrace
| > - in the 'unset' branch, it uses to recreate the variable:
| > set ::$varName {}
| > lappend ::$varName
| > trace variable ::$varName wu $data(listVarTraceCmd)
| > (should better use code similar to makeListVar involving upvar and
| > accessing the upvar'd variable)
| >
| > - proc tablelist::makeListVar
| > - uses
| > if {[array exists ::$varName]} {
| > to detect if the variable is an array
| > (also should use 'upvar' and array exists on the upvar'd name)
>
| Thanks, I will examine and improve the code accordingly.

I have sent you a personal mail with patches which resolve these issues
for me.

R'

[nemethi]

Many thanks for your valuable contribution!