Created attachment 237477 [details] [review]
gd-stack: Remove last_visible_pattern_width/height

Since stacks are homogenous, we should never have to track this
in order to do a cross-fade.

Created attachment 237478 [details] [review]
gd-stack: Remove some initializations

As the properties that govern these are CONSTRUCT properties,
gobject will set the defaults for us.

Created attachment 237479 [details] [review]
gd-stack: Disable transitions when we have gtk-enable-animations disabled

Created attachment 237480 [details] [review]
gd-stack: Split crossfade-y parts into other functions

Created attachment 237481 [details] [review]
gd-stack: Move transition implementations into vfuncs

This will make it easier to add more transitions in the future without
making the system too flexible.

Created attachment 237482 [details] [review]
gd-stack: Don't make calls to transition vfuncs when not transitioning

This prevents extra waste, and will prevent against extra checks when
making more attempts to split the transition infrastructure out.

Created attachment 237483 [details] [review]
gd-stack: Put transition-specific data in its own structure

As we want to add more transitions, this keeps it out of the
main structure.

Created attachment 237484 [details] [review]
gd-stack: Split out size_allocate as well

Sliding transitions will require a different size_allocate, so
let's clean up this for now.

Created attachment 237485 [details] [review]
gd-stack: Add a tick handler

The cross-fade will queue a redraw from here, and the slide
will queue a relayout.

Created attachment 237486 [details] [review]
gd-stack: Add some slide transitions

This allows people to slide pages left or right instead of
cross-fading, which would be useful in an assistant scenario.

Created attachment 237487 [details] [review]
gd-stack: Put transition-specific data in its own structure

As we want to add more transitions, this keeps it out of the
main structure.



Fix a crash that can happen if you add a bunch of widgets to the
stack while using the CROSSFADE transition style.

Review of attachment 237477 [details] [review]:

Are animations guaranteed to be turned off when homogeneous is FALSE? If so, this looks good.

Review of attachment 237478 [details] [review]:

OK

Review of attachment 237479 [details] [review]:

Nice

Review of attachment 237480 [details] [review]:

Nice cleanup - can you explain this change though?

::: libgd/gd-stack.c
@@ +445,3 @@
   done = pos >= 1.0;
 
+  if (done)

I wonder if removing the (priv->last_visible_pattern != NULL) here changes any semantic for the following set_child_visible call.

Review of attachment 237481 [details] [review]:

Looks good

Review of attachment 237482 [details] [review]:

Shouldn't the existing transition_pos < 1 have the same effect?

Review of attachment 237487 [details] [review]:

::: libgd/gd-stack.c
@@ +1103,3 @@
+  cairo_pattern_t *last_visible_pattern;
+  int last_visible_pattern_width;
+  int last_visible_pattern_height;

These are back?

Review of attachment 237484 [details] [review]:

OK

Review of attachment 237485 [details] [review]:

OK

Review of attachment 237486 [details] [review]:

Cool!

Created attachment 237495 [details] [review]
gd-stack: Put transition-specific data in its own structure

As we want to add more transitions, this keeps it out of the
main structure.



Hm, the "don't make calls" was originally much larger, but it seems that
was trimmed down over time.

I removed the extra two fields that came back (yay bad rebases) -- now
we're allocating a pointer-sized structure -- if you want me to take the
intermediate structure out, I can do that.

Review of attachment 237477 [details] [review]:

I added this because we now do crossfade on non-homogenous stacks too

Review of attachment 237480 [details] [review]:

::: libgd/gd-stack.c
@@ +445,3 @@
   done = pos >= 1.0;
 
+  if (done)

Yeah, this seems wrong. We want to hide the last visible child as soon as we have a snapshot of it. This will keep the child visible and any windows of it mapped, which will let the user modify state and interact with those widgets.

@@ +455,1 @@
+      gd_stack_transition_done_crossfade (stack);

I don't think last_visible_pattern is really crossfade specific. We want to use it for all animations, so that we can show the last state in the old widget for the animation, rather than keep showing any changes in state in the old widget during the animation.

@@ +525,3 @@
+      switch (priv->transition_type) {
+      case GD_STACK_TRANSITION_TYPE_CROSSFADE:
+        gtk_widget_set_opacity (widget, 0.999);

We need this for other transition types too, if we want to take a snapshot.

Review of attachment 237486 [details] [review]:

I don't think this is right at all.
There are several issues.
First of all you're continually redrawing the "old" widget, rather than working with a snapshot of it. I don't think its right to show any kind of state changes in the old widget as we move to the new one. Its commonly gonna be the case that you e.g. reset the state of the old widget in some ways (loses focus, clear data from entries, etc) which should not be seen during the transition. Furthermore, you also allow events to the old widget meaning the user will be able to interact with it, which is pretty bad.

Secondly, you can't just size allocate the children so that they are outside the stack allocation. That will be completely broken for any windows in the child widgets, as GdStack as-is is a non-window widget, and will not clip such child windows.

I basically disagree with most of this series. I'll try to come up with an alternative version.

Attachment 237478 [details] pushed as 83d366e - gd-stack: Remove some initializations
Attachment 237479 [details] pushed as 8a3d891 - gd-stack: Disable transitions when we have gtk-enable-animations disabled

Created attachment 237514 [details] [review]
GdStack: Add windows

This adds a base window which is needed for clipping child windows when we
start sliding them. It also adds a bin window that makes it more efficient
to slide child widgets/windows.

Created attachment 237515 [details] [review]
GdStack: Add sliding transitions

Note: This requires the latest gtk+ version as there was
a bug in the opacity group support.

Created attachment 237516 [details] [review]
test-stack: Add tests for all transition types

Here is my approach. It correctly clips child windows, it doesn't relayout during animation, it scrolls via xcopyarea and it doesn't update the old widget due to state changes after the initial expose (nor does it allow interaction with it).

Review of attachment 237514 [details] [review]:

Looks good otherwise - I think Alex's approoach is indeed much cleaner.

::: libgd/gd-stack.c
@@ +273,3 @@
+    }
+
+  attributes.event_mask =

Copy/paste leftover?

Review of attachment 237515 [details] [review]:

OK

Review of attachment 237516 [details] [review]:

Sure

Note that your stack doesn't apply an ease to the slide. You should probably add that.

Attachment 237514 [details] pushed as deb4a9a - GdStack: Add windows
Attachment 237515 [details] pushed as d1f34e1 - GdStack: Add sliding transitions
Attachment 237516 [details] pushed as 3432e75 - test-stack: Add tests for all transition types

Pushed this with fixes to the comments and ease-out.

Jasper had another comment about how my patch handles the "interrupted slide" case. My code doesn't do this very nicely. But, what is the right behaviour here? Should we just keep the partially slid in "old new" widget in there and start sliding in the new one directly? Do we queue them up?

(In reply to comment #37)
> Pushed this with fixes to the comments and ease-out.
> 
> Jasper had another comment about how my patch handles the "interrupted slide"
> case. My code doesn't do this very nicely. But, what is the right behaviour
> here? Should we just keep the partially slid in "old new" widget in there and
> start sliding in the new one directly? Do we queue them up?

I think it would be nice to imagine the stack as one giant thing, and that the slide is just a pan over it, so if you can, doing the tricky thing of showing both widgets you're skipping over in the animation would be nice.

I pushed some stuff preparing for a nicer panning. Also fixes some gtk issues that made things not work with complex stack children (added to testcase).

The following commit breaks the panels in gnome-control-center. Try Backgrounds or Users for an example.

commit deb4a9af27524fd11c1e67d28dbc47c1a4536186
Author: Alexander Larsson <alexl@redhat.com>
Date:   Wed Feb 27 10:35:06 2013 +0100

    GdStack: Add windows
    
    This adds a base window which is needed for clipping child windows when we
    start sliding them. It also adds a bin window that makes it more efficient
    to slide child widgets/windows.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=694769

That has been fixed now in libgd master.
Since the topic of this bug is fully implemented, I'm going to close this as FIXED.