Created attachment 131322 [details]
Screenshot of YouTube source after searching for "firefox"

Currently clicking buttons just open the video's URL in the default web browser.

Created attachment 131484 [details]
YouTube extension source

Hey Kevin, this looks promising.

Just wanted to mention that here's a similar idea that I didn't have time for:
http://idea.opensuse.org/content/ideas/party-extension-for-banshee-youtubeflash-streaming-mixed-with-current-music-in-the-user-s-library

Created attachment 135221 [details]
YouTube page in the content pane

Created attachment 135222 [details] [review]
Patch to add youtube page to the content pane

Updating bug summary to reflect extension changes

Created attachment 135244 [details] [review]
Apply second patch to show message when no videos found

Thanks Kevin, this looking pretty good.

1) Please use the same copyright header you find elsewhere, eg make sure you have the // Copyright (C) 2009 ... line.  You of course keep your copyright.

2) public class VerticalTile: Button  => put a space before the :

3) You've got some slightly off indentation, eg:
+            Relief = ReliefStyle.None;
+        }
+       
+       public string PrimaryText {

4) The "+    copyright="© Kevin Duffus." in the addin.xml needs to have a year

5) In your ContextPage.cs and YouTubePane.cs files you have some tabs for indentation

6) Please combine both patches

Created attachment 135543 [details] [review]
This patch should cover all the requested changes

A few minor nitpicks:

* We use Banshee.IO.File instead of System.IO.File:
    if (File.Exists (cache_file)) {
    using (FileStream file_stream = File.Open (cache_file, FileMode.Create)) {
    etc...

* There's a small visual glitch on dark themes, I will attach a screen-shot.

Other than that, the patch looks (and works) great, well done!

Created attachment 135863 [details]
Visual glitch on the Darklooks theme

Created attachment 135903 [details] [review]
Fix glitch in dark themes and use Banshee.IO instead of System.IO

Hmm. I'm not sure but would the following be more appropriate for saving a file?

+            using (HttpWebResponse response = (HttpWebResponse) request.GetResponse ()) {
+                using (Stream stream = GetResponseStream (response)) {
+                    using (FileStream file_stream = Banshee.IO.File.OpenWrite (uri, true)) {
+                            Banshee.IO.StreamAssist.Save (stream, file_stream);
+                    }
+                }
+            }

(In reply to comment #13)
> Hmm. I'm not sure but would the following be more appropriate for saving a
> file?

Yes, it does exactly the same.

Created attachment 135924 [details] [review]
This one should apply cleanly

Getting

Making all in Banshee.YouTube
  MCS   ../../../bin/Banshee.YouTube.dll
error CS2007: Unrecognized command-line option: `-r/usr/lib/mono/GData-Sharp/Google.GData.Extensions.dll'

any ideas?

Thats odd. It seems that a make file wasn't generated correctly. There should be a space following the '-r' switch. Perhaps running autogen.sh again could fix it?

ah, the missing colon, right; must be a bug in my package or version of gdata-sharp

Yeah, was just fixed upstream on March 25:

http://code.google.com/p/google-gdata/source/diff?spec=svn890&r=890&format=side&path=/trunk/clients/cs/misc/gdata-sharp-core.pc.in

Does anyone believe this is worthy of being pushed upstream or rather packaged separately? I currently don't have a storage facility online for it at the moment.

Review of attachment 135924 [details] [review]:

Looks really good, Kevin.  Just a few niticks.

What's the status of being able to play videos within Banshee - does YouTube expose the actual video URL yet?  I heard rumors about them supporting the HTML5 <video> tag, thought maybe the story is better than when I last checked.

I'm fine with this going into master and the default plugins set.

::: configure.ac
@@ +94,3 @@
 BANSHEE_CHECK_WEBKIT
 
+dnl webkit (optional through --enable-gdata)

s/webkit/gdata/

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Data/YouTubeData.cs
@@ +54,3 @@
+    {
+        public Feed<Video> videoResults;
+        public List<string> allCategories = new List<string> ();

Why are these two public?  If they really need to be, make them properties.

Also, please name all non-public members like_this, and public members LikeThis

::: src/Extensions/Banshee.YouTube/Banshee.YouTube/YouTubePane.cs
@@ +97,3 @@
+                
+                if (!String.IsNullOrEmpty (query))
+                {

put this on the same line as the if

@@ +202,3 @@
+
+                    foreach (Video entry in videoFeed.Entries)
+                    {

put { on foreach line

@@ +203,3 @@
+                    foreach (Video entry in videoFeed.Entries)
+                    {
+                        if (result_display_count++ >= max_results_display) {

change to if (entry.IsDraft) { continue; } else if (result_display_count++...)

@@ +207,3 @@
+                        }
+
+                        Log.DebugFormat ("YouTube: Adding tile for video \"{0}\"", entry.Title);

This is probably a little verbose logging

@@ +213,3 @@
+                            YouTubeTile tile = new YouTubeTile (entry);
+                            results_tv.AddWidget (tile);
+                            results_tv.ShowAll ();

put ShowAll outside/beneath the foreach

I've made the suggested changes and did a little bit of cleaning up. I wasn't able to dig up any information regarding HTML5 video exposed via the GData API. However, there are a lot of videos that have RSTP URLs (*.3gp files for mobile) that could be retrieved. But I'm not sure exactly how to pass the Uri directly to the player engine to have it queued or played.

Created attachment 148190 [details] [review]
Patch cleaned up with respect to Gabriel's input

Taking a queue from the YouTube plugin from Totem, I was able to find a means of playing the videos directly within Banshee =). Known issues:

1. I've noticed that an initial click of the video button may result in a timeout waiting for a response from the server. Here I'm simply passing the URI to Banshee.Streaming.RadioTrackInfo.OpenPlay () so I'll need to figure out how to increase the default timeout value.

2. After the video finishes playing. Banshee returns to the playing from the top of the previous playback source. Not sure what would be the preferable starting point here (next song after the that which was playing before the video was clicked?)

Created attachment 148191 [details]
Screenshot of YouTube video playing back in Banshee

Review of attachment 148190 [details] [review]:

I think there are several file that should not be in the patch :
- Banshee.YouTube.sln
- Banshee.YouTube.usertasks
- bk.csproj

My apologies. I realised this soon after uploading.  I'll make sure to review more carefully before attaching next time.

Also, the UI seems to be blocked while the YouTube extension is loading items.

Created attachment 148210 [details] [review]
Updated patch

Latest patch fixes the following issues:
1. Stream fail dialogue no longer appears due to timeout.
2. Tiles are loaded/removed in a separate thread so UI is no longer blocked

Also, track info display now shows the YouTube video's title and uploader's username instead of "Unknown..."

Kevin I love the afford you put into this. Thank you very much, looking forward for this feature.
Another Improvement:
Could the Video be an element in the playlist like a normal music or videofile so I can just create a playlist also with music not on my computer? Or just save Element on my harddisk. With support for HD on youtube audio quality is quite good.
Create an easy interface to add support for other media sites in the same plugin.

Review of attachment 148210 [details] [review]:

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Data/DataFetch.cs
@@ +78,3 @@
+                
+                while (count > 0) {
+                    contents = String.Concat (contents, new String(read_buffer, 0, count));

Any reason not to use stream_reader.ReadToEnd () instead of doing all this buffer stuff?

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/YouTubeTile.cs
@@ +72,3 @@
+            if (!match.Success) { return null; }
+            
+            t_param = Regex.Unescape(match.Result ("$1"));

=> Unescape (
that is, add a space

@@ +94,3 @@
+
+                foreach (MediaContent m in yt_video.Media.Contents)
+                {

put { on same line as foreach

@@ +112,3 @@
+                }
+            }
+            else

change to } else {

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.addin.xml
@@ +6,3 @@
+    copyright="© Kevin Duffus. Licensed under the MIT X11 license."
+    name="YouTube"
+    category="User Interface"

Change category to "Context Pane"

@@ +7,3 @@
+    name="YouTube"
+    category="User Interface"
+    description="View results of searching YouTube for playing song"

Change description to "Show related YouTube videos in the context pane." maybe?

::: src/Extensions/Banshee.YouTube/Banshee.YouTube/YouTubePane.cs
@@ +160,3 @@
+        }
+
+        private class RefreshVideosJob : Banshee.Kernel.Job

Do not use Banshee.Kernel.Job - instead use Hyena.Jobs.

@@ +200,3 @@
+                    foreach (Video entry in video_feed.Entries) {
+                        // Don't include videos that are not live
+                        if (entry.IsDraft) { continue; }

change all single-line if's like this to be if (..) {\n .. \n}

@@ +202,3 @@
+                        if (entry.IsDraft) { continue; }
+                        else if (result_display_count++ < max_results_display) {
+                            YouTubeTile tile = new YouTubeTile (entry);

Should not do GUI/GTK work off the main thread.  You can use ThreadAssist.ProxyToMain to run code on the main loop.

Sorry, I've been out of town. Gabriel, thanks for the review. I should have the fixes in by this weekend.

Created attachment 151380 [details] [review]
Updated patch from last review

Review of attachment 151380 [details] [review]:

Thanks Kevin, looks pretty good. See my comments.

I still haven't been able to test it, I'm still fighting to get gdata-sharp installed properly on my system.

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/VideoStreamTile.cs
@@ +233,3 @@
+        }
+
+        /*private void OnAddToPlayQueue (object o, EventArgs args)

Why is the "Add to play queue" stuff commented out ?
You might want to add a TODO explaining it, or remove the code.

::: src/Extensions/Banshee.YouTube/Banshee.YouTube.Gui/YouTubeTile.cs
@@ +64,3 @@
+ 
+            if (String.IsNullOrEmpty (watch_page_contents)) {
+				return null;

Indent with 4 spaces, not tabs. Also happens several times further down in the file and maybe in other files.

Created attachment 152904 [details] [review]
Latest patch

Commented out "Add to Play Queue" code removed as I couldn't figure out how to do this cleanly. Also, tabs switched to 4 spaces.

Created attachment 155218 [details] [review]
Updated to git master

Looks good and works well. I adapted it slightly to apply to git master.

The only issue I'm seeing is in the Makefile, somehow one of the gdata libs has an incorrect parameter (should be -r:/usr/...):

GDATASHARP_LIBS = ... -r/usr/local/lib/mono/GData-Sharp/Google.GData.Extensions.dll ...

Can't figure out what's causing it, could be a problem with my tarball install.

Gabriel, any chance this can be committed before the feature freeze?

(In reply to comment #35)
> Created an attachment (id=155218) [details] [review]
> Updated to git master
> 
> Looks good and works well. I adapted it slightly to apply to git master.
> 
> The only issue I'm seeing is in the Makefile, somehow one of the gdata libs has
> an incorrect parameter (should be -r:/usr/...):
> 
> GDATASHARP_LIBS = ...
> -r/usr/local/lib/mono/GData-Sharp/Google.GData.Extensions.dll ...
> 
> Can't figure out what's causing it, could be a problem with my tarball install.

It was a bug in the gdata-sharp .pc file :
http://code.google.com/p/google-gdata/issues/detail?id=336

(In reply to comment #36)
> It was a bug in the gdata-sharp .pc file :
> http://code.google.com/p/google-gdata/issues/detail?id=336

Thanks Bertrand!

Comment on attachment 155218 [details] [review]
Updated to git master

Yeah, let's get this in now.

Hrm, let's put this in a youtube branch.  I think there are still some issues with it w/ threading.

For example:

+            ThreadAssist.Spawn (delegate {
+                if (video_feed.TotalResults > 0) {
+                    if (!showing_results) {
+                        Remove (no_results_label);
+                        Add (results_sw);
+                        ShowAll ();
+                    }

This appears to be calling Remove, Add, and ShowAll off the GUI thread.

I got to a state where Banshee's UI was frozen, which can happen when you do GUI updates off the GUI thread.

Review of attachment 155218 [details] [review]:

I committed this patch to a new branch called `youtube`

Glad to see movement on this patch. Alex, thanks for updating to master. Gabriel, my apologies for the threading issues as its the first time I'm working with this language.

Kevin, no problem at all - it's a tricky thing to get right for all of us.

(In reply to comment #41)
> I got to a state where Banshee's UI was frozen, which can happen when you do
> GUI updates off the GUI thread.

Should be fixed now.

Thanks Alexander.  What is being cached?  Is anything cached across restarts of Banshee?  It's loading fairly slowly for me (granted not on a great internet connection atm) - Wireshark shows it issues two GETs per each 12 videos (one for metadata, one for thumbnail), so 25 GETs total, and no keepalive action I don't think (maybe within the GData api, but not for thumbnails).

(In reply to comment #46)
> Thanks Alexander.  What is being cached?  Is anything cached across restarts of
> Banshee?

The thumbnails are cached for two hours, may be the period should be increased.

> It's loading fairly slowly for me (granted not on a great internet
> connection atm) - Wireshark shows it issues two GETs per each 12 videos (one
> for metadata, one for thumbnail), so 25 GETs total, and no keepalive action I
> don't think (maybe within the GData api, but not for thumbnails).

Same here, it takes up to 10 seconds to show the list of videos.

The youtube branch has been merged into master a few days ago and the extension is available in 1.5.5