#321 Moving the cursor in elided text freezes Tk

closed-fixed
Jan Nijtmans
18. [text] (26)
5
2012-01-19
2012-01-11
Francois VOGEL
No

The attached patch fixes item 3021557:

Moving the cursor in elided text freezes Tk:
https://sourceforge.net/tracker/?func=detail&aid=3021557&group_id=12997&atid=112997

The patch is against the latest Fossil sources.

The existing text.test suite passes with no failure (but one completely unrelated: text-33.11).
I have added one test item for issue 3021557 that the patch is fixing.

Discussion

1 2 > >> (Page 1 of 2)
  • Francois VOGEL
    Francois VOGEL
    2012-01-11

    Patch for item 3021557

     
    Attachments
  • Francois VOGEL
    Francois VOGEL
    2012-01-11

    • assigned_to: hobbs --> vincentdarley
     
  • Francois VOGEL
    Francois VOGEL
    2012-01-11

    Note about efficiency:

    The fix replaces a loop calling TkBTreeNextLine by a single test with TkTextIndexCmp.

    This was already proposed as an alternate option in a comment in the source code, highlighting alledged better efficiency of the loop calling TkBTreeNextLine.

    It turns out that the loop calling TkBTreeNextLine did not do what is wanted, which is the cause of the bug, whereas TkTextIndexCmp is the right thing to do.

    Regarding efficiency, I did not find any difference between the current bugged code and the patched code. The script I used for timing both cases is the following:

    text .t
    pack .t
    .t insert end [string repeat "abcde\n" 2000]
    update
    time {.t count -displaylines 1.0 end} 100

    I have then concluded that the comment about lesser efficiency of the single call to TkTextIdexCmp as compared to the loop repeatedly calling TkBTreeNextLine was actually not relevant.

     
  • Jan Nijtmans
    Jan Nijtmans
    2012-01-17

    Branch bug-3021557 created, derived from core-8-5-branch

    On Ubuntu, the test fails, but that's because currently many Tk tests
    are dependant on font sizes available at the machine. This is
    a known fact. On trunk this is better, but still not 100%

     
  • Francois VOGEL
    Francois VOGEL
    2012-01-17

    The test fails for me on Windows as well.

    The cause seems that the widget is not packed during the test.
    The test I originally provided in my patch used a dedicated text widget, did pack it and destroy it at the end of the test.
    The test currently committed in branch bug-3021557 does not pack the text widget it is using.

    However, even when packing the widget there is still something odd in the testing since it still fails whereas running the test case test-9.2.44 standalone in wish succeeds. Are the successive test cases really independent from each other? I doubt.

     
  • Francois VOGEL
    Francois VOGEL
    2012-01-17

    Sorry, this is my mistake. In the currently committed test the text widget really is packed. However it has quite a small width, which changes the expected results because of lines wrapping (we're measuring -displaylines here!).

    The fix is simple: shorten the lines so that they do not wrap, or adapt the expected results by taking the wrapping into account.

    The attached patch "3021557_V1_1.patch" takes the first path.

     
  • Francois VOGEL
    Francois VOGEL
    2012-01-17

    Patch shortening the lines in the text widget and making test-9.2.44 pass

     
    Attachments
  • Francois VOGEL
    Francois VOGEL
    2012-01-18

    Another way: add

    .t configure -wrap none

    before the update in the -setup part of test-9.2.44

    This does apparently not impact any other test since it's anyway also done just after test-9.2.44

    This may be the best solution, but the choice is up to you.

     
  • Francois VOGEL
    Francois VOGEL
    2012-01-18

    • assigned_to: vincentdarley --> nijtmans
     
  • Jan Nijtmans
    Jan Nijtmans
    2012-01-19

    Done as suggested, this indeed fixes the test.

    Committed to core-8-5-branch. Trunk still to go,
    there I will use the original test as supplied.

     
1 2 > >> (Page 1 of 2)