GNOME Bugzilla – Bug 639460
StScrollView: Implement real fade effect
Last modified: 2011-01-20 19:54:13 UTC
The designers has been using one in the mockups [1] for a while, we used to fake it by using gradients/shadows. 1: http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/overview-application-picker.png
Created attachment 178258 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass.
Created attachment 178260 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass. -- *) Whitespace fixes
Created attachment 178276 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack.
Created attachment 178316 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack. --------- *) Small fixes (testcase) and cleanups
Created attachment 178320 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack. --- Fix typo
Created attachment 178568 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack. --- *) Rebased
Review of attachment 178568 [details] [review]: Generally looks good. I don't have a lot of comments on the patch. I'm still a bit concerned about performance, but from your tests earlier, it certainly doesn't seem like a big slowdown. I think it's probably worth only enabling the effect (clutter_meta_set_enabled()) when there are actually shadows. This will keep from having overhead when we have some area that is potentially scrolled but not actually scrolled. What I'd probably do is make the effect do it itself - to connect to CHANGED signal of StAdjustment and figure out whether the effect should be enabled or not. I'm not really happy with the vfade property name - really we should have a property name that describes the intent and not the effect - a property name that could apply both to the old shadows and the new fading - so we don't have to rename it again if we change the appearance again. vmark-scrolled-edges ? Not really that great either. Unless you have a better idea, the 'vfade' name is OK. ::: src/Makefile.am @@ +261,3 @@ --library=libst-1.0.la \ -DST_COMPILATION \ + $(filter-out %-private.h %/st-scroll-view-fade.h, $(addprefix $(srcdir)/,$(st_source_h))) \ Need to add st_non_gir_sources like shell_recorder_non_gir_sources, (would there be any real problem if this class was included in the gir, even if it is conceptually non-public?) ::: src/st/st-scroll-view-fade.c @@ +1,3 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* + * st-scroll-view-fade.h: Edge fade effect for StScrollView, based on ClutterBlurEffect This line is a short description of the source file - 'based on ClutterBlurEffect' isn't what this source file is. If you want to include the information put it on a separate line @@ +4,3 @@ + * + * Copyright (C) 2010 Intel Corporation. + * Copyright (C) 2011 Adel Gadllah Omit the (C), no legal meanings @@ +202,3 @@ + CoglHandle material; + + StScrollView *scroll_view = ST_SCROLL_VIEW(self->actor); missing space @@ +211,3 @@ + + g_object_get (scroll_view, "vscroll", &v_scroll, NULL); + g_object_get (v_scroll, "adjustment", &v_adjustment, NULL); These calls will return additional references to these objects, so you leak them. Rather than worrying about unreffing, use: st_scroll_view_get_vscroll_bar st_scroll_bar_get_adjustment which are also more type safe and more efficient. Also, note that your v_scroll and v_adjustment variables wth the with the v_ don't conform to existing convention. @@ +236,3 @@ + cogl_program_set_uniform_1f (self->program, self->width_uniform, clutter_actor_get_width (self->actor)); + if (self->scrollbar_width_uniform > -1) + cogl_program_set_uniform_1f (self->program, self->scrollbar_width_uniform, clutter_actor_get_width (CLUTTER_ACTOR(v_scroll))); missing space ::: src/st/st-scroll-view-fade.h @@ +4,3 @@ + * + * Copyright (C) 2010 Intel Corporation. + * Copyright (C) 2011 Adel Gadllah Some comments as the .c ::: src/st/st-scroll-view.c @@ +162,3 @@ void +st_scroll_view_set_vfade (StScrollView *self, + gboolean vfade) Identation @@ +809,3 @@ clutter_actor_set_parent (priv->vscroll, CLUTTER_ACTOR (self)); + priv->vfade_effect = NULL; Initializations to NULL aren't necesary and we generally don't do them.
(In reply to comment #7) > Review of attachment 178568 [details] [review]: > > Generally looks good. I don't have a lot of comments on the patch. I'm still a > bit concerned about performance, but from your tests earlier, it certainly > doesn't seem like a big slowdown. > > I think it's probably worth only enabling the effect > (clutter_meta_set_enabled()) when there are actually shadows. This will keep > from having overhead when we have some area that is potentially scrolled but > not actually scrolled. What I'd probably do is make the effect do it itself - > to connect to CHANGED signal of StAdjustment and figure out whether the effect > should be enabled or not. OK > I'm not really happy with the vfade property name - really we should have a > property name that describes the intent and not the effect - a property name > that could apply both to the old shadows and the new fading - so we don't have > to rename it again if we change the appearance again. vmark-scrolled-edges ? > Not really that great either. Unless you have a better idea, the 'vfade' name > is OK. I can't really think of anything that is descriptive but not too long. > ::: src/Makefile.am > @@ +261,3 @@ > --library=libst-1.0.la \ > -DST_COMPILATION \ > + $(filter-out %-private.h %/st-scroll-view-fade.h, $(addprefix > $(srcdir)/,$(st_source_h))) \ > > Need to add st_non_gir_sources like shell_recorder_non_gir_sources, (would > there be any real problem if this class was included in the gir, even if it is > conceptually non-public?) It wouldn't really be a problem, but I don't see a reason for doing so. It is an implementation detail of StScrollView. > ::: src/st/st-scroll-view-fade.c > @@ +1,3 @@ > +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ > +/* > + * st-scroll-view-fade.h: Edge fade effect for StScrollView, based on > ClutterBlurEffect > > This line is a short description of the source file - 'based on > ClutterBlurEffect' isn't what this source file is. If you want to include the > information put it on a separate line OK > @@ +4,3 @@ > + * > + * Copyright (C) 2010 Intel Corporation. > + * Copyright (C) 2011 Adel Gadllah > > Omit the (C), no legal meanings Well I generally don't add copyright lines, but since you added them in St a while ago I though we have a different policy for St, but well will remove them. > @@ +202,3 @@ > + CoglHandle material; > + > + StScrollView *scroll_view = ST_SCROLL_VIEW(self->actor); > > missing space OK > @@ +211,3 @@ > + > + g_object_get (scroll_view, "vscroll", &v_scroll, NULL); > + g_object_get (v_scroll, "adjustment", &v_adjustment, NULL); > > These calls will return additional references to these objects, so you leak > them. Rather than worrying about unreffing, use: > > st_scroll_view_get_vscroll_bar > st_scroll_bar_get_adjustment > > which are also more type safe and more efficient. Also, note that your v_scroll > and v_adjustment variables wth the with the v_ don't conform to existing > convention. OK > @@ +236,3 @@ > + cogl_program_set_uniform_1f (self->program, self->width_uniform, > clutter_actor_get_width (self->actor)); > + if (self->scrollbar_width_uniform > -1) > + cogl_program_set_uniform_1f (self->program, self->scrollbar_width_uniform, > clutter_actor_get_width (CLUTTER_ACTOR(v_scroll))); > > missing space OK > ::: src/st/st-scroll-view-fade.h > @@ +4,3 @@ > + * > + * Copyright (C) 2010 Intel Corporation. > + * Copyright (C) 2011 Adel Gadllah > > Some comments as the .c OK > ::: src/st/st-scroll-view.c > @@ +162,3 @@ > void > +st_scroll_view_set_vfade (StScrollView *self, > + gboolean vfade) > > Identation > > @@ +809,3 @@ > clutter_actor_set_parent (priv->vscroll, CLUTTER_ACTOR (self)); > > + priv->vfade_effect = NULL; > > Initializations to NULL aren't necesary and we generally don't do them. OK
Created attachment 178782 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack. -- *) Added singnal handler that enables/disables the effect when needed *) Fixed style issues *) Added st_non_gir_sources
Review of attachment 178782 [details] [review]: ::: src/Makefile-st.am @@ +169,3 @@ libst_1_0_la_SOURCES = \ $(st_source_c) \ + $(st_non_gir_sources) \ Line up the \ ::: src/st/st-scroll-view-fade.c @@ +102,3 @@ + gdouble value, lower, upper, page_size; + gboolean enabled, needs_fade; + enabled = clutter_actor_meta_get_enabled (CLUTTER_ACTOR_META (effect)); This isn't worthwhile, clutter_meta_set_enabled() internally short-circuits the no-change case. @@ +144,3 @@ + g_signal_connect (self->vadjustment, "changed", + G_CALLBACK (on_vadjustment_changed), + effect); This is too late. We've already created the offscreen fbo, etc. You need to set things up so that if no scrolling is present the ClutterMeta is disabled *before* we try to paint once. You also need to call the on_vadjustmnet_changed once after making the connection.
Review of attachment 178782 [details] [review]: ::: src/st/st-scroll-view-fade.c @@ +131,3 @@ + g_warning ("Unable to use the ShaderEffect: the graphics hardware " + "or the current GL driver does not implement support " + "for the GLSL shading language."); Oh, forgot to write up something in my first review. This warning is going to fire every time the actor paints. I don't see a point in a wanring at all, though we might want to check this feature when deciding if to enable the shader to avoid performance costs on marginal hardware. @@ +174,3 @@ + gchar *log_buf = cogl_shader_get_info_log (self->shader); + + g_warning (G_STRLOC ": Unable to compile the fade shader: %s", As will this - I think GLSL implementations are allowed to refuse to compile shaders for arbitrary reasons, so you can't crazily spew in this case - you need to set things up to only try and compile once. Maybe even use a static variable so you don't try and compile shaders for each scroll view we create.
(In reply to comment #11) > Review of attachment 178782 [details] [review]: > > ::: src/st/st-scroll-view-fade.c > @@ +131,3 @@ > + g_warning ("Unable to use the ShaderEffect: the graphics hardware " > + "or the current GL driver does not implement support " > + "for the GLSL shading language."); > > Oh, forgot to write up something in my first review. This warning is going to > fire every time the actor paints. No as it disables itself after printing the warning, so it should never show up more than once (unless someone is toggling the vfade property like crazy). > @@ +174,3 @@ > + gchar *log_buf = cogl_shader_get_info_log (self->shader); > + > + g_warning (G_STRLOC ": Unable to compile the fade shader: %s", > > As will this - I think GLSL implementations are allowed to refuse to compile > shaders for arbitrary reasons, so you can't crazily spew in this case - you > need to set things up to only try and compile once. Maybe even use a static > variable so you don't try and compile shaders for each scroll view we create. OK, only compiling it once does indeed make sense.
(In reply to comment #10) > Review of attachment 178782 [details] [review]: > > ::: src/Makefile-st.am > @@ +169,3 @@ > libst_1_0_la_SOURCES = \ > $(st_source_c) \ > + $(st_non_gir_sources) \ > > Line up the \ OK > ::: src/st/st-scroll-view-fade.c > @@ +102,3 @@ > + gdouble value, lower, upper, page_size; > + gboolean enabled, needs_fade; > + enabled = clutter_actor_meta_get_enabled (CLUTTER_ACTOR_META (effect)); > > This isn't worthwhile, clutter_meta_set_enabled() internally short-circuits the > no-change case. OK, should have looked at it first. > @@ +144,3 @@ > + g_signal_connect (self->vadjustment, "changed", > + G_CALLBACK (on_vadjustment_changed), > + effect); > > This is too late. We've already created the offscreen fbo, etc. You need to set > things up so that if no scrolling is present the ClutterMeta is disabled > *before* we try to paint once. You also need to call the on_vadjustmnet_changed > once after making the connection. OK
(In reply to comment #12) > (In reply to comment #11) > > Review of attachment 178782 [details] [review] [details]: > > > > ::: src/st/st-scroll-view-fade.c > > @@ +131,3 @@ > > + g_warning ("Unable to use the ShaderEffect: the graphics hardware " > > + "or the current GL driver does not implement support " > > + "for the GLSL shading language."); > > > > Oh, forgot to write up something in my first review. This warning is going to > > fire every time the actor paints. > > No as it disables itself after printing the warning, so it should never show up > more than once (unless someone is toggling the vfade property like crazy). Oh if I move the on_vadjustment_changed to run earlier that would be indeed the case.
Created attachment 178870 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack. --- *) Fix style issues *) Compile the shader only once and reuse it across instances *) Avoid warning flood on error *) Make sure that the adjustment check is done before we set up fbos etc
Review of attachment 178870 [details] [review]: Generally looks good ::: src/st/st-scroll-view-fade.c @@ +109,3 @@ + + self->actor = clutter_actor_meta_get_actor (CLUTTER_ACTOR_META (effect)); + if (self->actor == NULL) Changing self->actor here is just weird. It has to be already set to the right value, no? @@ +233,3 @@ + parent->set_actor (meta, actor); + + if (actor == NULL && self->vadjustment) This shouldn't have actor == NULL here - you'd do the same thing if changing from actor 1 to actor 2 @@ +236,3 @@ + { + g_signal_handlers_disconnect_by_func (self->vadjustment, + on_vadjustment_changed, Because of a bug in the g_signal_handlers_disconnect_by_func prototype, this needs to be: (gpointer)on_vadjustment_changed @@ +237,3 @@ + g_signal_handlers_disconnect_by_func (self->vadjustment, + on_vadjustment_changed, + CLUTTER_EFFECT (self)); The cast to ClutterEffect is unnecessary @@ +249,3 @@ + g_signal_connect (self->vadjustment, "changed", + G_CALLBACK (on_vadjustment_changed), + CLUTTER_EFFECT (self)); cast to ClutterEffect is unnecessary @@ +275,3 @@ + g_signal_handlers_disconnect_by_func (self->vadjustment, + on_vadjustment_changed, + CLUTTER_EFFECT (self)); Same comments as above Also, you need to set self->vadjustment to NULL. [this is actually a mess - ClutterActorMeta clears self->priv->actor in two places: - In finalize - When self->actor is destroyed but doesn't go through clutter_actor_meta_set_meta() in the second place as it should. So clearing a side-property of the actor in 'dispose' is wrong since it doesn't correspond to when the actor for the meta is changed, but there's no good fix, might as well leave it like this. I'd also clear self->actor here though] @@ +327,3 @@ + else + { + g_warning ("Unable to use the ShaderEffect: the graphics hardware " "the ShaderEffect" should be something like "scroll view fade effect", though honestly, I don't see a point in this warning. Our logs should be clean for anything we can't fix. We can't fix whether or not the user has GLSL support, so they should just not get shadows. right? I'd probably leave the "cannot compile warning" because it provides significant information that might be useful in debugging some problem with a particular GLSL implementation - the error message.
(In reply to comment #16) > Review of attachment 178870 [details] [review]: > > Generally looks good > > ::: src/st/st-scroll-view-fade.c > @@ +109,3 @@ > + > + self->actor = clutter_actor_meta_get_actor (CLUTTER_ACTOR_META (effect)); > + if (self->actor == NULL) > > Changing self->actor here is just weird. It has to be already set to the right > value, no? Indeed ... > @@ +233,3 @@ > + parent->set_actor (meta, actor); > + > + if (actor == NULL && self->vadjustment) > > This shouldn't have actor == NULL here - you'd do the same thing if changing > from actor 1 to actor 2 Which currently never happens, but fixed that anyway. > @@ +236,3 @@ > + { > + g_signal_handlers_disconnect_by_func (self->vadjustment, > + on_vadjustment_changed, > > Because of a bug in the g_signal_handlers_disconnect_by_func prototype, this > needs to be: > > (gpointer)on_vadjustment_changed OK > @@ +237,3 @@ > + g_signal_handlers_disconnect_by_func (self->vadjustment, > + on_vadjustment_changed, > + CLUTTER_EFFECT (self)); > > The cast to ClutterEffect is unnecessary OK > @@ +249,3 @@ > + g_signal_connect (self->vadjustment, "changed", > + G_CALLBACK (on_vadjustment_changed), > + CLUTTER_EFFECT (self)); > > cast to ClutterEffect is unnecessary OK > @@ +275,3 @@ > + g_signal_handlers_disconnect_by_func (self->vadjustment, > + on_vadjustment_changed, > + CLUTTER_EFFECT (self)); > > Same comments as above Also, you need to set self->vadjustment to NULL. OK > [this is actually a mess - ClutterActorMeta clears self->priv->actor in two > places: > > - In finalize > - When self->actor is destroyed > > but doesn't go through clutter_actor_meta_set_meta() in the second place as it > should. So clearing a side-property of the actor in 'dispose' is wrong since it > doesn't correspond to when the actor for the meta is changed, but there's no > good fix, might as well leave it like this. I'd also clear self->actor here > though] OK > @@ +327,3 @@ > + else > + { > + g_warning ("Unable to use the ShaderEffect: the graphics hardware " > > "the ShaderEffect" should be something like "scroll view fade effect", though > honestly, I don't see a point in this warning. Our logs should be clean for > anything we can't fix. We can't fix whether or not the user has GLSL support, > so they should just not get shadows. right? I'd probably leave the "cannot > compile warning" because it provides significant information that might be > useful in debugging some problem with a particular GLSL implementation - the > error message. Makes sense.
Created attachment 178877 [details] [review] StScrollView: Implement real fade effect Implement an edge fade effect (top/bottom) using a ClutterOffscreenEffect subclass, replacing the former shadow hack.
Review of attachment 178877 [details] [review]: ::: src/st/st-scroll-view-fade.c @@ +229,3 @@ + } + + if (actor != self->actor && self->vadjustment) This still isn't right. If actor == self->actor you'd end up with two signal connections. Just do: if (self->vadjustment) OK to commit with that change
Attachment 178877 [details] pushed as 00ba937 - StScrollView: Implement real fade effect