Tk Source Code

Consider following code:
pack [text .t]
.t insert end foo\nbar
.t delete 2.0 end

This should only delete the second line ("bar"), and leave "foo\n" in the text. However, it actually deletes the newline too, leaving only "foo". This is wrong, since [.t delete 2.0 end] should *never* delete anything before index 2.0 !

I'm here proposing the same patch as in tracker 1854913. That one was closed with a different solution, fixing only the symptoms in interactive mode (<Delete> key deleting backwards), but not the cause.

Apparently, the proposed patch broke 'text-19.8' of the tk testsuite. In my opininion, that test is wrong. It deletes from index 4.0, and then checks that the selection tag ends at index 3.5. But it should still include the newline at the end of line 3. (Without this patch, that newline did not exist anymore!)

I've attached a file which includes the original solution for 1854913, and a patch for test-19.8.

See also [e8c28f71b7]

This fix was later reverted, see [18c08df753]

Discussion on TCLCORE list resulted in decision to apply the OP patch.

Therefore branch bug-2886436fff-option1 is now merged in both core-8-5-branch and trunk.

Closing the ticket.

Now there are two branches dealing with this:

- bug-2886436fff-option1: apply the OP patch, change the behavior of the text widget, never delete anything before index1

- bug-2886436fff-option2: don't change the behavior of the widget, but document it better


Prompted TCLCORE for comments on this ticket, see thread here:

http://code.activestate.com/lists/tcl-core/14704/

Indeed I think that these tests are wrong. All these tests contain something like "delete 13.0 end", whose behaviour is indeed chaned by the patch, but in the right way (inho). In fact, since it is not 'officially' allowed to delete till the end, the tests should only delete upto end-1c in the first place.

Still, the comments in the original code, and these tests, indicates that the behaviour was certainly intentional. What I find strange is that the authors of the original code wrote tests for this behaviour, but did not realize that the <Delete> key behaved wronly as a result (see ticket 1854913, which preceded this one).

I understand the doubt about this patch, so feel free about how to handle it. However, if you update the comment as in branch bug-2886436fff, please also fix the statement about an 'even' number of lines getting deleted. It is very well possible to delete an odd number of lines :-)

I understand your point.

However, applying your original patch triggers 5 failures in the test suite for the text widget. This does not smell good to me at first sight: it reveals that the behaviour of the text widget is changed by the patch, in such a way that existing user code could be broken.

The point is that your original patch, while perhaps providing more intuitive behavior of the text widget and reduction of the code complexity, definitely does change the behavior of the widget. There is a risk that existing code in the wild get broken by your patch.

Unless all these tests are wrong in their "normal" expectations? Could you please analyze these results?



Herebelow the tests log I get against the current core-8-5-branch on Windows:


Only running test files that match:  text*.test
Tests began at Fri Jul 10 20:43:46 +0200 2015
text.test


==== text-17.8 DeleteChars procedure FAILED
==== Contents of test case:

    setup
    .t tag add sel 1.0 end
    .t delete 4.0 end
    list [.t tag ranges sel] [.t get 1.0 end]

---- Result was:
{1.0 4.0} {Line 1
abcde
12345

}
---- Result should have been (exact matching):
{1.0 3.5} {Line 1
abcde
12345
}
==== text-17.8 FAILED

textBTree.test
textDisp.test


==== textDisp-4.9 UpdateDisplayInfo, filling in extra vertical space FAILED
==== Contents of test case:

    .t delete 1.0 end
    .t insert end "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17"
    .t yview 16.0
    update
    .t delete 15.0 end
    list [.t bbox 7.0] [.t bbox 12.0]

---- Result was:
{3 18 7 15} {3 93 7 15}
---- Result should have been (exact matching):
{3 33 7 15} {3 108 7 15}
==== textDisp-4.9 FAILED



==== textDisp-4.10 UpdateDisplayInfo, filling in extra vertical space FAILED
==== Contents of test case:

    .t delete 1.0 end
    .t insert end "1\n2\n3\n4\n5\nLine 6 is such a long line that it wraps around.\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\
n17"
    .t yview end
    update
    .t delete 13.0 end
    update
    list [.t index @0,0] $tk_textRelayout $tk_textRedraw

---- Result was:
6.0 {13.0 7.0 6.40 6.20 6.0} {6.0 6.20 6.40 7.0 13.0}
---- Result should have been (exact matching):
5.0 {12.0 7.0 6.40 6.20 6.0 5.0} {5.0 6.0 6.20 6.40 7.0 12.0}
==== textDisp-4.10 FAILED



==== textDisp-4.11 UpdateDisplayInfo, filling in extra vertical space FAILED
==== Contents of test case:

    .t delete 1.0 end
    .t insert end "1\n2\n3\n4\n5\nLine 6 is such a long line that it wraps around, not once but really quite a few times
.\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17"
    .t yview end
    update
    .t delete 14.0 end
    update
    list [.t index @0,0] $tk_textRelayout $tk_textRedraw

---- Result was:
6.60 {14.0 7.0 6.80 6.60} {6.60 6.80 7.0 14.0}
---- Result should have been (exact matching):
6.40 {13.0 7.0 6.80 6.60 6.40} {6.40 6.60 6.80 7.0 13.0}
==== textDisp-4.11 FAILED



==== textDisp-28.1 "yview" option with bizarre scroll command FAILED
==== Contents of test case:

    catch {destroy .t2}
    toplevel .t2
    text .t2.t -width 40 -height 4
    .t2.t insert end "1\n2\n3\n4\n5\n6\n7\n8\n"
    pack .t2.t
    wm geometry .t2 +0+0
    update
    .t2.t configure -yscrollcommand bizarre_scroll
    .t2.t yview 100.0
    set result [.t2.t index @0,0]
    update
    lappend result [.t2.t index @0,0]

---- Result was:
6.0 2.0
---- Result should have been (exact matching):
6.0 1.0
==== textDisp-28.1 FAILED

textImage.test
textIndex.test
textMark.test
textTag.test
textWind.test

Tests ended at Fri Jul 10 20:44:05 +0200 2015
all.tcl:        Total   1607    Passed  1539    Skipped 63      Failed  5
Sourced 8 Test Files.
Files with failing tests: text.test textDisp.test

Ok, it seems that deleting up to the 'end' tag is in fact illegal. Raising an error is obviously not an option, since almost all existing code will do [.t delete 1.0 end] when it wants to delete all contents.

Given this, arguments to select a behaviour should (imho):
- do the least surprising thing
- not increase the code complexity (to support an illegal operation)

The current code does the opposite: it does the most surprising thing, and it increases the code (and comments) complexity.

Deleting from a certain position should _never_ delete backwards. Currently, it deletes backwards when there is a newline before it, but not otherwise. This is highly irregular and surprising!

So, I still prefer my original patch.

Proposal in branch bug-2886436fff

I believe the patch has to be rejected, based on considerations exposed in the following thread:

https://groups.google.com/d/msg/comp.lang.tcl/oCz1aatwJZQ/3ji5FAhsSm4J

More precisely in that message:

https://groups.google.com/d/msg/comp.lang.tcl/oCz1aatwJZQ/mUQHfsfQG1IJ

>> The patch contains (on the "-" side) what was the reason for this 
>> irregularity in the first place: it was the idea that a deletion 
>> including the final \n is an attempt to delete a whole line, so the 
>> \n before the deleted block shall become the new final \n and some 
>> tags shifted accordingly. 
>
>
> Indeed. You express this original intention so clearly that I think I 
> will: 
> - add this as a comment in the source code 
> - add a note in the "delete" section of the text widget man page 
> - and as a consequence reject the patch proposed in bug [2886436fff], 
> by relying once again on the fact that this will now be documented 
>
> So far I did not apply this patch because I couldn't figure out who 
> was right: the patch submitter or the existing source code. Now the 
> way ahead is clearer to me.

Any thoughts before I proceed...?

data_type - 112997

File Added - 348218: textdelete.patch