Re: [TCLCORE] About TIPs 727 and 729

[Csaba N. <csa...@t-...>]

Hi Emiliano,

Many thanks for your quick feedback!  See my embedded comments below.


Am 27.09.25 um 02:07 schrieb Emiliano:
> On Fri, 26 Sep 2025 19:53:23 +0200
> Csaba Nemethi <csa...@t-...> wrote:
> 
>> Work completed.  Thanks to Emiliano for his high-quality tkcargo
>> implementation, which the tk attribtable code is based on!
>>
>> Best regards,
>>
>> Csaba
> 
> Great work! Thanks!
> 
> I have a few remarks, none of them critical.
> 
> In the attribtable man page, I think the final remark about the entries
> in the tables being cleared when the widget is destroyed should be before
> the subcommands description. After all, this is the main selling point of
> the functionality, IMO.
> 

Good suggestion!  Done.

> The error result raised by Tcl_WrongNumArgs here
> https://core.tcl-lang.org/tk/file?ci=6a9c2f6168e06a30&name=generic%2FtkCmds.c&ln=802
> is not the same as other tk commands, like the widget themselves. While I have
> no strong preference, I apply the "when in Rome, do as the romans do"
> principle. Example using entry:
> % entry .e
> .e
> % .e
> wrong # args: should be ".e option ?arg ...?"
> % .e asd
> bad option "asd": must be bbox, cget, configure, delete, get, icursor, index, insert, scan, selection, validate, or xview
> 

Instead of the generic word "subcommand" I have written 
"set|get|unset|clear|exists|names|pathnames" because this in addition 
enumerates the supported subcommands.  Compare this to the following 
examples:

% place
wrong # args: should be "place option|pathName args"

% canvas .c; .c yview scroll
wrong # args: should be ".c yview moveto|scroll args"

The "..." in the error message stands for "zero or more additional 
arguments".  An alternative form would be "?arg ...?" (like in the 
tkcargo code), but this would read: The subcommand can *optionally* be 
followed by at least one argument.  However, this is not quite accurate, 
because the "pathnames" subcommand mustn't be followed by anything, 
while all the others expect at least one argument.  Now I have replaced 
the "..." with "args", which suits both cases and is more familiar to 
the users, since in Tcl it means "zero or more arguments".

> In the call to Tk_NameToWindow here
> https://core.tcl-lang.org/tk/file?ci=6a9c2f6168e06a30&name=generic%2FtkCmds.c&ln=813
> the interp argument can be NULL, making the following call to
> Tcl_ResetResult unnecessary. This is not clear in the docs, and I think it
> should be fixed there, making it explicit.
> 

While this is correct, for all but two of the supported subcommands it 
is important to have the error message in interp, because of the "return 
TCL_ERROR".

> Finally, there is an interaction between "unset" and "pathnames". When all
> the names are "unset" and the attributes dictionary becomes empty, the
> widget is not shown in "pathnames", as if "clear" was called on it.

This was a deliberate decision from me, also because of the manual.  It 
is just an *internal* design decision that the attributes are kept in 
dictionaries.  From the user's point of view it should make no 
difference whether for a given widget the table contains no dictionary 
or an empty one instead.  With my version the documentation is fully in 
sync with the code.

BTW: We have the same interaction between "unset" and "exists" without 
the optional "name" argument.

> This has the advantage that further "set" calls are faster, and the
> disadvantage that AttribTableDestroyHandler keep being called for other
> events that match StructureNotifyMask, so it's a tradeoff decision to remove
> the entry or not. My (mild) preference would be to remove it, but it's
> fine as it is.

Your tkcargo implementation doesn't remove the entry either when the 
dictionary gets empty due to "unset" invocations with at least one name 
argument.  (Instead of tkcargo's "unset" subcommand we now have an 
"unset" and a "clear", as suggested by Ashok.)

> 
> Again, thanks for your time and patience!
> 
> Regards

Best regards,

Csaba

-- 
Csaba Nemethi  https://www.nemethi.de  mailto:csa...@t-...