Tk Source Code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Tcl 2019 Conference, Houston/TX, US, Nov 4-8
Send your abstracts to [email protected]
or submit via the online form by Sep 9.
Ticket UUID: 509cafafae48cba46796e12d0503a335f0dcfe0b
Title: ttk::treeview tag configure not working since upgrade to 8.6.9
Type: Bug Version: 8.6.9
Submitter: j_4321 Created on: 2018-11-21 08:28:31
Subsystem: 88. Themed Tk Assigned To: fvogel
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2019-04-24 11:32:52
Resolution: Fixed Closed By: ulfalizer
    Closed on: 2019-04-24 11:32:52
Description:

Since I have upgraded to tcl/tk 8.6.9 (in Linux), ttk::treeview tag configure no longer works. The following code puts the background of the item in red and the foreground in yellow with tk 8.6.8 but not with tk 8.6.9

    package require Tk
    pack [ttk::treeview .t]
    .t insert {} end -text 1 -tags {test}
    .t tag configure test -background red -foreground yellow

User Comments: ulfalizer added on 2019-04-24 11:32:52:

For people using Tkinter, here's a workaround based on cjmcdonald's fix.

Maybe it should check the Tk version too, but I couldn't find anything more exact than "8.6" from some searching around.

def fixed_map(option):
    # Returns the style map for 'option' with any styles starting with
    # ("!disabled", "!selected", ...) filtered out

    # style.map() returns an empty list for missing options, so this should
    # be future-safe
    return [elm for elm in style.map("Treeview", query_opt=option)
            if elm[:2] != ("!disabled", "!selected")]

style.map("Treeview",
          foreground=fixed_map("foreground"),
          background=fixed_map("background"))
Improvement suggestions welcome.


fvogel added on 2019-04-07 14:20:04:
Thanks for your testing time, I appreciate it a lot.

ulfalizer added on 2019-04-07 14:12:23:
> I understand. I believe the fix we designed for this ticket does not introduce 
> any regression (or rather: removes the regression introduced in 8.6.9). Best 
> would be you try core-8-6-branch with your application so that we're even more 
> sure of this?

Just tried it out. Works fine!

fvogel added on 2019-04-07 11:36:52:
I understand. I believe the fix we designed for this ticket does not introduce any regression (or rather: removes the regression introduced in 8.6.9). Best would be you try core-8-6-branch with your application so that we're even more sure of this?

ulfalizer added on 2019-04-07 11:15:33:

Being able to use a different color for specific rows is very important in the application I'm working on too (a GUI version of this terminal utility - it's for the red entries that show up in show-all mode).

Whatever the mechanism, being able to style rows differently is a huge boon for me.

If background compatibility wasn't a concern, then maybe tags could be bound to styles, instead of being bound directly to colors and the like.


fvogel added on 2019-01-20 19:51:04:
Merged to core-8-6-branch and trunk. Thank you all for your participation, and special thanks to cjmcdonald for suggesting the fix.

anonymous added on 2019-01-01 18:21:08:

I am the author who requested the change in [882108bf05]

My understanding is the same as was written here already. The theming engine should layout and style the user interface. If the interface needs to be enhanced for special items, like cells of the tree view, the use of tags should override the base styling.

From what I have read so far the fix rom cjmcdonald for the regression seems most suitable.


fvogel added on 2019-01-01 16:37:21:

Many thanks for this clear explanation. And I think it's an excellent suggestion. I see no issue with this, and a lot of advantages (the ones you already mentioned).

I have committed this in the bugfix branch (bug-509cafafae) as [0951cc94].

I have tested this on Windows and I confirm your findings. Could anyone else have a look please?


cjmcdonald added on 2018-12-31 21:19:13:

Sorry for not being clear enough. What I am proposing is to remove the {!disabled !selected} statespec from the -background and -foreground state map, and then do not apply your patches on the [bug-509cafafae] bugfix branch. I don't think they're required, the problem being addressed in this ticket was caused by the unnecessary map entries.

So the winative theme state map becomes

ttk::style map Treeview \
    -foreground {disabled SystemGrayText \
	selected SystemHighlightText} \
    -background {disabled SystemButtonFace \
	selected SystemHighlight}

Similarily for the other themes. Then everything works in tk8.6.9 without any other changes:

1) The widget can still show a disabled state, as required by the original [882108bf05] ticket and [8a5d6b9a] bug fix, eg:

.t state disabled
.t state !disabled

2) The tagging in this ticket works again, showing yellow text and red background.

3) Selecting an entry shows the selected colours from the map, overriding any tag colours.

4) My desire to be able to change the treeview colours in a theme works:

ttk::style configure Treeview \
	-background green -foreground black

Which changes all non-tagged entries. Note that I don't need to do anything with the state map (unless I also want to change the disabled or selected colours). This works the same as tk8.6.8, before the [8a5d6b9a] bug fix.


fvogel added on 2018-12-30 14:31:10:

Thank you for your proposal, it helps!

My understanding is that you're suggesting:

- to keep my patches as they are in the bugfix branch

- in addition to that, to remove the {!disabled !selected} statespec from the -background and -foreground style map

If so, yes the script of the OP in the present ticket now works as he is expecting (item showing yellow text and red background, through tagging).

But it's still not possible to set the treeview background colour in a style (your comment dated 2018-12-20 11:16:51). This looks normal given your observation regarding memset(record, 0, tagTable->recordSize); (we could move this statement from Ttk_TagSetValues to Ttk_TagSetApplyStyle, BTW).

(Note: all this was observed on Windows running the winnative theme)

So I'm not sure your proposal enhances the status, unless I didn't understand what your proposal really is? For the sake of clarity, could you perhaps post a patch against the tip of the bugfix branch?


cjmcdonald added on 2018-12-20 12:50:33:

I may be missing something here, but the original [8a5d6b9a] fix appears to be more invasive than necessary, forcing the background & foreground colour to be set whenever the treeview widget is in the {!disabled !selected} state.

In tk8.6.8 defaults.tcl the treeview configuration for the default theme was:

	ttk::style configure Treeview \
	    -background $colors(-window) \
	    -foreground $colors(-text) ;
	ttk::style map Treeview \
	    -background [list selected $colors(-selectbg)] \
	    -foreground [list selected $colors(-selectfg)] ;

which with [8a5d6b9a] went to:

	ttk::style configure Treeview \
	    -background $colors(-window) \
	    -foreground $colors(-text) ;
	ttk::style map Treeview \
	    -background [list disabled $colors(-frame)\
				{!disabled !selected} $colors(-window) \
				selected $colors(-selectbg)] \
	    -foreground [list disabled $colors(-disabledfg) \
				{!disabled !selected} black \
				selected $colors(-selectfg)]

I don't see why the {!disabled !selected} entries are necessary, without it the colours for the normal state remain as set in ttk::style configure:

	ttk::style configure Treeview \
	    -background $colors(-window) \
	    -foreground $colors(-text) ;
	ttk::style map Treeview \
	    -background [list disabled $colors(-frame) \
				selected $colors(-selectbg)] \
	    -foreground [list disabled $colors(-disabledfg) \
				selected $colors(-selectfg)]

This still allows the disabled state to be set, but the OP's example at the top of this ticket now works.

Similarly for other themes?


cjmcdonald added on 2018-12-20 11:16:51:

With your patch applied it is no longer possible to set the treeview background colour in a style. For example, if I want all treeviews in my application to have a green background, matching some overall application style, then I might do:

package require Tk

ttk::style configure Treeview \ -background green -foreground black

ttk::style map Treeview \ -background [list disabled #cccccc \ {!disabled !selected} green \ selected blue] \ -foreground [list disabled #666666 \ {!disabled !selected} black \ selected white]

pack [ttk::treeview .t] .t insert {} end -text TestEntry1 .t insert {} end -text TestEntry2

This is for the "default" theme, on Linux. It works in tk 8.6.9 without your patch, but fails with your patch - the green background colour is ignored. Note that I'm not even using tags here, the entries are added without tags.

I think the problem is when you override styles with tag settings:

Ttk_TagSetApplyStyle(tv->tree.tagTable, style, state, displayItem); Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);

Looking in Ttk_TagSetValues there appears to be a global erase of the config settings:

memset(record, 0, tagTable->recordSize);

regardless of whether any tag settings apply or not. So the style settings are lost.


fvogel added on 2018-12-09 20:08:57:
No feedback, really?

If nobody speaks up I'll merge in a few days from now.

fvogel added on 2018-11-25 21:49:28:

I have committed my patch in branch bug-509cafafae, and have added a bit of documentation there as well.

Please have a look and test.

Right now, this is the best I can think of. Now we have the same behaviour of the treeview as in 8.6.8 (i.e. same as before [8a5d6b9a] was applied), except when the treeview is in the disabled state. In the disabled state the style still takes precedence over the tags, which means the {disabled selected} state will look as specified in the style (the tags will not be used). If we would not let this happen and would rather like to keep the selected rendering in the disabled state as it was before [8a5d6b9a], then this means we should revert [8a5d6b9a] and forget about the disabled state of the treeview completely rather than implementing function PrepareItem() to do the exact opposite of [8a5d6b9a].


anonymous added on 2018-11-25 08:56:19:
Excuse my butting in, but, may I add a few thoughts:

Any fix should IMHO reinstate the previous behaviour in 8.6.8
It should be assumed that any tags created by the programmer should be applied wherever possible.
It may be that some of the thoughts outlined below fall better into the category of 'enhancements'.

So perhaps the fvogel patch of 2018-11-22 21:13:10 would work in this context?

fvogel added on 2018-11-23 06:48:50:
Another approach: we could add a tag option specifying if the tag or the style has precedence, regardless of the state as a first approach. As a refinement this precedence flag could depend on the state, much like the statespec works in the style.

For each tag perhaps (and let prioritization by creation time play unchanged), or an option that would be global to all tags of the treeview.

This dumps the problem on the user's shoulder a bit, but at least it would be adjustable as wanted. The default should be such that old code has unchanged behaviour.

And explain all this in the man page, of course.

chw added on 2018-11-23 06:30:01:
Of course, should the "selected" tag visible on the Tcl level to make the
selection similar to the text and other widgets. Ideally, it's name is "sel"
(less to learn and to remember).

bll added on 2018-11-22 22:44:18:
That idea seems doable.

The internal selected tag would have to be inserted at the proper
spot in the tag list so that it has priority.  That part
might be a little messy.

Also note unless more messiness is added, the internal selected
tag would be visible to the user.

chw added on 2018-11-22 22:31:23:
Still unfinished: the "selected" tag inherits the "selected" style. It is only
present, when there's something in "selected" state.

chw added on 2018-11-22 22:27:07:
An unfinished thought: what about a temporary, special, highest priority
tag named "selected"? Would it make things simpler? Would it simplify
finding all selected items?

bll added on 2018-11-22 21:33:54:
Don't forget that ttk::treeview is also used as a spreadsheet, and users
like to color the contents of each cell.

If ttk::treeview was just a selection tool, yes it would be accurate
that ttk styling should take care of the appearance.  But it is used for
more than that.

I think your latest proposed patch is best.  And better than my proposal.

The user documentation needs to state that mixed tag styling and state
styling is not supported.

fvogel added on 2018-11-22 21:13:10:

Specializing for the 'selected' state only would be done through something like:

Index: generic/ttk/ttkTreeview.c
==================================================================
--- generic/ttk/ttkTreeview.c
+++ generic/ttk/ttkTreeview.c
@@ -1707,12 +1707,17 @@
     Treeview *tv, TreeItem *item, DisplayItem *displayItem)
 {
     Ttk_Style style = Ttk_LayoutStyle(tv->core.layout);
     Ttk_State state = ItemState(tv, item);

-    Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);
-    Ttk_TagSetApplyStyle(tv->tree.tagTable, style, state, displayItem);
+    if (state & TTK_STATE_SELECTED) {
+        Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);
+        Ttk_TagSetApplyStyle(tv->tree.tagTable, style, state, displayItem);
+    } else {
+        Ttk_TagSetApplyStyle(tv->tree.tagTable, style, state, displayItem);
+        Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);
+    }
 }

 /* + DrawCells --
  *     Draw data cells for specified item.
  */

That is: tag options have precedence over the style, except when in state 'selected'.

This would bring back the previous behavior we had before applying [8a5d6b9a]. OK, then we get a display change (compared to before [8a5d6b9a]) when the treeview is in state 'disabled': the selected item does no longer seem selected. This is nevertheless perhaps the least surprising fix?

I don't think that generalizing this approach of tweaking precedence order between tags and style as a function of the state will work: doing this we're just working against the styling engine, by specifically preventing it from doing its job whenever there is a tag applied to the considered entry. And I think it's not very sane to have such a silent strong link between the statespecs of the treeview and the function PrepareItem().

About your latest proposal below: this would really be our last resort, right? The very purpose of the theming/styling engine is to take care of the appearance of widgets without the programmer having to worry about the state they are in (see https://tkdocs.com/tutorial/styles.html). Could we really state that this applies only as long as there is no tag applied to a treeview item? Well, perhaps. But in this case, considering the backwards incompatibility, we would need to do this for 8.7 (and revert [8a5d6b9a] in core-8-6-branch).

So what other option do we have? Backout [8a5d6b9a] and just forget about the improvement it brought? Would be a pity...

I'm mixed. People reading this please step in and drop your thoughts.

In any case, we should take this opportunity to clarify the precedence order between tags and style in the man page of ttk::treeview. And the tags priority also.


bll added on 2018-11-22 19:33:19:
> Well, there is not just 'selected' to special case. What about 'active',
> 'hover', 'pressed' and maybe other states? Even 'disabled' is discussible: 
> should the "disabled" state take precedence (in it's styling effects) 
> over the tag specification? The answer is far from obvious. 

Enabling disabled has really opened a can of worms.

There are too many possible statespec combinations for the program to 
determine when to override.

Proposal:  Simply have a tag always override the styling, selected, disabled,
hover, whatever.  It just needs to be documented that the user, when using
tags to change the background/foreground styling, needs to be aware that 
they need to process the various state changes themselves.

This is still a backwards compatibility problem, as the "selected" state
will be different.  But, hopefully, will be reasonable for a majority of users.

fvogel added on 2018-11-22 18:55:25:

> The style sets the base style. 
> Tags are more specific and should override the base styling.

It looks easy to give precedence to tags over the style. The following patch achieves this:

Index: generic/ttk/ttkTreeview.c
==================================================================
--- generic/ttk/ttkTreeview.c
+++ generic/ttk/ttkTreeview.c
@@ -1707,12 +1707,12 @@
     Treeview *tv, TreeItem *item, DisplayItem *displayItem)
 {
     Ttk_Style style = Ttk_LayoutStyle(tv->core.layout);
     Ttk_State state = ItemState(tv, item);
 
-    Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);
     Ttk_TagSetApplyStyle(tv->tree.tagTable, style, state, displayItem);
+    Ttk_TagSetValues(tv->tree.tagTable, item->tagset, displayItem);
 }
 
 /* + DrawCells --
  *	Draw data cells for specified item.
  */

But with this, of course, when you hover or click an item you see no special visual effect, which is bad: the user receives no visual feedback. You said it already:

> The problem is "selected", which as a statespec would be 
> overridden by a tag.  But "selected" needs to override 
> the tag. So it seems that it should be applied as:
>    base styling with statespec, tag, "selected" statespec.

Well, there is not just 'selected' to special case. What about 'active', 'hover', 'pressed' and maybe other states? Even 'disabled' is discussible: should the "disabled" state take precedence (in it's styling effects) over the tag specification? The answer is far from obvious.

Regarding tags prioritization for the ttk::treeviwe it's pretty simple in the source code: tags are prioritized by creation order: the earlier the tag is created, the highest priority it has. At the programmer level, there is no way to change a tag priority (except to destroy all tags and recreate them in a different order).


bll added on 2018-11-21 21:03:48:
> Isn't it the style that should set the appearance of the treeview? That is the exact purpose of the styling engine, right? 

The style sets the base style. 
Tags are more specific and should override the base styling.

The problem is "selected", which as a statespec would be 
overridden by a tag.  But "selected" needs to override 
the tag. So it seems that it should be applied as:
   base styling with statespec, tag, "selected" statespec.

Special cases are bad, but if tags are wanted for styling
purposes I don't see another way.

fvogel added on 2018-11-21 20:55:21:

Other people found this bug as well: [d709297384]


chw added on 2018-11-21 20:11:35:
Very debatable, indeed. An argument in favor for tags: tags are more specific
since they can be applied to single items and thus should override more common
options. But this is only an opinion. And it immediately raises the next
question on how to prioritize tags. The text widget has "tag lower/raise"
for this reason.

fvogel added on 2018-11-21 19:14:14:

What's happening is that for the treeview [8a5d6b9a] had the consequence that -background/foreground tag options are now overriden by the statespec in the style.

From simply (say, with the vista theme):

ttk::style map Treeview \
	    -background [list selected $colors(-selectbg)] \
	    -foreground [list selected $colors(-selectfg)] ;

we now have:

	ttk::style map Treeview \
	    -background [list   disabled SystemButtonFace \
				{!disabled !selected} SystemWindow \
				selected SystemHighlight] \
	    -foreground [list   disabled SystemGrayText \
				{!disabled !selected} SystemWindowText \
				selected SystemHighlightText]

The style engine examines each statespec/value pair in order and uses the value corresponding to the first matching statespec.

Before the fix, the state considered in the statespec only was 'selected'. The non-selected state of the widget was not considered. I think that in this case the widget uses the -background/foreground tag options.

After the fix, the statespec specifies what to use as background and foreground in case {!disabled !selected} that is now specified. Since this is specified, the tag options are not used.

This is really a question of whether the tag options should take precedence over the style or not. This can really be discussed IMO. Isn't it the style that should set the appearance of the treeview? That is the exact purpose of the styling engine, right?

On the other hand, in this case the tag options -foreground/background should not be available.

Opinions?


fvogel added on 2018-11-21 15:46:00:

Oops, sorry. I'm really seeing the problem for all themes on Windows. Not sure how I originally concluded otherwise.

This is now easier to understand and to fix since there is no inconsistency among themes that received the same fix [8a5d6b9a].


fvogel added on 2018-11-21 15:30:49:

Indeed, this was introduced by [7f002659], which was merged into core-8-6-branch by [8a5d6b9a].

I can reproduce on Windows, however:

- Only the "vista" theme is incorrect
- All the other themes available on Windows (winnative clam alt default classic xpnative) are OK
- The aqua theme being non available on Windows I couldn't test it

From a quick inspection of the culprit commit I don't see at once what's wrong. The other themes received the same changes and they still work. I will dig further.


chw added on 2018-11-21 14:19:09:
Most likely this is caused by check-in [7f002659cfad068e] which changed
tree view state mapping. With the following script, previous behavior can
be restored by retaining states "disabled" and "selected" only.

apply {name {
    set newmap {}
    foreach {opt lst} [ttk::style map $name] {
        if {($opt eq "-foreground") || ($opt eq "-background")} {
            set newlst {}
            foreach {st val} $lst {
                if {($st eq "disabled") || ($st eq "selected")} {
                    lappend newlst $st $val
                }
            }
            if {$newlst ne {}} {
                lappend newmap $opt $newlst
            }
        } else {
            lappend newmap $opt $lst
        }
    }
    ttk::style map $name {*}$newmap
}} Treeview