Tk Source Code

View Ticket
Login
Ticket UUID: cda289a8ea8d5ac64d245cd67266cd619638bc89
Title: Revised [text]: text disappears when widget is out of focus
Type: Bug Version: revised_text
Submitter: fvogel Created on: 2017-05-13 19:48:50
Subsystem: 18. [text] Assigned To: gcramer
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2017-05-28 18:20:39
Resolution: Fixed Closed By: fvogel
    Closed on: 2017-05-28 18:20:39
Description:

Revised [text]: text disappears when widget is out of focus

Paste this in tclsh:

    package require Tk
    pack [text .t]
    .t insert end "Line 1"
    .t tag add stdout 1.0 end
    .t tag add sel 1.1 1.3

The selected text becomes invisible (drawn in white on a white background) when the text widget does not have the focus.

It's not the case with the legacy widget.

This is probably due to or at least connected with the new option -inactiveselectforeground

(note: this is a trimmed down case from what can be seen with wish on windows)

User Comments: fvogel added on 2017-05-28 18:20:39:

Fixed by [70f50bf6dd].


fvogel added on 2017-05-28 15:25:09:

Commit [b2f64dc3df] freaks three tests from the test suite on Windows:

==== textDisp-2.23 LayoutDLine, spacing options FAILED
==== Contents of test case:

    .t configure -wrap word
    .t delete 1.0 end
    .t tag delete x y
    .t insert end "Short line\nLine 2 is long enough "
    .t insert end "to wrap around a couple of times"
    .t insert end "\nLine 3\nLine 4"
    set i [.t dlineinfo 1.0]
    set b1 [expr [lindex $i 1] + [lindex $i 4]]
    set i [.t dlineinfo 2.0]
    set b2 [expr [lindex $i 1] + [lindex $i 4]]
    set i [.t dlineinfo 2.end]
    set b3 [expr [lindex $i 1] + [lindex $i 4]]
    set i [.t dlineinfo 3.0]
    set b4 [expr [lindex $i 1] + [lindex $i 4]]
    .t configure -spacing1 4 -spacing2 4 -spacing3 4
    .t tag configure x -spacing1 1 -spacing2 2 -spacing3 3
    .t tag add x 1.0 end
    .t tag configure y -spacing1 0 -spacing2 3
    .t tag add y 2.19 end
    .t tag raise y
    set i [.t dlineinfo 1.0]
    set b1 [expr [lindex $i 1] + [lindex $i 4] - $b1]
    set i [.t dlineinfo 2.0]
    set b2 [expr [lindex $i 1] + [lindex $i 4] - $b2]
    set i [.t dlineinfo 2.end]
    set b3 [expr [lindex $i 1] + [lindex $i 4] - $b3]
    set i [.t dlineinfo 3.0]
    set b4 [expr [lindex $i 1] + [lindex $i 4] - $b4]
    list $b1 $b2 $b3 $b4

---- Result was:
1 3 4 5
---- Result should have been (exact matching):
1 5 13 16
==== textDisp-2.23 FAILED



==== textDisp-5.1 DisplayDLine, handling of spacing FAILED
==== Contents of test case:

    .t configure -wrap char
    .t delete 1.0 end
    .t insert 1.0 "abcdefghijkl\nmnopqrstuvwzyz"
    .t tag configure spacing -spacing1 8 -spacing3 2
    .t tag add spacing 1.0 end
    frame .t.f1 -width 10 -height 4 -bg black
    frame .t.f2 -width 10 -height 4 -bg black
    frame .t.f3 -width 10 -height 4 -bg black
    frame .t.f4 -width 10 -height 4 -bg black
    .t window create 1.3 -window .t.f1 -align top
    .t window create 1.7 -window .t.f2 -align center
    .t window create 2.1 -window .t.f3 -align bottom
    .t window create 2.10 -window .t.f4 -align baseline
    update
    list [winfo geometry .t.f1] [winfo geometry .t.f2]  [winfo geometry .t.f3] [winfo geometry .t.f4]

---- Result was:
10x4+24+11 10x4+55+16 10x4+10+53 10x4+76+50
---- Result should have been (exact matching):
10x4+24+11 10x4+55+16 10x4+10+47 10x4+76+44
==== textDisp-5.1 FAILED



==== textDisp-22.9 TkTextCharBbox, handling of spacing FAILED
==== Contents of test case:

    .t configure -wrap char
    .t delete 1.0 end
    .t insert 1.0 "abcdefghijkl\nmnopqrstuvwzyz"
    .t tag configure spacing -spacing1 8 -spacing3 2
    .t tag add spacing 1.0 end
    frame .t.f1 -width 10 -height 4 -bg black
    frame .t.f2 -width 10 -height 4 -bg black
    frame .t.f3 -width 10 -height 4 -bg black
    frame .t.f4 -width 10 -height 4 -bg black
    .t window create 1.3 -window .t.f1 -align top
    .t window create 1.7 -window .t.f2 -align center
    .t window create 2.1 -window .t.f3 -align bottom
    .t window create 2.10 -window .t.f4 -align baseline
    update
    list [.t bbox .t.f1] [.t bbox .t.f2] [.t bbox .t.f3] [.t bbox .t.f4]  [.t bbox 1.1] [.t bbox 2.9]

---- Result was:
{24 11 10 4} {55 16 10 4} {10 53 10 4} {76 50 10 4} {10 11 7 15} {69 42 7 15}
---- Result should have been (exact matching):
{24 11 10 4} {55 16 10 4} {10 47 10 4} {76 44 10 4} {10 11 7 15} {69 36 7 15}
==== textDisp-22.9 FAILED


fvogel added on 2017-05-28 14:56:57:
Confirmed to be fixed, thanks.

gcramer added on 2017-05-26 14:54:42:
The old handling/implementation of the selection options is tohubohu, so I used the opportunity to overwork it:

1. I added the tag options -inactivebackground and -inactiveforegound, and these options are tied to widget options -inactiveselectbackground and -inactiveselectforeground.

2. For symmetry reasons I added the tag options -inactiveselectbackground and -inactiveselectforeground, these options will overrule the options -inactivebackground and -inactivebackgound of the "sel" tag, provided that the actual tag has a higher priority.

3. The manual has been updated with new options. Furthermore section "THE SELECTION" has been refined.

4. In legacy widget tag option -selectbackground is tied to widget option -selectbackground if the tag option -selectbackground is not null, otherwise the widget option is tied to tag option "-background", this is very confusing, and not conform to documentation, this binding has been changed. Now the widget option -selectbackground is tied with tag option "-background" (of the "sel" tag), this is conform to (revised and legacy) documentation, it is a clear behavior, and allows more freedom in configuration. The tag options "-selectbackground" and "-selectforeground" now will overrule the options "-background" and "-foreground" of the "sel" tag, provided that the actual tag has a higher priority.

5. I changed test case textTag-5.23 according to (3). Moreover this test case has been extended for testing all bindings. BTW: Test case text-5.24 has been removed, it was a duplicate of prior textTag-5.23.

6. Complete rework of function MakeStyle(), the "sel" tag now will be handled separately, after all other tags have been processed, this makes it easier to follow the flow.

7. The old implementation has an erroneous resource management with the shared (tied) options of the "sel" tag and the selection options of the widget. This has been replaced with a proper implementation. Unfortunately the new implementation for resource management of shared options is a bit tricky, because the option table does not support shared options.

8. DEF_TEXT_INACTIVE_SELECT_BG_COLOR has been set to NULL for Windows, this should finally fix the issue of this bug report.

These changes has been committed with [b2f64dc3].

fvogel added on 2017-05-24 13:35:44:
In my mind the "sel" tag is special, but it nevertheless complies with the tag priority rules identically as any other tag.

For me the current behaviour is OK.

Regarding the man page, in my understanding section "THE SELECTION" of the [text] man page does not really say anything about the priority of the "sel" tag.

I don't clearly see where "THE SELECTION" seems to imply that the "sel" tag could always have the highest priority, except perhaps when it says that "the -inactiveselectbackground option for the text widget is used instead of -selectbackground when the text widget does not have the focus"? OK, here we could maybe append "(depending on the priority of the sel tag compared to the priority of other tags)" or something alike better phrased.

As a final note, "THE SELECTION" could say a word about -inactiveselectforeground since it already explains -inactiveselectbackground.

gcramer added on 2017-05-24 12:34:34:

Before I do a fix an unclear behaviour has to be clarified. See the following script:

pack [text .t]
.t insert end "Line 1"
.t tag configure stdout -background yellow
#.t tag raise sel
.t tag add stdout 1.0 end
.t tag add sel 1.1 1.3
focus .t

This shows a line with yellow background, the selection background is ignored, but the selected text is displayed with the selection foreground.

  1. I would expect that the selection foreground will be ignored if the selection background is ignored, although this behaviour would be an exceptional case, because normally the tag priority is decisive.
  2. The current behaviour for the selection background is okay, because if the user wants that the selection background has higher priority then he can raise the sel tag. Otherwise, if I read the chapter "THE SELECTION", then it sounds as if the sel tag attributes have highest priority anyway. But this would restrict the formatting capability. Probably the section "THE SELECTION" has to be changed a bit, to make it clear that the priority of the tags is decisive.

What do you think?


gcramer added on 2017-05-14 15:45:21:

Your patch might work with specific test cases, but it isn't fixing, for instance this test script does not work (under Linux):

set font [list [font configure TkFixedFont -family] -14]
pack [text .t -font $font -width 20 -height 5]
.t configure -selectbackground blue -selectforeground #c3c3c3
.t configure -inactiveselectforeground blue
for {set i 1} {$i < 5} {incr i} { .t insert end "This is line $i\n" }
.t tag add sel 2.0 4.0
focus .t

I have to overwork the design/implementation a bit, because I didn't know that the Windows version has a different behavior than Linux.


fvogel added on 2017-05-14 15:02:10:

The following patch solves the problem:

Index: generic/tkTextDisp.c
==================================================================
--- generic/tkTextDisp.c
+++ generic/tkTextDisp.c
@@ -1908,11 +1908,11 @@
            if (tagPtr->selBorder) {
                border = tagPtr->selBorder;
            }
            if (tagPtr->selFgColor != None) {
                fgColor = tagPtr->selFgColor;
-           } else if (fgColor == None) {
+           } else if (fgColor != None) {
                fgColor = textPtr->selFgColorPtr;
            }
        }
        if (border && priority > prio[PRIO_BORDER]) {
            styleValues.border = border;

I don't merge because I find strange that you didn't see this during your development, and would like to get your opinion first.


fvogel added on 2017-05-14 14:24:27:

Note that it's not just a matter of the default value for -inactiveselectforeground.

To disappear, the characters need to be tagged by something (with default values for tag options), AND be tagged as sel.

If the other tag is not present (remove .t tag add stdout 1.0 end from the test script, or add .t tag remove stdout 1.0 end at its end), there is no problem.

Also, if the other tag has a non-empty foreground (e.g. add .t tag configure stdout -foreground blue), there is as well no problem.


fvogel added on 2017-05-14 12:04:42:
Does not appear to change anything.

gcramer added on 2017-05-14 11:52:23:
Then I have chosen the wrong default foreground color for DEF_TEXT_INACTIVE_SELECT_FG_COLOR (Windows), I've changed it to "SystemWindowText", this should show the same foreground color as legacy text widget when unfocused. This fix is committed with [73bbb6f1].

fvogel added on 2017-05-14 07:42:14:
I'm attaching four snapshots of the text widget, legacy code and revised code, in focus and out of focus.

gcramer added on 2017-05-13 21:10:49:
Currently I cannotunderstand, -inactiveselectforeground has the same default as -selectforeground, this is SystemHighlightText under Windows, and I didn't change the default. The background color for the selection is -inactiveselectbackground if the widget don't has the focus, and this default didn't change, too.

In which way works the legacy text widget? Did it use another background compared to focused windows, or did it change the foreground?

Attachments: