GNOME Bugzilla – Bug 651813
st-scroll-view: Make fade offset themable
Last modified: 2011-06-04 19:44:34 UTC
See patch. This is part of helping out jimmac for bug 640271.
Created attachment 189186 [details] [review] st-scroll-view: Make fade offset themable While before there was a hard limit of 68 pixels on the fade offset on scrollviews, now themes can use the property '-st-fade-offset' to specify the the length of the fade effect.
Review of attachment 189186 [details] [review]: Looks good.
Created attachment 189188 [details] [review] st-scroll-view: Make the fade effect and offset themable Theme authors now have the power (and responsibility) of creating fade effects with the new length property '-st-fade-offset'. A value of 0 disables the effect. The new class name 'fade' appears on elements that had the vfade effect before. Alternate implementation, from suggestions from drago01 and Florian. This is a bigger patch.
Created attachment 189198 [details] [review] st-scroll-view: Make the fade effect and offset themable Theme authors now have the power (and responsibility) of creating fade effects with the new length property '-st-fade-offset'. A value of 0 disables the effect. The new class name 'fade' appears on elements that had the vfade effect before. Fixed a critical that should never had happened.
Review of attachment 189198 [details] [review]: Looks good overall, some nitpicks thought: Re commit message: Instead of talking about "theme authors" you should mention that this is a css property and that the vfade (gobject) property is removed with this patch- And you have to update test/interactive/scroll-view-sizing.js as well. ::: data/theme/gnome-shell.css @@ +40,3 @@ } +StScrollView.fade I'd indicate that this is only in vertical direction by keeping the "v". We probably want to add a horizontal version as well later and having this called "fade" is just confusing. (That's why the property was called vfade). @@ +42,3 @@ +StScrollView.fade +{ + -st-fade-offset: 68px; Same here. ::: src/st/st-scroll-view.c @@ +151,2 @@ * @self: a #StScrollView + * @fade_offset: The length of the fade effect. You should add a short description what this actually does. @@ +153,3 @@ */ +static void +st_scroll_view_try_vfade_effect (StScrollView *self, I am not sure I like the "try" here ... "set" would be misleading as it does not directly set the effect ... maybe "update" ?
Created attachment 189228 [details] [review] st-scroll-view: Make the fade effect and offset themable Theme authors now have the power (and responsibility) of creating fade effects with the new CSS length property '-st-fade-offset'. A value of 0 disables the effect. This new CSS approach replaces the current programmatic toggle of the 'vfade' property. A new CSS style class name 'vfade' is used as a replacement for the old property. I'm obsoleting the first patch, it seems this approach is liked more.
Review of attachment 189228 [details] [review]: Looks good, fine to commit with the change below: ::: src/st/st-scroll-view.c @@ +154,3 @@ + * Enable or disable the fade effect depending on the fade_offset: + * if it's 0, the effect is disabled, otherwise, it determines + * the length of the fade effect in pixels. I'd write: Sets the height of the edge fade area in pixels. A value of 0 disables the effect.
Attachment 189228 [details] pushed as e728937 - st-scroll-view: Make the fade effect and offset themable Committed with suggested docstring change.