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
