GNOME Bugzilla – Bug 345778
Patch for new GtkExpander style property
Last modified: 2007-06-15 09:02:46 UTC
We would like to implement a new style property for GtkExpander. The enhancement will us to use the widget in a snazzy new palette for Glade. The style property is named "bold-background" and is a gboolean type. The property optionally enables the following behaviour: 1. A dark flat box is drawn in the widget background. 2. No prelight flat box is drawn when the mouse pointer hovers over the widget. In addition to a patch, I am attaching an example of what the proposed enhancement will look like.
Created attachment 67910 [details] What the proposed enhancement will look like
Created attachment 67911 [details] [review] A patch to implement proposed enhancement
This doesn't look good to me, it adds far too much overhead to the widget. If the expander were to just paint the flat box no matter what (probably with SHADOW_NONE unless prelit), it would look the same and get by without the added style property. As far as I can see, themes would look the same and all the same features would be possible.
Actually it wouldnt behave the same. Currently the expander only draws the prelight state, we want the flat box to be there - and no highlighting on prelight state (try a bold background that prelights and you'll see its just unnatural and weird - they should be exclusive). On the other hand - would it be better if we we're to - Do as you say and always draw the box (assuming here that the box would not appear unless you specified a colour for it that is different than the background) - Add a "can-prelight" style property to the expander to avoid the prelight state when we set the background colour to something "flat" ? Although this doenst make sence to me, because I think that the draw_flat_box() logic chooses an appropriately themed colour (like the same one in the trough area of a GtkScale for example), whereas always drawing a themable coloured box would have a different effect. maybe we have a misunderstanding :-/
To clarify, I'm saying that always drawing a flat box would: - look the same on almost all themes (the expander is most likely inside a window or similar, where the background of the expander has been drawn with a _flat_box call of onw of the parents already) - be less code to maintain and understand, both for Gtk devs and theme writers - allow all the same functionality than using such an option, since every theme (or theme engine) is free to just do nothing when detail == "expander" That's why I'm objecting to adding that style property. But I think it's very useful to always draw the flat box. It's bad behaviour (from my themeing point of view) if widgets draw certain style elements only in certain cases. (Another example for this is GtkMenuItem, which only draws its box in certain states.) It's much better if widgets draw the same style elements and allow the theme to filter them out, since that allows much more funtionality in the theme.
Heh, I did understand the reason for the objection - I just had a hard time understanding how to achive the same effect using your idea... I think I've figured it out what you are suggesting now - altough I'm still uncertain that your suggested approach is possible. I toyed around with gtkexpander.c and here are my findings: - The "prelight" state of the expander is a boolean on the private data struct, the GtkExpander never actually goes into prelight state itself (when we paint the prelight on the expander, its a hardcoded GTK_STATE_PRELIGHT). - The submitted patch above additionally draws the expander box with a hard-coded GTK_STATE_ACTIVE regardless of the expander's state. - So I tried doing the following: o remove the notion of the ->priv->prelight variable and set the expander widget's state to GTK_STATE_PRELIGHT upon enter_notify() (and reverse on leave_notify() - while preserving the manual propagation of the prelight state to the expander label itself) o always draw the flat box using the widget's current state (this is what I think you are suggesting btw... correct me if I'm wrong please) This produced seemingly good results - untill I added a GTK_NO_WINDOW child widget to the expander - in which case the whole expander widget prelights and no difference is made between the flat box and the rest of the widget. Ofcourse the oposite approach is worth mentioning; i.e. drawing a flat box hardcoded to GTK_STATE_NORMAL *all the time* just underneath the expander child - and let the style engine control the label/box area, that would potentially work but just seems a little backwards to me, not to mention it would mean more pixels drawn out by hand on every expose :( Are there any other alternatives ? so far it seems that manually drawing a hardcoded prelight/active box depending on enter/leave notify events and style properties is in fact the most straight forward way with the least overhead.
Some small comments: Can you post the patch you played with and what examples you tried it with? I'd like to test it myself. > not to mention it would mean more pixels drawn out by hand on every expose > Drawing a flat box takes practically no time, so it does not really matter if you draw 10 of them on top of each other - especially if it's so slow. See http://benjamin.sipsolutions.net/theme-perf/tests/window-background.png for how long it takes theme engines to draw a 600x400 flat box. So if you can enhance theme functionality by drawing flat boxes, I'm all for it. > so far it seems that manually drawing a hardcoded prelight/active box depending > on enter/leave notify events and style properties is in fact the most straight > forward way with the least overhead. > Until you think about educating theme creators about style properties and their effects on drawing routines of widgets. I think you can currently count the number of people on one hand that understand most of the style properties. And those are mostly not artists but coders. So my objection to adding style properties (not only here, but on all widgets) is that they are often very hard to get across to theme creators. And "draw a bold background" is not a good description for something. After thinking about what you want to do by looking at your attached screenshot, it seems to me you want to use a different background for certain widgets (in this case an expander) to highlight them. A style property however is something that themes should set, not something applications should set. So if that is what you want, you're looking at the wrong thing IMO.
Created attachment 71578 [details] example I think you want to have behaviour like the attached program, is that right? In that case, why not do it like the attachment? ;)
Well thats an interesting trick you played there. an additional hack would be needed to recycle the active state to override the prelight state on expander->style, in order to avoid the prelighting, and ofcourse trap theme changes in order to refresh your hacked colours and so and so (same would have to done for the child "label-widget" since the expander forces it into prelight state) - hardly an attractive hack to achieve such a basic effect. On the other hand, there's a good reason the gtk+ rc parsing engine has priority levels built in, so that apps _can_ override certain elements of the theme that are appropriate to thier particular context. In this case, it makes sence to let the artist decide what is the colour of "ACTIVE", and it is up to the programmer to decide to use this "ACTIVE" colour to draw the background of a slider trough, the downstate of a button or the box highlight on an expander. I think the decision to use an expander of this style should be in the hands of the person designing the UI, and that that person should certainly not be depraved of using style properties.
Generally I'd always opt for the style property variant. Your patch might look good with the engines/themes you've tried it but you never know what crazy ideas themers will have next. (There is momentum gathering for sharing theming know how at http://gnomethemes.org/forum/ )
(In reply to comment #2) > Created an attachment (id=67911) [edit] > A patch to implement proposed enhancement this patch implements the behaviour as a style property wich is only useful if *all* expanders in an application are supposed to look like in your screenshot, and if expanders where to have this new look or not, solely based on the current theme. as i understand, you only mean to use this new expander style for your pallette though (and probably do that in all themes), and in other dialogs still use regular expanders (e.g. if an expander is part of the standard file chooser). then, this proeprty should be an object property. (In reply to comment #8) > I think you want to have behaviour like the attached program, is that right? > In that case, why not do it like the attachment? ;) [attachment uses a GtkEventBox as parent with modified state/bg] the problem doing it this way is the prelighting the expander will still do. basically, tristan is right in wanting to add this functionality to the expander itself. in general, i think that if a feature is really useful in an application, can reasonably be implemented and maintained in the toolkit, and doesn't interfere with overall UI behaviour principles, it should be accepted for inclusion.
(In reply to comment #7) > And "draw a > bold background" is not a good description for something. i also think that "bold-background" is a bit odd. better might be "solid-background", to reflect that it's always there and doesn't change in prelight mode.
Seems to me that drawing anything without regard to its state (as this patch does) is a potentially theme-breaking thing. For instance in some HighContrast themes, if you draw the background in STATE_ACTIVE for all states, you will be negatively impacting visibility. So I don't think this is reasonable - you should always respect the current STATE when selecting a color. The already-existing expander has a problem with this too, and it is causing problems for the 'accessibility' themes. Let's not repeat/reinforce the error here... thanks
(In reply to comment #13) > Seems to me that drawing anything without regard to its state (as this patch > does) is a potentially theme-breaking thing. For instance in some HighContrast > themes, if you draw the background in STATE_ACTIVE for all states, you will be > negatively impacting visibility. you are very vague here Bill. if you see any concrete problems, please provide an accurate description. per-se, drawing things with STATE_ACTIVE shouldn't impact visibility any worse than when drawing a depressed button, and if that can't be seen, your theme has problems in the first place. > So I don't think this is reasonable - you > should always respect the current STATE when selecting a color. well, the point here is to always draw the expander looking depressed-alike (as far as the background color is concerned), so you have to fix the state in order to implement the desired UI effect. > The already-existing expander has a problem with this too, and it is causing > problems for the 'accessibility' themes. Let's not repeat/reinforce the error > here... again, if you have concrete problems, go ahead and describe them or file bug reports about them. isolated vague remarks about something "having problems" are not helpful for anyone and certainly won't lead to issues being fixed. > thanks thanks, too ;)
Tim Janik wrote: > this patch implements the behaviour as a style property wich is only useful if > *all* expanders in an application are supposed to look like in your screenshot, > and if expanders where to have this new look or not, solely based on the > current theme. > as i understand, you only mean to use this new expander style for your pallette > though (and probably do that in all themes), and in other dialogs still use > regular expanders (e.g. if an expander is part of the standard file chooser). > then, this proeprty should be an object property. An object property would be just fine and have the desired effect. We thought it was a style property because it has to do with the appearance, and intended that the programmer would set the style property on particular widgets by hand, not the entire expander class - since you can skin widgets by name - and you can override parts of the theme by using rc_parse_string(). I guess the real problem with that is leaving the door open to theme creators to eventually set this style property on a class-wide basis - which would indeed be harmfull. Tim Janik wrote: > i also think that "bold-background" is a bit odd. better might be > "solid-background", to reflect that it's always there and doesn't change in > prelight mode. solid-background sounds fine to me - we spent alot of time trying to come up with an appropriate name and were not completely satisfied with it anyway :)
I don't think I was being vague. As a general rule, AND IN THIS SPECIFIC CASE, fixing the state of the 'background' part of a visual element without fixing the state of the foreground, will break themes. The flat background of expanders is key to the visibility of the expander. My comment belongs in this bug report, not a new one, because it points out a specific fault with the proposed patch. If you fix the state of the background you should fix the state of the foreground portion of the expander as well.
(In reply to comment #16) bill.haneman@sun.com wrote: > fixing the state of the 'background' part of a visual element without fixing > the state of the foreground, will break themes. [...] > If you fix the state of the background > you should fix the state of the foreground portion of the expander as well. thanks Bill, that's indeed a valid concern. the expander itself doesn't have to change drawing here, since it doesn't e.g. draw labels in foreground color, it's children do however. so basically,for a state-correct implementation, something similar to (attachment (id=71578) from comment #8) would have to be done. to sum up: - introduce a ::solid-background object property (name still debatable). - when ::solid-background is TRUE, the expander's widget state needs to be fixed to GTK_STATE_ACTIVE with gtk_widget_set_state(). - upon enter/leave/set_label_widget, the expander should only set STATE_PRELIGHT if it's not STATE_ACTIVE (i.e. ::solid-background==FALSE). - the drawing code still needs adaptions to always draw the bg. that way, STATE_ACTIVE (like STATE_PRELIGHT) propagates down the widget hierarchy originating at the expander and everything is drawn properly themable. (In reply to comment #15) Tristan Van Berkom wrote: > We thought it was a style property because it has to do with the appearance, > and intended that the programmer would set the style property on particular > widgets by hand, not the entire expander class - since you can skin widgets > by name - and you can override parts of the theme by using rc_parse_string(). application programmers should never fiddle around with style settings like this. the style settings are there for theming, and everything that applications change here will break *some* theme with a high probability.
Thanks Tim, your restatement of the problem sounds right to me. Propagating the state "downwards" through the rendering of the expander ought to maintain state-consistency. Andrew Johnson has been working on the hc-engine recently and has confirmed that there are problems with the existing expander rendering code in gtk+, when using the highcontrast themes (particularly HighConstrastInverse, I believe). The problems 'appear' to be related to the fixing of states and/or hard-wiring of colors; since this patch touches the same code it would be a good time to fix the root problem. In the hc themes, mixing states can be especially bad since the theme explicitly tries to choose color at the extremes/poles of the "value" spectrum, and therefore the foreground of one state can be the background of another, etc. I have a rather patched-up theme/engine system at the moment, so it will take me some time to test the patch for this bug to see if it regresses things. But the patch submitters can try with HighContrastInverseLargePrint and HighContrastLargePrint (theme details tab of gnome-theme-manager) and see how it looks to them, in the meantime. (If you wear glasses, take them off and make sure the screen is nice and blurry before you give your reply). This is a test which all developers touching the gtk+ engine or rendering code should do before finalizing a change. It doesn't make sense for me to always be the one making the call, or pointing out the potential problem.
(In reply to comment #17) > - when ::solid-background is TRUE, the expander's widget state needs to be > fixed > to GTK_STATE_ACTIVE with gtk_widget_set_state(). Unfortunately thats not good, the flat box in the expander widget is a decorative portion of the widget that needs to be painted in the ACTIVE colour, similar to how the trough area of a GtkRange must be painted in ACTIVE colour. If you set the expander state itself to ACTIVE, then the GTK_NO_WINDOW child widget will be painted on an ACTIVE background (and probably worse visual effects when the expander is not expanded and has a large size allocation). > - upon enter/leave/set_label_widget, the expander should only set > STATE_PRELIGHT > if it's not STATE_ACTIVE (i.e. ::solid-background==FALSE). > - the drawing code still needs adaptions to always draw the bg. > that way, STATE_ACTIVE (like STATE_PRELIGHT) propagates down the widget > hierarchy originating at the expander and everything is drawn properly > themable. Currently (before this patch) _only_ the label widget gets fixed to prelight state when the prelight box itself is being drawn (as it is part of the decorative portion of the expander). I would propose that in the case that ::solid-background == TRUE; the expander always must draw an ACTIVE box as it does in the attached patch - but furthermore it should go on to fix the child label widget into the ACTIVE state. When ::solid-background == FALSE we just continue painting a prelight box and fixing the child label widget into prelight state when there are mouseovers.
(In reply to comment #19) > (In reply to comment #17) > > - when ::solid-background is TRUE, the expander's widget state needs to be > > fixed > > to GTK_STATE_ACTIVE with gtk_widget_set_state(). > > Unfortunately thats not good, > the flat box in the expander widget is a decorative portion > of the widget that needs to be painted in the ACTIVE colour, similar > to how the trough area of a GtkRange must be painted in ACTIVE colour. or a button when it's depressed. > If you set the expander state itself to ACTIVE, then the GTK_NO_WINDOW > child widget will be painted on an ACTIVE background (and probably worse > visual effects when the expander is not expanded and has a large size > allocation). right. there is no problem with this though, note that a child widget that doesn't paint the background itself, always has to have the same state as that parent container that does paint the background for it. that's the reason for propagating the state in the first place. or put simpler: you can't mix style->fg[state1] with style->bg[style2] (or base for that matter) if state1!=state2. > > - upon enter/leave/set_label_widget, the expander should only set > > STATE_PRELIGHT > > if it's not STATE_ACTIVE (i.e. ::solid-background==FALSE). > > - the drawing code still needs adaptions to always draw the bg. > > that way, STATE_ACTIVE (like STATE_PRELIGHT) propagates down the widget > > hierarchy originating at the expander and everything is drawn properly > > themable. > > Currently (before this patch) _only_ the label widget gets fixed to prelight > state when the prelight box itself is being drawn (as it is part of the > decorative portion of the expander). looking at the code, you're right. and that is indeed the problem that Bill described. the current expander code breaks with the normal Gtk+ drawing model assumptions and indeed has to be fixed to change it's own state to PRELIGHT upon enter/leave. otherwise, themes will continue to run into problems as described.
But that will not have a nice effect. When you mouse over the "clickable" expander area, you want that area to prelight; as it will activate the "expand/collapse" functionality. Since clicking the expander box and clicking the child button of the expander are two seperate things, you dont want the child button to prelight when you mouseover the expander box - you just want the expander box to prelight. If you fix the expander state to PRELIGHT: _everything_ will be prelighted, not just the label and box - that is the very good reason why its broken in the first place - although I still dont understand how exactly its broken, and why the "trough" area of a GtkRange is not equally broken, and what exactly the difference is. <fantasy> Ideally, the expander widget should be using some kind of decorative proxy child widget to pack the label into: GtkExpander (derives from hbox) contains: o GtkExpanderDecoration (derives from GtkBin and paints an arrow) o The child widget itself With a layout like that, one could easily just fix the state of the GtkExpanderDecoration without stomping on top of the state of the expander itself and its child widget, but thats a big big change that undoubtedly breaks the ABI. </fantasy>
(In reply to comment #21) > But that will not have a nice effect. > > When you mouse over the "clickable" expander area, you want that area to > prelight; as it will activate the "expand/collapse" functionality. > > Since clicking the expander box and clicking the child button of the > expander are two seperate things, you dont want the child button to > prelight when you mouseover the expander box - you just want the expander > box to prelight. child button? i don't think it makes sense at all for an expander to contain a child widget that processes input events on its own and thus does state changes like ACTIVE/PRELIGHT on its own (e.g. a GtkEntry, GtkButton or scrollbar). i.e. i'd expect an expander to reasonably only contain the type of children that you'd usually also pack into a button. what's the use case for a button as expander child? (real app screenshot?) > If you fix the expander state to PRELIGHT: _everything_ will be prelighted, > not just the label and box - that is the very good reason why its broken > in the first place - although I still dont understand how exactly its > broken, and why the "trough" area of a GtkRange is not equally broken, > and what exactly the difference is. yes, if a GtkRange took children as skip, the'd be ACTIVE with Gtk+'s current drawing model. > <fantasy> > Ideally, the expander widget should be using some kind of decorative > proxy child widget to pack the label into: > > GtkExpander (derives from hbox) contains: > o GtkExpanderDecoration (derives from GtkBin and paints an arrow) > o The child widget itself > > With a layout like that, one could easily just fix the state > of the GtkExpanderDecoration without stomping on top of the state > of the expander itself and its child widget, but thats a big big > change that undoubtedly breaks the ABI. > </fantasy> this would still have the same problem though, i.e. GtkExpander(Decoration) using STATE_ACTIVE background paintings, with a child painting in STATE_NORMAL on top of that. this bg/fg combination wouldn't mix properly. you can only expect the bg/fg/base/text members in a style to work together for the same state.
(In reply to comment #22) [...] > child button? i don't think it makes sense at all for an expander to contain a > child widget that processes input events on its own and thus does state changes > like ACTIVE/PRELIGHT on its own (e.g. a GtkEntry, GtkButton or scrollbar). i.e. > i'd expect an expander to reasonably only contain the type of children that > you'd usually also pack into a button. > what's the use case for a button as expander child? (real app screenshot?) Take the glade palette for example - it packs a table filled with buttons, both the table and buttons are GTK_NO_WINDOW widgets. And anyway that is besides the point, its not up to the gtk+ developers to decide what the users will pack into an expander - an expander has to "just work", otherwise you can expect it to break existing applications that depend on it to function properly. > > If you fix the expander state to PRELIGHT: _everything_ will be prelighted, > > not just the label and box - that is the very good reason why its broken > > in the first place - although I still dont understand how exactly its > > broken, and why the "trough" area of a GtkRange is not equally broken, > > and what exactly the difference is. > > yes, if a GtkRange took children as skip, the'd be ACTIVE with Gtk+'s current > drawing model. please excuse my illiteracy but I still dont understand a thing about that comment - what do you mean by "took children as skip" ? > > <fantasy> > > Ideally, the expander widget should be using some kind of decorative > > proxy child widget to pack the label into: > > > > GtkExpander (derives from hbox) contains: > > o GtkExpanderDecoration (derives from GtkBin and paints an arrow) > > o The child widget itself > > > > With a layout like that, one could easily just fix the state > > of the GtkExpanderDecoration without stomping on top of the state > > of the expander itself and its child widget, but thats a big big > > change that undoubtedly breaks the ABI. > > </fantasy> > > this would still have the same problem though, i.e. GtkExpander(Decoration) > using STATE_ACTIVE background paintings, with a child painting in STATE_NORMAL > on top of that. this bg/fg combination wouldn't mix properly. > you can only expect the bg/fg/base/text members in a style to work together for > the same state. While this is still a fantasy discussion down here - I wonder if you misunderstood me - the GtkExpanderDecoration would _be_ a child widget that contains a child "label-widget" and paints an arrow, this widget would have to be in gtk_widget_set_state (active/prelight) for the state to propagate correctly to the child label widget - without erronously propagating the state to the expander's child widget.
(In reply to comment #23) > (In reply to comment #22) > > i'd expect an expander to reasonably only contain the type of children that > > you'd usually also pack into a button. > > Take the glade palette for example - it packs a table filled with buttons, > both the table and buttons are GTK_NO_WINDOW widgets. uhm, sorry, we were talking about different children here. i was referring to the label_widget child the expander has. but of course the expander can also contain a regular child (wasn't thinking of that) that is shown/hidden (as opposed to hiding/showing a widget when expanding that is not its child). you're right that GTK_BIN (expander)->child should not change according to prelight/active painting. so yes, the state changes should only be applied to priv->label_widget (including the ACTIVE state), and that will work out correctly themable, as long as the expander paints the background area of label_widget with the state of the label_widget. > > > If you fix the expander state to PRELIGHT: _everything_ will be prelighted, > > > not just the label and box - that is the very good reason why its broken > > > in the first place - although I still dont understand how exactly its > > > broken, and why the "trough" area of a GtkRange is not equally broken, > > > and what exactly the difference is. > > > > yes, if a GtkRange took children as skip, the'd be ACTIVE with Gtk+'s current > > drawing model. > > please excuse my illiteracy but I still dont understand a thing about that > comment - what do you mean by "took children as skip" ? as a "skid", the thing that's moved around in the trough ;) [...] > While this is still a fantasy discussion down here - I wonder if you > misunderstood me - yes, as described above ;)
Created attachment 71878 [details] [review] Implements "solid-background" property on the expander This patch was written under the assumption that there is currently nothing wrong with the current expander themewise: Since the expander draws a flat box of PRELIGHT colour upon mouse-overs, it must fix the child label-widget's state to PRELIGHT - which it does. This patch adds a GObject property "solid-background" that will: o Draw the solid background always instead of sometimes the prelight o Fix the child label state to ACTIVE (which is the colour of the solid background) when appropriate o Update child label states appropriately when "label-widget" or "solid-background" is updated.
>This patch was written under the assumption that there is currently >nothing wrong with the current expander themewise: As I mentioned in comments 13 and 18, problems have been identified with the current expander and the HighContrastInverse themes. Andrew Johnson has been looking at that, cc-ing him for more input. Basically in the inverse themes the expander currently can "disappear" into the background under some conditions. It'd be good to know if this patch improves this or makes it worse; ideally we would fix the problem here (with this bug/patch) since it's closely related code.
Comment on attachment 71878 [details] [review] Implements "solid-background" property on the expander >@@ -66,6 +67,7 @@ struct _GtkExpanderPrivate > guint use_markup : 1; > guint button_down : 1; > guint prelight : 1; >+ guint solid_background : 1; > }; > > static void gtk_expander_set_property (GObject *object, >@@ -221,6 +223,14 @@ gtk_expander_class_init (GtkExpanderClas > GTK_TYPE_WIDGET, > GTK_PARAM_READWRITE)); > >+ g_object_class_install_property (gobject_class, >+ PROP_SOLID_BACKGROUND, >+ g_param_spec_boolean ("solid-background", >+ P_("Solid background"), >+ P_("Whether to draw a solid visible background instead of prelighting"), s/visible/distinct/ might be better here. >+ FALSE, >+ GTK_PARAM_READWRITE)); >+ > gtk_widget_class_install_style_property (widget_class, > g_param_spec_int ("expander-size", > P_("Expander Size"), >@@ -695,7 +711,7 @@ gtk_expander_unmap (GtkWidget *widget) > } > > static void >-gtk_expander_paint_prelight (GtkExpander *expander) >+gtk_expander_paint_flat_box (GtkExpander *expander, GtkStateType state) > { > GtkWidget *widget; > GtkContainer *container; >@@ -733,7 +749,7 @@ gtk_expander_paint_prelight (GtkExpander > area.height += !interior_focus ? (focus_width + focus_pad) * 2 : 0; > > gtk_paint_flat_box (widget->style, widget->window, >- GTK_STATE_PRELIGHT, >+ state, > GTK_SHADOW_ETCHED_OUT, > &area, widget, "expander", > area.x, area.y, >@@ -745,23 +761,27 @@ gtk_expander_paint (GtkExpander *expande > { > GtkWidget *widget; > GdkRectangle clip; >- GtkStateType state; >+ GtkStateType expander_state; > > widget = GTK_WIDGET (expander); > > get_expander_bounds (expander, &clip); > >- state = widget->state; >+ expander_state = widget->state; > if (expander->priv->prelight) > { >- state = GTK_STATE_PRELIGHT; >- >- gtk_expander_paint_prelight (expander); >+ expander_state = GTK_STATE_PRELIGHT; >+ >+ if (!expander->priv->solid_background) >+ gtk_expander_paint_flat_box (expander, GTK_STATE_PRELIGHT); > } > >+ if (expander->priv->solid_background) >+ gtk_expander_paint_flat_box (expander, GTK_STATE_ACTIVE); >+ i don't think this is correct, you rather need: GtkStateType expander_state = widget->state; if (GTK_WIDGET_IS_SENSITIVE (widget)) { if (expander->priv->solid_background) expander_state = GTK_STATE_ACTIVE; else if (expander->priv->prelight) expander_state = GTK_STATE_PRELIGHT; } if (expander_state == GTK_STATE_ACTIVE || expander_state == GTK_STATE_PRELIGHT) gtk_expander_paint_flat_box (expander, expander_state); and this is also the logic you need to set the label_widget state from. i.e. sensitivity takes precedence over ACTIVE takes precedence over PRELIGHT. you have to do this checking here, because you're bypassing widget->state (which usually does the sensitivity masking for you). > gtk_paint_expander (widget->style, > widget->window, >- state, >+ expander_state, > &clip, > widget, > "expander", >@@ -929,7 +949,7 @@ gtk_expander_enter_notify (GtkWidget > { > expander->priv->prelight = TRUE; > >- if (expander->priv->label_widget) >+ if (expander->priv->label_widget && !expander->priv->solid_background) > gtk_widget_set_state (expander->priv->label_widget, GTK_STATE_PRELIGHT); as said, you need logic similar to the above for the label_widget state here: GtkStateType expander_state = GTK_STATE_NORMAL; if (GTK_WIDGET_IS_SENSITIVE (widget)) { if (expander->priv->solid_background) expander_state = GTK_STATE_ACTIVE; else if (expander->priv->prelight) expander_state = GTK_STATE_PRELIGHT; } in contrast to the drawing above, expander_state is initialized with GTK_STATE_NORMAL here, because gtk_widget_set_state() will take care of masking STATE_NORMAL to STATE_INSENSITIVE if required. > > gtk_expander_redraw_expander (expander); >@@ -952,7 +972,7 @@ gtk_expander_leave_notify (GtkWidget > { > expander->priv->prelight = FALSE; > >- if (expander->priv->label_widget) >+ if (expander->priv->label_widget && !expander->priv->solid_background) > gtk_widget_set_state (expander->priv->label_widget, GTK_STATE_NORMAL); here too, label_widget state should be set from the logic outlined for gtk_expander_enter_notify(). > > gtk_expander_redraw_expander (expander); >@@ -1653,8 +1673,10 @@ gtk_expander_set_label_widget (GtkExpand > > gtk_widget_set_parent (label_widget, GTK_WIDGET (expander)); > >- if (priv->prelight) >+ if (priv->prelight && !priv->solid_background) > gtk_widget_set_state (label_widget, GTK_STATE_PRELIGHT); >+ else if (priv->solid_background) >+ gtk_widget_set_state (label_widget, GTK_STATE_ACTIVE); > } > > if (GTK_WIDGET_VISIBLE (expander)) >@@ -1684,6 +1706,62 @@ gtk_expander_get_label_widget (GtkExpand > > return expander->priv->label_widget; > } >+ >+ >+/** >+ * gtk_expander_set_solid_background: >+ * @expander: a #GtkExpander >+ * @solid_background: %TRUE if the expander should have a solid background >+ * >+ * If solid_background is set, a dark box will be drawn instead of the >+ * prelight state. not quite, since the box is also drawn when !prelight. e.g.: * If solid_background is set, the prelight area of an expander * will always be drawn in a distinct background color. however, i'm not entirely sure, that we really want no prelighting in case of ::solid-background=TRUE. this can be checked out once an initial patch has gotten into CVS though. >+ * >+ * Since: 2.12 >+ **/ >+void >+gtk_expander_set_solid_background (GtkExpander *expander, >+ gboolean solid_background) >+{ >+ GtkExpanderPrivate *priv; >+ >+ g_return_if_fail (GTK_IS_EXPANDER (expander)); >+ >+ priv = expander->priv; >+ >+ if (priv->solid_background != solid_background) >+ { >+ priv->solid_background = solid_background; >+ >+ if (priv->prelight && !priv->solid_background) >+ gtk_widget_set_state (priv->label_widget, GTK_STATE_PRELIGHT); >+ else if (priv->solid_background) >+ gtk_widget_set_state (priv->label_widget, GTK_STATE_ACTIVE); >+ else >+ gtk_widget_set_state (priv->label_widget, GTK_STATE_NORMAL); i don't think setting to NORMAL is really required here, you're not doing that in gtk_expander_set_label_widget() either. >+ >+ gtk_expander_redraw_expander (expander); >+ >+ g_object_notify (G_OBJECT (expander), "solid-background"); >+ } >+} >+ >+/** >+ * gtk_expander_get_solid_background: >+ * @expander: a #GtkExpander >+ * >+ * Return value: whether @expander draws a solid background. >+ * >+ * Since: 2.12 >+ **/ >+gboolean >+gtk_expander_get_solid_background (GtkExpander *expander) >+{ >+ g_return_val_if_fail (GTK_IS_EXPANDER (expander), FALSE); >+ >+ return expander->priv->solid_background; >+} the rest looks pretty ok to me.
(In reply to comment #26) > >This patch was written under the assumption that there is currently > >nothing wrong with the current expander themewise: > > As I mentioned in comments 13 and 18, problems have been identified with the > current expander and the HighContrastInverse themes. Andrew Johnson has been > looking at that, cc-ing him for more input. Basically in the inverse themes > the expander currently can "disappear" into the background under some > conditions. It'd be good to know if this patch improves this or makes it > worse; ideally we would fix the problem here (with this bug/patch) since it's > closely related code. Bill, rereading the code, the expander currently does one of: 1) not draw the background (relying on parent to clear the background according to widget->state) and then gtk_paint_expander(,widget->state,); 2) paint the expander background with STATE_PRELIGHT and then gtk_paint_expander(,STATE_PRELIGHT,); which both look ok with regards to state-handling for the painting. also, gtk_default_draw_expander() does correctly use the passed in state argument and not widget->state, so the actual rendering code shouldn't cause a problem either (unless you use a theme engine that has it's own implementation of style_klass->draw_expander). so, it'd really be good to hear more about the problem you have, since i don't currently have a good idea what could be causing it.
Why would anyone want a button (or clickable area in general) to not prelight? The expander area behaves like a togglebutton, and all buttons prelight when hovered over. This behaviour seems inconsistent to me and afaics is the only reason for the new property.
(In reply to comment #29) > Why would anyone want a button (or clickable area in general) to not prelight? > The expander area behaves like a togglebutton, and all buttons prelight when > hovered over. This behaviour seems inconsistent to me and afaics is the only > reason for the new property. i don't think it's such a clear call here, which is why i said whether we want prelighting for backgrounds that look like ACTIVE remains to be seen. gtk+ doesn't have a clear precedent here so far, currently we have various widgets that look like STATE_ACTIVE and have varying prelight behaviour: - GtkButton, when depressed looks ACTIVE but doesn't prelight the depressed area (prolly because it doesn't keep the depressed state across the point leaving the button area). - GtkToggleButton, when depressed looks ACTIVE and does the usual prelighting of the depressed area on enter/leaves. (of course, it *does* keep its depressed state across pointer leaves) - GtkRange, the trough is drawn with STATE_ACTIVE, prelighting is only applied to the skid though.
(In reply to comment #29) > Why would anyone want a button (or clickable area in general) to not prelight? > The expander area behaves like a togglebutton, and all buttons prelight when > hovered over. This behaviour seems inconsistent to me and afaics is the only > reason for the new property. It also behaves kindof like a treeview row, but the fact is that it is not a togglebutton and it is not a treeview - it is a "something new" that we've never seen before. Have you tried the patch ? the expander arrow /does/ still prelight btw... I'm not sure if I've stated this before so in the hope that I'm not sounding like a broken record: the rationale behind disabling the prelight box is that the solid-background already draws enough attention to the widget - adding an additional prelight of that entire box only makes it stick out like a sore thumb (yes, imho it behaves very obnoxiously if you combine the solid-background & the prelight box). (In reply to comment #27) > (From update of attachment 71878 [details] [review] [edit]) [...] > > i don't think this is correct, you rather need: > GtkStateType expander_state = widget->state; > if (GTK_WIDGET_IS_SENSITIVE (widget)) > { > if (expander->priv->solid_background) > expander_state = GTK_STATE_ACTIVE; > else if (expander->priv->prelight) > expander_state = GTK_STATE_PRELIGHT; > } > if (expander_state == GTK_STATE_ACTIVE || > expander_state == GTK_STATE_PRELIGHT) > gtk_expander_paint_flat_box (expander, expander_state); > > and this is also the logic you need to set the label_widget > state from. i.e. sensitivity takes precedence over ACTIVE > takes precedence over PRELIGHT. you have to do this checking > here, because you're bypassing widget->state (which usually does > the sensitivity masking for you). Uhh, yes and no - note that in that patch I renamed state --> expander_state; because I wanted store the target state of the "expander itself" (i.e. gtk_paint_expander)... in this way we draw a flat box of either PRELIGHT or ACTIVE or we dont draw any flat box at all (i.e. we shouldnt need to draw any flat box when insensitive), but we might draw an ACTIVE flat box _and_ a PRELIGHT gtk_paint_expander(). I hope that made sence - in other news... nice catch on the sensitivity checks, that was obviously missing from the previous expander code too. [...] > however, i'm not entirely sure, that we really want no prelighting in case > of ::solid-background=TRUE. this can be checked out once an initial patch > has gotten into CVS though. Initially we we're torn between using one or two properties (i.e. an additional "can-prelight"), we sided with simplicity and discarded the possibility that someone might want a solid-background that prelights on top of it - as noted above - it does still prelight on the expander arrow. Anyway, I'm open to suggestions - changes. > i don't think setting to NORMAL is really required here, you're not doing > that in gtk_expander_set_label_widget() either. set_label_widget() actually does restore the NORMAL state on the label widget before unparenting it - but thats beside the point, if the expander was previously set with ::solid-background = TRUE, the label-widget must be fixed back into a normal state when setting ::solid-background = FALSE. maybe it should be fixed not to NORMAL but to GTK_WIDGET (expander)->state ? At this point I'll refit this patch to take sensitivity into account everwhere appropriate and adjust the gtk-doc comments as you suggested.
(In reply to comment #31) > (In reply to comment #27) > > and this is also the logic you need to set the label_widget > > state from. i.e. sensitivity takes precedence over ACTIVE > > takes precedence over PRELIGHT. you have to do this checking > > here, because you're bypassing widget->state (which usually does > > the sensitivity masking for you). > > Uhh, yes and no - note that in that patch I renamed state --> expander_state; > because I wanted store the target state of the "expander itself" > (i.e. gtk_paint_expander)... in this way we draw a flat box of either > PRELIGHT or ACTIVE or we dont draw any flat box at all (i.e. we shouldnt > need to draw any flat box when insensitive), but we might draw an > ACTIVE flat box _and_ a PRELIGHT gtk_paint_expander(). > > I hope that made sence - ok, i'll reread your new patch with arrow prelighting in mind ;) > > i don't think setting to NORMAL is really required here, you're not doing > > that in gtk_expander_set_label_widget() either. > > set_label_widget() actually does restore the NORMAL state on the label widget > before unparenting it - but thats beside the point, if the expander was > previously set with ::solid-background = TRUE, the label-widget must be fixed > back into a normal state when setting ::solid-background = FALSE. > > maybe it should be fixed not to NORMAL but to GTK_WIDGET (expander)->state ? no, to STATE_NORMAL. that's because widget->state could be INSENSITIVE, e.g. due to a parent of the expander being insensitive. then you'd effectively set_state(label_widget, STATE_INSENSITIVE) but that's an alias for gtk_widget_set_sensitive (label_widget, FALSE); if you look at gtkwidget.c. so, when restoring the state on label_widget, use NORMAL and intentionally ignore sensitivity of the parent (i admit, all in all, state hanling here is not too easy ;-)
I think (and I tried it) that only prelighting the arrow is confusing, because it hgihlights something that can easily be >100 pixels away. I guess that's why checkboxes prelight the whole area and not just the checkmark. (side note: Ubuntulooks does not prelight checkbox backgrounds and it's weird from my pov) Also, prelighting is applied to all widgets in the sense that hovering the mouse over them changes the appearance of the whole widget. GtkToggleButton and GtkButton just use other means to achieve this. I particularly don't like the argument that drawing it as active makes it obvious that it is clickable. That doesn't work for me. I might check if it is by hovering, but if it doesn't change its appearance, it's a good indication it's not clickable. And range troughs should prelight if they can be clicked (they sometimes can't, in scales for example) - but I guess that's a different bug. Oh, and I guess Bill will complain that drawing a PRELIGHT expander on an ACTIVE background might not be visible (especially on HC themes ;))
Created attachment 71943 [details] [review] Refitted patch for "solid-background" This patch fixes the label widget state and draws a flat box taking widget sensitivity into account it also fixes the label-widget state from ::state_changed() it keeps the same old logic that avoids touching the label_widget state from enter/leave notify - seeing as you cannot recieve those events when in an insensitive state either. Also learnt a the meaning of a word today: "Ambivalent", it is basicly what we are in regards to the expander arrow itself prelighting, it seems ok when it does prelight - but it would also be ok if it didnt prelight at all in "solid-background" mode.
I was concerned about Bill's comments on accessibility. This eventually led me to implement an alternative expander using a GtkButton and GtkArrow. Attached is screenshot of the widget (currently used in glade). In my opinion it looks better than a GtkExpander with a dark background. I no longer see a need for the proposed API. Do we all want to close this bug as NOTABUG?
Created attachment 86714 [details] screenshot of an alternative expander
(In reply to comment #35) > I no longer see a need for the proposed API. Do we all want to close this bug > as NOTABUG? ok, agree, thanks for the resolution suggestion.