[Tcl-bugs] [ tktoolkit-Bugs-3588824 ] Text widget: bug in image index handling

[SourceForge.net <no...@so...>]

Bugs item #3588824, was opened at 2012-11-20 17:07
Message generated for change (Settings changed) made by dgp
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=112997&aid=3588824&group_id=12997

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: 18. [text]
Group: development: 8.6.0
>Status: Closed
>Resolution: Fixed
Priority: 9
Private: No
Submitted By: Emiliano (egavilan)
Assigned to: Francois VOGEL (fvogelnew1)
Summary: Text widget: bug in image index handling

Initial Comment:
The [$text index $image] subcommand fails with images which contains "+", "-"  or spaces in their names.

Sample script:

package require Tk
text .t
.t image create 1.0 -image [image create photo ::foo]
.t image create 1.1 -image [image create photo ::foo-1]
puts [.t image names]
foreach img [.t image names] {
    puts [.t index $img]
}

Output:

::foo ::foo-1
1.0
bad text index "::foo-1"

The source code in generic/tkTextIndex.c checkes first for a mark name
alone before trying to parse the provided input as "base+modifier", thus
allowing mark names with spaces or "+- " chars.

The attached patch solves the issue simply performing the same check
for an image name after the one for marks.

----------------------------------------------------------------------

>Comment By: Don Porter (dgp)
Date: 2012-12-04 09:03

Message:
Different fix commited for 8.5.14 and 8.6.0.

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2012-12-03 12:54

Message:
Committed NULL protections to make
the segfaults stop.  Leaving it up to
others to determine whether what's left
is still suitable for merging as a fix of
the original bug.

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2012-12-03 12:40

Message:
Caught at least one aspect of the problem.  The routine
TkTextSharedGetObjIndex() calls

GetIndex(interp, sharedTextPtr, NULL, Tcl_GetString(idxObj),
            indexPtr, NULL);

But GetIndex() is not able to tolerate a NULL valued third argument.

----------------------------------------------------------------------

Comment By: Francois VOGEL (fvogelnew1)
Date: 2012-12-03 11:35

Message:
In core-8-4-branch this test text-8-22 does not exist because it tests the
replace subcommand of the text widget, which does not exist in 8.4.


I can reproduce the segfault on trunk, with Vista running:

nmake -f makefile.vc test OPTS=symbols TCLDIR=%MYTCL% TESTFLAGS="-file
text.test -match text-8.22"

However, running directly in wish the content of text-8.22 does work OK:

package require tcltest
namespace import tcltest::*

test text-8.22 {TextWidgetCmd procedure, "replace" option with undo} -setup
{
    text .t
    set res {}
} -body {
    .t insert 1.0 "Line 1
aefghijklm
12345
Line 4
bOy GIrl .#@? x_yz
!@#$%
Line 7"
    .t configure -undo 0
    .t configure -undo 1
    .t replace 2.1 2.3 foo
    # Ensure we can override a text widget and intercept undo
    # actions.  If in the future a different mechanism is available
    # to do this, then we should be able to change this test.  The
    # behaviour tested for here is not, strictly speaking, documented.
    rename .t test.t
    proc .t {args} { lappend ::res $args ; uplevel 1 test.t $args }
    .t edit undo
    return $res
} -cleanup {
    rename .t {}
    rename test.t .t
    destroy .t
} -result {{edit undo} {delete 2.1 2.4} {mark set insert 2.1} {see insert}
{insert 2.1 ef} {mark set insert 2.3} {see insert}}


And this is OK in both core-8-5-branch and trunk.

I do not see where to start with this...
Anyone seeing the same as I do?

Any advice for fixing this perhaps?



----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2012-12-03 07:00

Message:

There's no segfault on core-8-4-branch, but
that might be because there's no test text-8.22 there.
The segfault is in the command [.t edit undo].  Once
the problem is identified, it should be verified whether
8.4 suffers too.

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2012-12-03 06:48

Message:

Test text-8.22 segfaults.

----------------------------------------------------------------------

Comment By: Francois VOGEL (fvogelnew1)
Date: 2012-12-02 17:43

Message:
Proposed fix merged in core-8-4-branch, core-8-5-branch and trunk.

----------------------------------------------------------------------

Comment By: Francois VOGEL (fvogelnew1)
Date: 2012-11-22 13:18

Message:
Fix proposal available for review in branch bug-3588824.

Regarding your remark: "the specification of indices is wrong":
I get your point and I agree with you. I have added a knownBug test as a
reminder in the test suite in tkTextindex.test


----------------------------------------------------------------------

Comment By: Emiliano (egavilan)
Date: 2012-11-21 06:08

Message:
The test suite doesn't check for weird image names, it uses "textimage" for
all tests (textIndex.test).
Regarding window names, the answer is yes, the check should be added
between the mark and image ones to follow the order given in the man page.

Is also worth noting that the specification of indices is wrong in the
sense it should be really a list and not an almost free-form string. For
example, while with the patch the code

[.t index ::foo-1]

works, the following (legal) doesn't:

[.t index {::foo-1 -1char}]

The solution is to rule out the form "base+modifier" without spaces, and
assume the first word is the base. Unfortunately, this breaks scripts in
the wild, notably tkcon.

----------------------------------------------------------------------

Comment By: Francois VOGEL (fvogelnew1)
Date: 2012-11-20 23:06

Message:
I will look at this.

The patch looks simple enough.

Wondering:
  - if the patch does not make any test of the test suite fail (I mean: new
failures)
  - if this kind of fix should include the case of embedded window names in
addition to image names (I think the answer is yes)


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=112997&aid=3588824&group_id=12997