GNOME Bugzilla – Bug 635683
add specific button background for single button (per side) case
Last modified: 2011-01-05 16:39:59 UTC
I need this features for the gnome3 theme, otherwise I can't nicelly theme dialog windows which may have minimize and close button. At the moment if say right_middle_background and left_right_background will cover the case of say minimize and close, but if only close it's on the window the background drawn is the one defined in right_middle_background, which messes up things for my case and I have no way to fix it (other then forcefully disabling all buttons but close using blank draw_ops, but it's a hack and it's ugly). So something like function=right_single_background and function=left_single_background should be an ideal solution to do things cleanly.
Created attachment 175687 [details] [review] theme: Add background functions for single buttons With the existing background functions, single buttons can not be styled separately - on the left side, the style of the left button is picked, and the right button's style on the right side. As theme authors may want to add rounded corners to button groups as a whole, it makes sense to treat the case of a single button in a group differently. The patch depends on bug 635686.
Created attachment 175691 [details] [review] theme: Add background functions for single buttons Fixed a typo.
Created attachment 175748 [details] [review] theme: Add background functions for single buttons Adjusted to changes in bug 635686.
Review of attachment 175748 [details] [review]: Basically looks fine to me except for dependence on the patch I didn't like. ::: src/ui/theme.c @@ +6874,3 @@ + case META_BUTTON_TYPE_LEFT_SINGLE_BACKGROUND: + case META_BUTTON_TYPE_RIGHT_SINGLE_BACKGROUND: + return 3000; 3003 right?
(In reply to comment #4) > Review of attachment 175748 [details] [review]: > > Basically looks fine to me except for dependence on the patch I didn't like. > > ::: src/ui/theme.c > @@ +6874,3 @@ > + case META_BUTTON_TYPE_LEFT_SINGLE_BACKGROUND: > + case META_BUTTON_TYPE_RIGHT_SINGLE_BACKGROUND: > + return 3000; > > 3003 right? Not in its current form - currently only the major format number is saved in theme.format_version, so unless this is updated as well we have to return major_version * 1000.
(In reply to comment #5) > (In reply to comment #4) > > Review of attachment 175748 [details] [review] [details]: > > > > Basically looks fine to me except for dependence on the patch I didn't like. > > > > ::: src/ui/theme.c > > @@ +6874,3 @@ > > + case META_BUTTON_TYPE_LEFT_SINGLE_BACKGROUND: > > + case META_BUTTON_TYPE_RIGHT_SINGLE_BACKGROUND: > > + return 3000; > > > > 3003 right? > > Not in its current form - currently only the major format number is saved in > theme.format_version, so unless this is updated as well we have to return > major_version * 1000. I think the whole META_THEME_ALLOWS thing is busted. If you use something that isn't allowed, then the parser will catch it. And then what buttons have been defined should depend on what buttons have been defined, not the format version.
Created attachment 176025 [details] [review] theme: Add background functions for single buttons Rebased to latest patch in bug 635686. I agree that the whole version matching stuff is messy, but it's hardly in the scope of this bug :-)
(In reply to comment #7) > Created an attachment (id=176025) [details] [review] > theme: Add background functions for single buttons > > Rebased to latest patch in bug 635686. I agree that the whole version matching > stuff is messy, but it's hardly in the scope of this bug :-) (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Review of attachment 175748 [details] [review] [details] [details]: > > > > > > Basically looks fine to me except for dependence on the patch I didn't like. > > > > > > ::: src/ui/theme.c > > > @@ +6874,3 @@ > > > + case META_BUTTON_TYPE_LEFT_SINGLE_BACKGROUND: > > > + case META_BUTTON_TYPE_RIGHT_SINGLE_BACKGROUND: > > > + return 3000; > > > > > > 3003 right? > > > > Not in its current form - currently only the major format number is saved in > > theme.format_version, so unless this is updated as well we have to return > > major_version * 1000. > > I think the whole META_THEME_ALLOWS thing is busted. If you use something that > isn't allowed, then the parser will catch it. And then what buttons have been > defined should depend on what buttons have been defined, not the format > version. This is a red herring - META_THEME_ALLOWS isn't involved here. As far as I can tell, it should be possible in theme-parser.c, for: if (!meta_frame_style_validate (info->style, info->theme->format_version, error)) And: if (meta_theme_earliest_version_with_button (info->button_type) > info->theme->format_version) To use peek_required_version(info) instead and then you can properly return 3003.
Created attachment 177518 [details] [review] theme-parser: Use peek_required_version() for validation When validating button functions and frame styles, the required format version of the features used in the theme was compared to the major version number of the supported format, limiting additions to major theme format bumps. Use peek_required_version() instead, so the minor version number of the supported theme format is taken into account.
Created attachment 177519 [details] [review] theme: Add background functions for single buttons Updated to report the correct required version.
Review of attachment 177518 [details] [review]: Looks good
Review of attachment 177519 [details] [review]: So, if the theme doesn't specify single backgrounds, then this falls back to middle background? Wouldn't a single button have been drawn with left or right previously?
Comment on attachment 177518 [details] [review] theme-parser: Use peek_required_version() for validation Attachment 177518 [details] pushed as 4bc8c70 - theme-parser: Use peek_required_version() for validation
Created attachment 177534 [details] [review] theme: Add background functions for single buttons (In reply to comment #12) > Review of attachment 177519 [details] [review]: > > So, if the theme doesn't specify single backgrounds, then this falls back to > middle background? Wouldn't a single button have been drawn with left or right > previously? Right.
Review of attachment 177534 [details] [review]: Looks good
Attachment 177534 [details] pushed as 0a2bb1b - theme: Add background functions for single buttons