Created attachment 162136 [details] [review]
Add support for using the GNOME proxy settings

Read GNOME's proxy settings from GConf and use them to make HTTP
requests. Unfortunately there doesn't seem to be a way to parse PAC
(autoconfig) files, so these aren't supported.

Review of attachment 162136 [details] [review]:

Thanks, Iain!  This looks really good.  Just a few nitpicks:

::: src/Backends/Banshee.Gnome/Banshee.GnomeBackend/GConfProxy.cs
@@ +52,3 @@
+        public GConfProxy ()
+        {
+

remove this blank line

@@ +67,3 @@
+        }
+
+        private void OnProxyChanged(object sender, GConf.NotifyEventArgs args)

Please put a space between method names and the ( in method invocations.

::: src/Backends/Banshee.Gnome/Banshee.GnomeBackend/GnomeService.cs
@@ +43,3 @@
         }
 
+        internal GConfProxy GConf {

Is this property used anywhere?  If not, remove it, if so, should probably rename it to Proxy or something similar

I'd thought Mono automatically respected the user's proxy settings - I guess that's not right?

Mono respects proxy settings that are properly exported into the user's environment. I'm not sure why GNOME is not doing this...

In any regard, this patch looks OK to me, and I think we need the same in MeeGo. I'm ok with this going in pending the minor syntax issues Gabriel pointed out (no space before method calls is the main problem I see).

Also, my nitpicks:

+using Banshee.Configuration;
+using Hyena;
+using System;
+using System.Net;

We tend to group imports by what's provided by Mono/.NET first, then more general namespaces/extenal (Hyena), and then Banshee-specific namespaces, with newlines in-between groups. So I'd prefer:

+using System;
+using System.Net;
+
+using Hyena;
+
+using Banshee.Configuration;

Also, please drop the (c) from your copyright declaration. It doesn't do anything :)

Created attachment 162240 [details] [review]
Add support for using the GNOME proxy settings

Read GNOME's proxy settings from GConf and use them to make HTTP
requests. Unfortunately there doesn't seem to be a way to parse PAC
(autoconfig) files, so these aren't supported.

There's a new one, thanks for your reviews! I hope it's alright now.

Aaron, it seems like almost all of the other files contain (C) too, but I dropped it at your request. :)

And yes, setting the environment up correctly is something GNOME should be doing. I spoke to mvo a bit about this, and there are concerns that it is less dynamic or something. But it doesn't seem sensible for every app to have to have code like this. There was an upstream bug somewhere that I could try to dig up.

Oh, true, there are a lot of (C), but that's something I've just been trying to change. I appreciate your attention to the details there. /me obsessed with style.

As for the data not being exported from gconf to the proper env variables, I'm sure a lot of back and forth can happen on that subject. But there's no way for Mono, as a platform agnostic piece to know about what isn't exported in the expected ways. Hence, your patch.

What should happen though, is that to the best of the platform's ability (GNOME), its proxy settings should be exported to the right environment variables. This is a "better than nothing" approach. I appreciate the intricacies that exist in it not being perfect though.

Luckily, Banshee is designed to integrate with various platforms. And therefore, your patch is very appreciated, and easily drops in.

I will merge this. It looks like a solid usable start. Many thanks.

Merged to master.

http://git.gnome.org/browse/banshee/commit/?id=ebb43a8a21bd8bcf37b4b9b5881cbfbf49e57887

Thanks!