After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 651813 - st-scroll-view: Make fade offset themable
st-scroll-view: Make fade offset themable
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-06-03 21:48 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2011-06-04 19:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-scroll-view: Make fade offset themable (1.21 KB, patch)
2011-06-03 21:48 UTC, Jasper St. Pierre (not reading bugmail)
accepted-commit_now Details | Review
st-scroll-view: Make the fade effect and offset themable (8.55 KB, patch)
2011-06-03 22:34 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
st-scroll-view: Make the fade effect and offset themable (8.75 KB, patch)
2011-06-04 02:52 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
st-scroll-view: Make the fade effect and offset themable (10.00 KB, patch)
2011-06-04 18:49 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2011-06-03 21:48:08 UTC
See patch.

This is part of helping out jimmac for bug 640271.
Comment 1 Jasper St. Pierre (not reading bugmail) 2011-06-03 21:48:10 UTC
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.
Comment 2 drago01 2011-06-03 21:58:33 UTC
Review of attachment 189186 [details] [review]:

Looks good.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-06-03 22:34:37 UTC
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.
Comment 4 Jasper St. Pierre (not reading bugmail) 2011-06-04 02:52:31 UTC
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.
Comment 5 drago01 2011-06-04 17:47:25 UTC
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" ?
Comment 6 Jasper St. Pierre (not reading bugmail) 2011-06-04 18:49:02 UTC
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.
Comment 7 drago01 2011-06-04 19:05:06 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-06-04 19:44:32 UTC
Attachment 189228 [details] pushed as e728937 - st-scroll-view: Make the fade effect and offset themable


Committed with suggested docstring change.