Thanks a ton Manuel; I will test the patch on the weekend, and let you know :) <div class="gmail_quote">On Thu, Mar 14, 2013 at 7:27 AM, Manuel Quiñones <<a href="mailto:manuq@laptop.org" target="_blank">manuq@laptop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <a href="https://bugs.sugarlabs.org/attachment/ticket/3070/0001-Add-proxy-configuration-support-to-Network-Control-P.patch" target="_blank">https://bugs.sugarlabs.org/attachment/ticket/3070/0001-Add-proxy-configuration-support-to-Network-Control-P.patch</a> 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.</blockquote><div> 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) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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: <a href="https://bugs.sugarlabs.org/attachment/ticket/3070/proxy_gsettings.patch" target="_blank">https://bugs.sugarlabs.org/attachment/ticket/3070/proxy_gsettings.patch</a> (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. </blockquote><div> 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 <a href="http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041779.html">http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041779.html</a> :) Anyways, the end result is that, we HAVE TAKEN care of the racy-behaviour, as far as the persistence of values is concerned. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <a href="https://developer.gnome.org/gio/stable/GSettings.html#GSettings-changed" target="_blank">https://developer.gnome.org/gio/stable/GSettings.html#GSettings-changed</a> 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): <a href="http://lists.sugarlabs.org/archive/sugar-devel/2012-August/038804.html" target="_blank">http://lists.sugarlabs.org/archive/sugar-devel/2012-August/038804.html</a> 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? </blockquote><div> 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 <a href="http://svn.webkit.org/repository/webkit/trunk/Tools/GtkLauncher/main.c">http://svn.webkit.org/repository/webkit/trunk/Tools/GtkLauncher/main.c</a> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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: <a href="https://developer.gnome.org/gio/stable/GSettings.html#g-settings-bind" target="_blank">https://developer.gnome.org/gio/stable/GSettings.html#g-settings-bind</a> 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) </blockquote><div> These look good !!! </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 .. </blockquote></div> -- Regards, Ajay Garg Dextrose Developer Activity Central: <a href="http://activitycentral.com/" style="font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)" target="_blank">http://activitycentral.com</a>