2013/3/14 Ajay Garg <a...@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 <ma...@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 .. _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel