Created attachment 118982 [details] [review]
add GnomeBGCrossfade object and gnome_bg_set_pixmap_as_root_with_crossfade function

This patch adds an analog to gnome_bg_set_pixmap_as_root called gnome_bg_set_pixmap_as_root_with_crossfade which performs the crossfade.

It returns a crossfade object which can be used to cancel the animation or monitor when it's finished.

bug 552857 and bug 552859 contain the gnome-settings-daemon and eel patches that use the new apis to create the cross fade effect.

Created attachment 118985 [details] [review]
s/animatin/animations/

there was a typo in attachment 118982 [details] [review] that caused some warnings in nautilus

Created attachment 119171 [details] [review]
add a flush call after clearing

I don't think this flush call really matters, but in principle it's a good idea.

Created attachment 119174 [details] [review]
s/animatins/animations/

In the name of taking one step forward and one step back, the last patch regressed the "animations" typo fix.

Created attachment 121306 [details] [review]
ad

This is latest gnome-desktop patch that adds new api needed by nautilus and settings-daemon (see their bugs).

Created attachment 121400 [details] [review]
Patch that applies against svn head

Here's a quick review, it mostly looks good, so good job!

First, new policy in gnome-desktop: no new API without API doc :-)


At least in gnome-bg.c, there seems to be a mix of tabs and spaces in the patch. Please use tabs only.


Sometimes, there are things like this:
+	    if (result == Success && type == XA_PIXMAP &&
+		    format == 32 && nitems == 1) {
while it should be
+		if (result == Success && type == XA_PIXMAP &&
+		    format == 32 && nitems == 1) {
Probably a bad example because there were some tab/space mix involved, but you get the point ;-)


In gnome_bg_get_pixmap_from_root(), I'd prefer to see something like this:
====
if (data != NULL)
 return NULL;

[...]
====
There might be other places in the patch where the same comment applies.


Why do we sometimes do "XGrabServer (display);" and sometimes "gdk_x11_display_grab (display);"? I'd prefer to keep only the gdk version if possible. Same for XFlush and gdk_display_flush.


Not sure why "#include <cairo-xlib.h>" is needed in gnome-bg.c?


+struct _GnomeBGCrossfadePrivate
+{
+	GObject		 parent_instance;
This sounds wrong: the parent instance isn't needed in the private structure.


Please initialize a few fields in gnome_bg_crossfade_init, at least the ones that are in the finalize call: fading_pixmap, end_pixmap, timeout_id.


gnome_bg_crossfade_stop(): please set fade->priv->timeout_id to 0


gnome_bg_set_pixmap_as_root_with_crossfade(): don't call gnome_bg_crossfade_start() if gnome_bg_crossfade_set_start_pixmap() or gnome_bg_crossfade_set_end_pixmap() failed. This is paranoid, I know :-)


In on_finished(), how can fade->priv->end_pixmap be NULL? (maybe on_finished() can be call multiple times, but I fail to see how) Same for fade->priv->fading_pixmap. Or is it purely to be on the safe side? (I'd prefer a g_assert() here)


In animations_are_disabled(), how can fade->priv->window be NULL? I'd do a g_assert() here, I guess.


Thanks!

Created attachment 123838 [details] [review]
Crossfade patch with changes from attachment 121400 [details] [review] to address comment 8 

I think this should address all of your comments.  I've also broken the patch up into multiple chunks to make it easier to read.

Thanks for the quick update! I didn't check if you fixed everything in my comments, but I'll trust you :-) Just s/gnome-bg-crossfade.h/gnome-bg-crossfade.c/ in the gnome-bg-crossfade.c header.

So, I'm now looking at the API itself based on the patches for g-s-d and eel.


a) gnome_bg_set_pixmap_as_root_with_crossfade()
===============================================

Should gnome_bg_set_pixmap_as_root_with_crossfade() really return a GnomeBGCrossfade? Is it useful? Can't GnomeBG keep a reference to this object and unref it when it's done?

If we change this, do we actually need gnome_bg_set_pixmap_as_root_with_crossfade()? Maybe it makes sense to just add a "gboolean fade" argument to gnome_bg_set_pixmap_as_root()? (okay, this breaks API, but only two modules depend on this API and we have no API guarantee, so...)


b) gnome_bg_get_pixmap_from_root()
==================================

Instead of making gnome_bg_get_pixmap_from_root() public, maybe we can assume that if pixmap is NULL in the call to gnome_bg_crossfade_set_start_pixmap(), then the current pixmap of the window should be used when the fading starts (or something like this)?


c) trim down GnomeBGCrossfade API
=================================

Hrm. It might make sense to kill gnome_bg_crossfade_set_start_pixmap() and gnome_bg_crossfade_set_end_pixmap(): in the end, the two pixmaps are more or less construct-only properties (in the sense that they don't make sense when gnome_bg_crossfade_start() is called). So putting the start_pixmap and end_pixmap as arguments to gnome_bg_crossfade_new() would make sense to me.

It wouldn't be an issue for start_pixmap (that's trivial). It requires some changes to the eel patch, but a quick look makes me think it's doable without major issues.

In this case, we can also change things so that creating the object will automatically start it. This means we can kill gnome_bg_crossfade_start(), gnome_bg_crossfade_is_started() and gnome_bg_crossfade_stop(). If the user wants to stop the crossfade, just unref the object.

(we'd replace width/height arguments in gnome_bg_crossfade_new() with the window argument from gnome_bg_crossfade_start() and deduce the width/height from the window)

That would result in just one easy to use function to benefit from this feature: gnome_bg_crossfade_new().

(we could even add some auto_unref option to automatically unref the object when the fade is done, but this might be confusing)


d) if we don't do c)
====================

Is the context argument to gnome_bg_crossfade_start() useful?

Is gnome_bg_crossfade_stop() really useful? It sounds to me that it should just happen when the GnomeBGCrossfade is finalized. But on the other hand, if we provide start(), providing stop() can make sense (not sure).


e) gnome_bg_ignore_changes()
============================

I don't see it being used in patches? Am I missing something?

In gnome_bg_ignore_changes(), it probably makes sense to do:
 	if (bg->changed_id > 0) {
 		g_source_remove (bg->changed_id);
 	}
if ignore_changes is TRUE.


f) question about draw_background() and others...
=================================================

If the window is foreign, shouldn't we grab the server before doing anything to it? Maybe GDK takes care of this, though.

====

That's it. I guess the most interesting point is c). It makes sense to me but tell me what you think!

a)
Note the comment in gnome-bg.h above the gnome_bg_set_pixmap_as_root declaration:

"Set a pixmap as root - not a GnomeBG method "

The set_pixmap_as_root function is just a helper api, it's not a method on a gnome-bg object.  

The with_crossfade variant is the same sort of idea.  We could potentially change both functions to be methods on a GnomeBG object, but then that leads to bigger api changes, since there would be no more need for gnome_bg_create_pixmap anymore (we wouldn't need to pass the pixmap to the caller just so it could pass it back into the object).  Honestly, I don't really see much point in churning the existing api.  It doesn't buy us anything.  If we were going to push to make it an official stable api, then it might make sense.

b)
There's no way to get the current back pixmap of a window.  That's why the root pixmap xid is traditionally stored as a property on the root window.

c)

set_end_pixmap can get called mid animation.  This is useful if the user is in gnome-appearance-properties and is looking at a few backgrounds in quick succession.  It doesn't really make sense to call set_start_pixmap mid-animation (since it would cause the animation to abruptly change frames), but it seemed more symmetrical this way.  Also, the operations can conceivably fail, and returning errors at object construction time is iffy.

Also, making _new have side-effects (like starting animations) seems like a bad idea to me.  It doesn't really match what most good of the good gobject APIs i've come across do.  An argument could be made that this shouldn't be a gobject at all though.  We could potentially make it a GSource and have an api analogous to g_timeout_add, (e.g., gnome_bg_crossfade_add).  It would mean that the crossfade state reference would be owned by the mainloop, and would go away with the mainloop.  So you'd basically just have:

CrossfadeSource *crossfade_source_new (window, start_pixmap, end_pixmap);
and
guint crossfade_add (window, start_pixmap, end_pixmap)

which would do the usual:
{
source = crossfade_source_new (window, start_pixmap, end_pixmap);

/* Transfer owner ship of the source to the main loop */
tag = g_source_attach (source, g_main_context_get_default ());
g_source_unref (source);

return tag;
}

and then calls would keep the guint tag around to check if the animation is in progress and to cancel it. The only reasons I'd oppose this sort of change are:

1) there's not easy way to do the set_end_pixmap change midstream
2) It doesn't seem obviously better to me to do it this way than the already written way.  

I guess the big bit I'm missing is what you don't like about start()/stop() etc.

d) The context argument isn't super useful, since both users of the api use the default context anyway.  I mostly just added it for completeness, and don't really mind ditching it.

e) gnome_bg_ignore_changes() isn't needed anymore because of the changes behdad did.  gnome-settings-daemon used to try to wait 8 seconds before drawing the background if /apps/nautilus/preferences/show_desktop was true since nautilus would normally get started immediately afterward and set the background itself.  The logic was flawed though because it would do load_from_preferences at startup which causes a changed signal to get emitted that makes the background get drawn immediately (and then again 8 seconds later).  ignore_changes() suppressed the changed signal.  Behdad made a change that causes load_from_preferences to get called later instead of right away, so it's not a problem anymore and we can drop ignore_changes().

f) I don't think we need to grab the server.  We certainly couldn't grab the server across the entire fade since that would make the machine seemingly freeze (except for the fade) for almost a second.  I don't think we need to grab the display around every frame either.  The animation is just doing XClearWindow() in a loop.

Actually, we may want to error out if the window is foreign.  We currently only use the api the nautilus window and the root window.

Created attachment 124015 [details] [review]
small leak fix

Right after putting attachment 123838 [details] [review] up, I built it into rawhide.  Today a user noticed that patch caused a leak. I dropped an unref call when splitting the patch up into multiple chunks.

(see https://bugzilla.redhat.com/show_bug.cgi?id=468339)

*** Bug 565550 has been marked as a duplicate of this bug. ***

Bah, sorry for giving up on this, I'll come back soon.

Ray, can you make sure that the crash of bug 565550 is fixed in your latest patch? Thanks!

Created attachment 125864 [details] [review]
stop cross fade in finalize before freeing pixmap instead of after

That's so weird, tbzatek noticed this same bug today and I had just built a fix into rawhide before opening my mail and seeing comment 14

Thanks for being so patient with me :-)

Please commit with just a few changes:

(In reply to comment #11)
> a)
> Note the comment in gnome-bg.h above the gnome_bg_set_pixmap_as_root
> declaration:
> 
> "Set a pixmap as root - not a GnomeBG method "
> 
> The set_pixmap_as_root function is just a helper api, it's not a method on a
> gnome-bg object.  
> 
> The with_crossfade variant is the same sort of idea.  We could potentially
> change both functions to be methods on a GnomeBG object, but then that leads to
> bigger api changes, since there would be no more need for
> gnome_bg_create_pixmap anymore (we wouldn't need to pass the pixmap to the
> caller just so it could pass it back into the object).  Honestly, I don't
> really see much point in churning the existing api.  It doesn't buy us
> anything.  If we were going to push to make it an official stable api, then it
> might make sense.

Works for me. Can you just put a comment about this above the functions. Something like "if we have to break this API, or if we want to make it an official stable API, we should do..."

> d) The context argument isn't super useful, since both users of the api use the
> default context anyway.  I mostly just added it for completeness, and don't
> really mind ditching it.

Please remove it then :-)

> f) I don't think we need to grab the server.  We certainly couldn't grab the
> server across the entire fade since that would make the machine seemingly
> freeze (except for the fade) for almost a second.  I don't think we need to
> grab the display around every frame either.  The animation is just doing
> XClearWindow() in a loop.
> 
> Actually, we may want to error out if the window is foreign.  We currently only
> use the api the nautilus window and the root window.

If possible, do the error for the foreign window. Else, please open a bug about this so that we don't forget it :-)

okay commited.  This won't be useful until the g-s-d and eel changes land too (redoing them now to work with the api changes and for other reasons)