Re: [Sugar-devel] Proxy Settings in Network Control Panel
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
Re: [Sugar-devel] Proxy Settings in Network Control Panel
On Wed, Mar 13, 2013 at 10:43 PM, Ajay Garg a...@activitycentral.com wrote: 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 GtkLauncher is not a part of WebKit (the library, as used by Sugar). It is a standalone test app. So unless you have another place truly inside webkit where $http_proxy is used, I am doubtful that webkit is using this. Daniel ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Proxy Settings in Network Control Panel
Oh and forgot to say: in my commit message I tried to respect all the information from the original patch, and added new paragraphs to describe the new implementation. If you see anything I missed, please tell me. 2013/3/13 Manuel Quiñones ma...@laptop.org: 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. 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. 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? 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) 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 .. -- .. manuq .. ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] Proxy Settings in Network Control Panel
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 :) 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 ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel