GNOME Bugzilla – Bug 779653
Remove GtkShadowType & change set_shadow_type() to set_draw_border()/etc.
Last modified: 2018-05-02 18:15:13 UTC
The set_shadow_type() in GtkFrame, GtkScrolledWindow, and GtkViewport just * adds the .flat (Frame) or .frame (ScrolledWindow, Viewport) style class if the argument is GTK_SHADOW_NONE, * or removes it for any other shadow type. The other 4 GtkShadowTypes are all treated identically (removing said class) by GTK+, and they do not set any CSS classes that themes could use to differentiate between the various ShadowTypes. Therefore, GtkShadowType is mostly ineffective, and misleading as it really controls a simple no/yes on whether to draw a border/frame around these widgets. To make this clearer, I'd suggest replacing these set_shadow_type(GtkShadowType) methods with one taking a boolean indicating whether the border/frame should be drawn - something like set_draw_border(gboolean) or set_is_flat(gboolean). An alternative resolution is to add appropriate classes (e.g. .in, .out, .etched) on the relevant CSS nodes, which themes could use to make the other GtkShadowTypes actually accomplish something - but I'm guessing this is unlikely. I'm happy to work on this if a decision is made on how to proceed.
I don't think there's a desire on the theme side to differentiate this.
OK, thanks - that's the more difficult option, but the one I expected! Is draw-border an OK name for the property in all 3 widgets? That's what I've run with so far. That also assumes you want to keep the ability to add a border from code for the latter 2 widgets. I don't know whether that is widely used or considered useful?
Another question that seems relevant: Do we want to keep the current defaults for whether a border is drawn? If emulating the current behaviour (default shadow-type), the default for draw-border would be: • Frame: TRUE • ScrolledWindow: FALSE • Viewport: TRUE (albeit disabled by CSS if inside a ScrolledWindow)
Created attachment 347351 [details] [review] Frame: Replace :shadow-type with :draw-border
Created attachment 347352 [details] [review] ScrolledWindow: Replace :shadow-type with :draw-border
Created attachment 347353 [details] [review] Viewport: Replace :shadow-type with :draw-border
Created attachment 347354 [details] [review] gtkenums.h: Remove GtkShadowType
Created attachment 347355 [details] [review] Remove redundant calls to frame_set_draw_border (TRUE) Frame:draw-border defaults to TRUE, so these don’t do anything.
Created attachment 347356 [details] [review] Remove redundant calls to scrolled_window_set_draw_border (FALSE) ScrolledWindow:draw-border defaults to FALSE, so these don’t do anything
The above patches assume 'yes' to my questions so far. As for GTK+ 3,22, it behaves the same as GTK+ 4, but the documentation is not clear about this: > GtkShadowType: > @GTK_SHADOW_NONE: No outline. > @GTK_SHADOW_IN: The outline is bevelled inwards. > @GTK_SHADOW_OUT: The outline is bevelled outwards like a button. > @GTK_SHADOW_ETCHED_IN: The outline has a sunken 3D appearance. > @GTK_SHADOW_ETCHED_OUT: The outline has a raised 3D appearance. > > Used to change the appearance of an outline typically provided by a #GtkFrame. > > Note that many themes do not differentiate the appearance of the > various shadow types: Either their is no visible shadow (@GTK_SHADOW_NONE), > or there is (any other value). Themes physically cannot differentiate between the non-NONE types, because GTK+ does not add any classes or other means for the theme to discern between them. I dunno what the normal policy is, but my vote would go to rewording this to clarify that it is a piece of backwards-compatibility but nowadays only controls whether or not there is a border. NONE should be described as 'Draw a border' and all the rest as 'Do not draw a border.' And chiefly, the part that indicates that themes can somehow make use of the other types should go, as that isn't correct.
(In reply to Daniel Boles from comment #10) > NONE should be described as 'Draw > a border' and all the rest as 'Do not draw a border.' or rather, the opposite of that. duh.
Created attachment 347373 [details] [review] Frame: Replace :shadow-type with :draw-border fix widget-factory.ui to only have 2 frames (removing a last "shadow_type" which somehow eluded grep), rather than 4 redundant ones, and don't draw-border on one
Created attachment 347376 [details] [review] ScrolledWindow: Replace :shadow-type with :draw-border better documentation of the .frame style class to be used by themes
Created attachment 347377 [details] [review] Viewport: Replace :shadow-type with :draw-border better documentation of the .frame style class to be used by themes
Generally for boolean values: use guint value_name: 1; instead of a gboolean in structs. In setters, you should use value = !!value or value = value != FALSE; before saving it, otherwise the internal value can end up being not just 0 or 1. I suppose "draw-border" is not technically correct since you could still style it so it draws a border... I'm sure Benjamin has an opinion on this, though he might have an entirely different plan for this.
Good points, thanks! CCing Benjamin in case he has different ideas for what to do here
Created attachment 347382 [details] [review] Frame: Replace :shadow-type with :draw-border updating these 3 as per Timm's comments
Created attachment 347383 [details] [review] ScrolledWindow: Replace :shadow-type with :draw-border
Created attachment 347384 [details] [review] Viewport: Replace :shadow-type with :draw-border
Why do we want to have these calls? When is a developer supposed to use to set draw-border to TRUE/FALSE on each of these widgets?
(In reply to Benjamin Otte (Company) from comment #20) > Why do we want to have these calls? > When is a developer supposed to use to set draw-border to TRUE/FALSE on each > of these widgets? At the simplest level: TRUE when that dev previously set a non-NONE shadow-type, FALSE otherwise. Beyond that, we're on to debating whether users should have API to control whether these widgets get a border around them at all. For the current patches (again, without judging the merits of it) I just assumed being able to toggle the border on/off was something we'd want to retain, more from a perspective of retaining functionality than making strong judgments on how worthwhile it is. But I think there's an argument for at least some of it: For GtkFrame, being able to toggle the border with one call is much snappier than having to do get_style_context/add_class("flat"), especially because the HIG recommends borderless, which is not the default (yet?), so that needs to be done very often. For the other two widgets, I don't know how useful it is to maintain API for the border. Maybe in those cases, we could just require users to do it themselves by adding the generic .frame class via the StyleContext or in the .ui If you don't feel any of this is worth keeping around, sure, that's far easier. I was just hoping to clean up old API, whether that ends up being done by naming it less badly, or simply deleting it.
For Frame, it should always behave as if there is a border. In the case where developers don't want a border, they don't use a GtkFrame. For the scrolled window, you can achieve the same thing by putting it in a GtkFrame. For a viewport, I'm not even sure where the frame is supposed to be drawn - around the view window or around the bin window? One important thing: People setting style classes with GtkStyleContext is *always* a bug, so that is not the solution. I just want to be sure supporting different modes is what we want, because it means we need to adapt widget-factory to give theme developers a way to test these different modes and we need to make sure both regular and theme developers know when these properties are set and what for. And "draw a border" is a not a description I like - it describes what is done but not why it is done. I want a description that gives me an answer to the questions "Should the scrolled window in Nautilus/gnome-terminal/testgtk draw a border"?
(In reply to Benjamin Otte (Company) from comment #22) > For Frame, it should always behave as if there is a border. In the case > where developers don't want a border, they don't use a GtkFrame. I see your point. Do we take that further, though? Should frame still have a label-widget and [xy]align properties for it? (of which y is now meaningless) Or should people who want that use a Box, pack/align the label and child in it themselves, and put that Box in a Frame? Put simply, should Frame exist purely to draw a decorative border around one child? (since the option of adding the generic .frame class is a no-no) > For the scrolled window, you can achieve the same thing by putting it in a > GtkFrame. > > For a viewport, I'm not even sure where the frame is supposed to be drawn - > around the view window or around the bin window? haven't tested ViewPort in much detail, so can't say at the moment > I just want to be sure supporting different modes is what we want, because > it means we need to adapt widget-factory to give theme developers a way to > test these different modes and we need to make sure both regular and theme > developers know when these properties are set and what for. Yeah, I've tried to address this in the widget docs, as well as ensuring there are both with- and without-border cases in widget-factory. It's definitely _better_ than it was! Maybe not clear enough yet, still, though. I'm documenting it based on my own experience and way of thinking about it, which isn't necessarily most intuitive for everyone else. > And "draw a border" is a not a description I like - it describes what is > done but not why it is done. I want a description that gives me an answer to > the questions "Should the scrolled window in Nautilus/gnome-terminal/testgtk > draw a border"? Again, I've tried to document that these properties really control the addition of style classes, which /indicate/ that themes /should/ draw a border (if they want to) - as GTK+ will not enforce this itself. A lot of these questions are equally applicable to what we currently have, but if there's a chance to make it clearer in conjunction with the renaming, then that's even better.
(In reply to Daniel Boles from comment #2) > OK, thanks - that's the more difficult option, but the one I expected! Well, I didn't mean my comment as an encouragement for any changes in this area.
I don't think we need to do anything here. But if you get Benjamin and Tim to agree and things don't get changed in incompatible ways, then maybe. Just remember that we've documented all the style classes and the node hierarchy now, so we can't go willy-nilly changing things around anymore.
I get that. That's why, in case it wasn't clear, all that these patches do is to change the name and type of the property for each widget. After that, the code still adds the same style classes to the same nodes for the same widgets. It just doesn't use a property name and type that are total historical baggage (even in 3)
I'm generally not a fan of changing public API just to clean it up. Because it means every developer using that API will have to change his code when porting to GTK4 and see no benefit at all. You can kinda see this in your patches: All they do is rename things, but nothing but the name gets any better. Docs aren't clearer, it is still the same amount of code inside GTK and themes don't see any benefits either.
Again, is GtkFrame actually good for much now, after all the features that have been removed from it? (In reply to Daniel Boles from comment #23) > (In reply to Benjamin Otte (Company) from comment #22) > > For Frame, it should always behave as if there is a border. In the case > > where developers don't want a border, they don't use a GtkFrame. > > I see your point. Do we take that further, though? Should frame still have a > label-widget and [xy]align properties for it? (of which y is now > meaningless) Or should people who want that use a Box, pack/align the label > and child in it themselves, and put that Box in a Frame? > > Put simply, should Frame exist purely to draw a decorative border around one > child? (since the option of adding the generic .frame class is a no-no)
(In reply to Daniel Boles from comment #28) > Again, is GtkFrame actually good for much now, after all the features that > have been removed from it? I don't think anyone has a definitive answer for that yet. I'd say no, but I haven't checked what all the apps use. I do know that there's one of the gnome games that uses a GtkAspectFrame, but GtkFrame is generally used to just add a frame and we could probably keep it like that, remove the shadow-type property and be done with it. Then the label widget doesn't make a lot of sense either. IIRC inkscape used to use frames with labels a lot but I can't see them there anymore so it either changed or I misremember. I think we should just wait for a few apps to port to gtk4 and then see how/if they use it.
(In reply to Timm Bäder from comment #29) > I don't think anyone has a definitive answer for that yet. I'd say no, but I > haven't checked what all the apps use. I do know that there's one of the > gnome games that uses a GtkAspectFrame It's always seemed weird to me that AspectFrame is a Frame. I understand the desire for a widget that enforces an aspect ratio, but not why it has to be a Frame at the same time. To me these are orthogonal features. Maybe in GTK+ 4, AspectFrame can just be AspectBin or something? > but GtkFrame is generally used to > just add a frame and we could probably keep it like that, remove the > shadow-type property and be done with it. Then the label widget doesn't make > a lot of sense either. IIRC inkscape used to use frames with labels a lot > but I can't see them there anymore so it either changed or I misremember. > > I think we should just wait for a few apps to port to gtk4 and then see > how/if they use it. I'm torn between two points of view: one that strips out the rest of the leftover API that makes no sense given what's already been removed, and one that wishes it hadn't been removed in the first place! My current app has a fairly complex, dense interface consisting of blocks within blocks. I divide this up and provide visual distinction by taking advantage of Frames with borders, shaded backgrounds, and label-widgets that can be y-aligned centrally over the top border. This is currently the thing I'm dreading trying to port to GTK+ 4, as it looks like I won't be able to do it and will probably have to redesign those parts of the app. Then again, I'm not developing a core app and therefore am not strictly following the HIG, so I don't know whether anyone really cares about how I use GTK+!
(In reply to Daniel Boles from comment #28) > Again, is GtkFrame actually good for much now, after all the features that > have been removed from it? > You would use it to put a frame around a widget - or rather: group of widgets. How else would the theme know to put a border somewhere?
I would've just added the .frame style class to said widget/container, but you said that should never be done: (In reply to Benjamin Otte (Company) from comment #22) > One important thing: People setting style classes with GtkStyleContext is > *always* a bug, so that is not the solution. This seems odd to me. What, then, am I supposed to do with all the widgets in my app where I add/remove style classes to reflect/style their current state? The simplest example is just adding .(destructive|suggested)-action to buttons. Not everyone has custom widgets or uses .ui files, so then going via StyleContext seems to be the only way. Surely I'm missing something obvious.
Those should be public APIs exposed by GtkButton. I consider the fact that we expect applications to set undocumented style classes on certain widgets a bug (multiple actually: an API design bug, a documentation bug and a stability bug). Also note that I am talking about public APIs exposed to application developers here, not about people who design their own widgets. But when application developers compose an application out of the core GTK widgets, they should not need to know about style classes and all the styling they are meant to influence can be accessed using regular properties of their widget.
(In reply to Benjamin Otte (Company) from comment #33) > Those should be public APIs exposed by GtkButton. I consider the fact that > we expect applications to set undocumented style classes on certain widgets > a bug (multiple actually: an API design bug, a documentation bug and a > stability bug). Fair enough, that would indeed be nicer. > Also note that I am talking about public APIs exposed to application > developers here, not about people who design their own widgets. I guess my other use cases fall in the latter category. They are custom widgets in the sense that I aggregate sets of core widgets together to do boring app-specific things, and add style classes on e.g. the topmost container among those, so I can apply the right style for current state. I don't do any subclassing yet, though.
Use cases where you add your own CSS are also use cases for style classes. I was more thinking about exposing the functionality of the theme. So I imagine having a frame or a button::is-suggested-action property as a method for the application developer to use features expose by Adwaita etc.
(In reply to Benjamin Otte (Company) from comment #33) > Those should be public APIs exposed by GtkButton. I consider the fact that > we expect applications to set undocumented style classes on certain widgets > a bug (multiple actually: an API design bug, a documentation bug and a > stability bug). Just to correct this: the suggested-action and destructive-action style classes are not undocumented. https://developer.gnome.org/gtk3/stable/GtkButton.html#id-1.3.9.2.11.4
If that means it's not to be considered a bug, I can close the other ticket?
Nor are things like .frame or .flat undocumented, although the documentation does avoid overtly saying 'you can trust this to apply style X for your own purposes'. https://developer.gnome.org/gtk3/stable/GtkStyleContext.html#GTK-STYLE-CLASS-FRAME:CAPS
An alternative is to keep these ShadowTypes, and actually make them do something: We could make them add CSS classes for the relevant border styles, then have those CSS classes in our themes to apply the relevant values to the border-style property. I've recently demod the corresponding border-styles in widget-factory: https://git.gnome.org/browse/gtk+/commit/?h=gtk-3-22&id=66767adc63dd919504135c04dcbfe57737bac93b This would mean we could ensure they apply correctly to all cases, e.g. adding .border-outset to a Frame could be made to apply the border to its > border node (in GTK+ 3 only) rather than its main node, and so on. Thoughts on that approach?
I don't think we want to do that because during all of GTK3 nobody has ever requested that feature - and even during GTK2 it was barely used. I don't think we want to provide APIs that nobody is gonna use.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/778.