[Sugar-devel] Proxy Settings in Network Control Panel

[Ajay Garg]

Thanks a ton Manuel; I will test the patch on the weekend, and let you know
:)

On Thu, Mar 14, 2013 at 7:27 AM, Manuel Quiñones <manuq at laptop.org> wrote:

> Hi Ajay and other devs,
>
> Ajay, whlie reviewing your patch for the proxy feature, I decided to
> step in and work on the issues I've found by myself.  I did this to
> speed up the process, instead of going through a long
> review/patch/review loop.  I hope you understand.  A (rather long,
> sorry) description of what I did follows.  But first of all, I would
> be glad if you can test my last patch:
>
>
> https://bugs.sugarlabs.org/attachment/ticket/3070/0001-Add-proxy-configuration-support-to-Network-Control-P.patch
>
> The first thing I've found is that your patch does both GConf and
> GSettings.  I decided to go just for GSettings because Browse and all
> underlayng GTK use it.


Great !!
If we are sure that no application carries the legacy "Gconf"
proxy-settings/schema, we are done, (not to forget less lines of code to
carry :P)




>  In any case I think you should have added
> another mixin class for GSettings instead of adding code to GConfMixin
> class.  Sasha's original patch had that quite separated to make adding
> another backend an easy task.
>
> So my first attempt was doing only GSettings and fixing the issues
> I've found in yours:
>
> https://bugs.sugarlabs.org/attachment/ticket/3070/proxy_gsettings.patch
> (note this is not my final patch, let's call this my a) patch)
>
> An issue with your patch: the UI does not always update when a setting
> is changed.  TestCase: change a setting, quickly close the network
> section and open it again: you will see the original value.  This is
> because the timeout that calls _commit has not finished yet.  Sasha's
> patch had gconf client.notify_add() for this.  In my a) patch I have
> added a callback to the 'changed' event of gsettings that does the
> same.
>

I had fixed this, in the patch I had floated :)
The "self._section_view.perform_accept_actions()" did the fix in my patch;
now substituted by "self._section_view.apply()"  in your patch.

I had even mentioned that in point c) in the patch at
http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041779.html :)

Anyways, the end result is that, we HAVE TAKEN care of the racy-behaviour,
as far as the persistence of values is concerned.




> https://developer.gnome.org/gio/stable/GSettings.html#GSettings-changed
>
> There are other differences between Sasha's patch and yours.  For
> example you seem to forgot the try/finally clause in the _commit
> method that blocks the __changed_cb callback.  I have added it back in
> my a) patch.
>
> I intentionally skipped $http_proxy environment variable (Sasha's patch
> 3/3):
>
> http://lists.sugarlabs.org/archive/sugar-devel/2012-August/038804.html
>
> This could be added later if there is a reason.  I don't see GNOME
> doing it.  Which activities or parts of Sugar need it?  Indeed this
> makes wget work with proxy by default but then why GNOME does not do
> it?  Am I missing any conversation?
>

Hmm.. One place where I know for confirmed it is used in "webkitgtk3", thus
making its use in "Browse".
But you said earlier that using ONLY GSettings (without setting the
"http_poxy" variable), propogates the proxy-settings in "Browse". If that
is the case, I think setting "http_proxy" is not needed.

For more details of the "http_proxy" interactions with GTK+, please see the
source-code of webkit, in particular
http://svn.webkit.org/repository/webkit/trunk/Tools/GtkLauncher/main.c




>
> So, now for my final patch.  After having my a) patch working, I took
> another path using the convenient way GSettings provides to bind
> widgets and settings:
>
> https://developer.gnome.org/gio/stable/GSettings.html#g-settings-bind
>
> The adventages can be seen in the ~100 lines removed.  There is...
>
> - no need for a mixin class,
>
> - no need for connecting callbacks to signals between widgets and
> settings in both directions
>
> - no need to implement get_value_for_gsettings /
> set_value_from_gsettings for each widget.  Sasha's gconf
> implementation needed a big hierarchy of classes to archive this.
>
> - no need to test the key type to map the corresponding getter ('s' to
> use get_string(), for example, compare with my a) patch)
>


These look good !!!




>
> So, I welcome testing in different proxy scenarios for my patch.
> Thanks a lot Ajay for your contributed work, which I took as basement.
>
> --
> .. manuq ..
>



-- 
Regards,

Ajay Garg
Dextrose Developer
Activity Central: http://activitycentral.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20130314/ea75efa4/attachment.html>