[Sugar-devel] Proxy Settings in Network Control Panel

[Manuel Quiñones]

2013/3/14 Ajay Garg <ajay at activitycentral.com>:
> 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 :)

Ah OK.  It might have been anything else then, the bug was
reproduceable with the given testcase.

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

In the case of using callbacks, it is saner to do what Sasha did,
adding a timestamp.  Otherwise you end writting the setting each time
a signal is sent.  Each time a user presses a key for example.  In the
case of using bind, GTK takes care of that. as the documentation say.

>
>
>>
>> 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.

Yes, Browse gets the proxy settings from GSettings and does not need
$http_proxy .

> 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

I guess $http_proxy is a fallback.

>
>
>>
>>
>> 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



-- 
.. manuq ..