I guess because the icon would be quite small and ugly, and only one instance of Totem can be started at a time. I'd like to see what it would look like though...

In Epiphany we dropped that because of the big mess it could take too.
IMHO, it's easier to look for the totem icon than for a $something-im-seeing icon.

Diego, if you fancy cooking up a quick patch that shows that it's impractical, I'd appreciate.

Created attachment 95823 [details] [review]
Dirty hack just to try it

I copy pasted some stuff from totem-video-thumbnailer to try this, at the end I couldn't make it thumbnail the video on open nor avoid the jumping caused by the interesting_frame seeking.
BUT, I think that for a proof of concept of how this looks it's good enough.
To make it actually do something, load a video and hit P or pause and play via the play/pause button.

shame on me.

Created attachment 95824 [details]
screenshot

This is how it looks. I wouldn't kill for it.

Doesn't look too useful (although I'd have implemented it using libgnomeui's thumbnailing functions, so we only used ready made thumbnails).

Sebastien, does your reporter like the result? I don't think it's useful, but maybe he's got some more hindsight into its usefulness...

new submitter comment

"They do have a point indeed, mainly because of what Bastien says: "only one instance of Totem can be started at a time".

I think that -together with your quote- is a good reason to keep the Totem icon even if EOG and Evince use the document thumbnail."

A case where this would be useful is task lists or docks with larger icons (think awn) and Alt+Tab with compiz (large icons too). As an example, rhythmbox cover art, gimp, eog and pidgin buddy icons are very useful and look great.

Victor has a point, reopening.

This could be done easily in a plugin. Just listening to the file_open and file_closed signals, and setting the window icon there. Anyone wants to take a crack at it?

How would you listen to file_open and file_closed? Sorry, I'm pretty new to GTK+.

OK, so I've got something that's essentially working, two remaining questions:

1. What's the optimal way to get the totem icon (for when there is nothing loaded)?
2. gio or gnome-vfs? Right now I've written it with gio.

Created attachment 102786 [details] [review]
patch to add this

Created attachment 102787 [details]
thumbnail plugin

Minor quibble with the first patch:

+plugin_error_or_ignore "you need gnome installed for the window thumbnail plugin"

s/gnome/GNOME/

With the second patch:

libthumbnail_la_LIBADD = $(top_builddir)/lib/libtotemscrsaver.la

Are you sure you meant this (in Makefile.am)?

 - You may want to change the copyright line in totem-thumbnail.c.
 - There's a load of debug g_prints in totem-thumbnail.c which need removing.
 - You'll want to disconnect from the file-opened and file-closed signals when the plugin's deactivated.
 - The only other thing I can see is that you're not putting braces on the same lines as the if statements, which is something we do in Totem code. For example:

if (...)
{
 foo
}
else
{
 ...
}

becomes...

if (...) {
 foo
} else {
 ...
}

This is good for a first attempt, so thank you. :-)

Couple of comments:
- Don't use gnome_thumbnail_factory_generate_thumbnail to get the thumbnail, it will create one if it doesn't already exist, and we don't want that (you'll need to look for the thumbnail by hand)
- Don't post tarballs when a patch could do (especially a tarball with useless binary bits in it).

Rest looks alright.

Created attachment 102829 [details] [review]
fixed patch

Comment on attachment 102829 [details] [review]
fixed patch

>+		thumbnail)
>+			if test "${HAVE_GNOME}" != "yes" ; then
>+				plugin_error_or_ignore "you need GNOME installed for the window thumbnail plugin"
>+				add_plugin="0"
>+			fi

I think you can build it unconditionally.

<snip>
>+static GnomeThumbnailFactory *
>+get_thumbnail_factory (void)

This isn't used, so please remove it.

<snip>
>+	file_name = g_strdup_printf ("%s/.thumbnails/normal/%s.png",
>+				     g_get_home_dir (),
>+				     uri_md5);

Please use g_build_filename here, not g_strdup_printf.

<snip>
>+TotemPlaylist *
>+totem_get_playlist (Totem *totem)
<snip>

We should be exporting the MRL as a property, not using the playlist object.

>+GConfClient *
>+totem_get_gconf_client (Totem *totem)

This is unused, please revert those changes.

(In reply to comment #16)
> Created an attachment (id=102829) [edit]
> fixed patch
> 

I get this:

totem-thumbnail.c: In function 'update_from_state':
totem-thumbnail.c:133: warning: implicit declaration of function 'g_compute_checksum_for_string'
totem-thumbnail.c:133: warning: nested extern declaration of 'g_compute_checksum_for_string'
totem-thumbnail.c:133: error: 'G_CHECKSUM_MD5' undeclared (first use in this function)
totem-thumbnail.c:133: error: (Each undeclared identifier is reported only once
totem-thumbnail.c:133: error: for each function it appears in.)
totem-thumbnail.c:135: warning: assignment makes pointer from integer without a cast
totem-thumbnail.c:122: warning: unused variable 'expanded_file_name'
make[5]: *** [libthumbnail_la-totem-thumbnail.lo] Error 1
make[5]: Leaving directory `/home/victor/src/totem/src/plugins/thumbnail'
make[4]: *** [all-recursive] Error 1
make[4]: Leaving directory `/home/victor/src/totem/src/plugins'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/home/victor/src/totem/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/victor/src/totem/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/victor/src/totem'
make: *** [all] Error 2

(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=102829) [edit]
> > fixed patch
> > 
> 
> I get this:
> 
> totem-thumbnail.c: In function 'update_from_state':
> totem-thumbnail.c:133: warning: implicit declaration of function
> 'g_compute_checksum_for_string'


You need glib 2.15.x or better. The deps should be updated.

(In reply to comment #17)
> >+GConfClient *
> >+totem_get_gconf_client (Totem *totem)
> 
> This is unused, please revert those changes.

I believe it's used in Patrick's patch for bug #481080, but is included in that patch also, so could be removed here without harm.

Created attachment 102868 [details] [review]
actually fixed patch... i think?

Comment on attachment 102868 [details] [review]
actually fixed patch... i think?

>Index: configure.in
>===================================================================
>--- configure.in	(revision 5018)
>+++ configure.in	(working copy)

You need to update the glib requirements, or make the plugin dependant on the presence of glib 2.15 or better.

<snip>
>+	if (pixbuf == NULL) {
>+		if (err != NULL) {
>+			g_printerr ("%s\n", err->message);
>+		}
>+		set_icon_to_default (totem);
>+		return;
>+	}

Please ignore load errors, and try to load from the large thumbnails first.

<snip>
>+	g_object_get (totem, "current-mrl", &mrl);

That needs to be null-terminated, otherwise it's likely to crash.

<snip>
>+_Description=Set the window thumbnail to the thumbnail of whatever is playing

Better to say:
Set the window icon to the thumbnail of the playing movie


Rest looks good

Created attachment 103178 [details] [review]
i hope this is good :D (patch)

2008-01-21  Bastien Nocera  <hadess@hadess.net>

        Patch from Patrick Hulin <patrick.hulin@gmail.com>

        * src/totem-object.c: (totem_object_class_init),
        (totem_object_set_property), (totem_object_get_property):
        Add current-mrl property which gets/sets the current MRL
        that Totem is playing

        * configure.in:
        * src/plugins/thumbnail/*: Add the thumbnail plugin to show
        the currently playing file's thumbnail as the window icon
        (Closes: #407513)

Created attachment 103628 [details]
A screenshot of how this looks with AWN

Great job, thanks !

(In reply to comment #15)
> Couple of comments:
> - Don't use gnome_thumbnail_factory_generate_thumbnail to get the thumbnail, it
> will create one if it doesn't already exist, and we don't want that (you'll
> need to look for the thumbnail by hand)

Bastien, why not generate the thumb ? It is inconsistent. Imagine a user that sometimes sees a thumbnail and sometimes doesn't; and no explanation why.

I think each app that uses thumbnails should generate one for the file if it doesn't exist yet.

Thoughts ?

(In reply to comment #26)
> (In reply to comment #15)
> > Couple of comments:
> > - Don't use gnome_thumbnail_factory_generate_thumbnail to get the thumbnail, it
> > will create one if it doesn't already exist, and we don't want that (you'll
> > need to look for the thumbnail by hand)
> 
> Bastien, why not generate the thumb ? It is inconsistent. Imagine a user that
> sometimes sees a thumbnail and sometimes doesn't; and no explanation why.
> 
> I think each app that uses thumbnails should generate one for the file if it
> doesn't exist yet.

Because:
1) We would have to reimplement Nautilus' checks on whether or not to thumbnail a file (and we'd end up with the same problem, no icon without any real explanations)
2) Because it would create more strain on the filesystem on which the file is, especially if the filesystem is remote, creating a bad playback experience.

In most cases, Totem would be started from Nautilus, or files might be added using Nautilus through drag'n'drop, which means that the thumbnail would be generated there.

I'd happily accept a patch to monitor the thumbnails directory and automatically update the window icon if the file is read by Totem before the Nautilus had a chance to thumbnail it, although that would need to be a separate bug.