Hello Daniel,

first of all, as Simon mentioned, this patch is the proper solution to make 
gnome-keyring-daemon start a new instance within the windowed sugar session.

That said, here are a few suggestions.

> Subject: [sugar] Unset gnome keyring environment variables
Ok, the patch does this.  But why?  What about "Enable gnome-keyring-daemon to 
run within sugar-emulator"?

> Rather than starting it manually. As suggested on
> https://bugzilla.gnome.org/show_bug.cgi?id=628302

It's great you included the link to the bug report.  It helped me find the 
necessary information to understand the patch.  But why didn't you explain the 
patch, either in your own words or by including the explanation Stef Walter 
gave within the bug report?  It explains both the problem and the solution.


> @@ -120,6 +120,11 @@ def _start_window_manager():
>
>
>  def _setup_env(display, scaling, emulator_pid):
> +    for variable in ['GPG_AGENT_INFO', 'SSH_AUTH_SOCK',
> +                     'GNOME_KEYRING_CONTROL', 'GNOME_KEYRING_PID']:
> +        if variable in os.environ:
> +            del os.environ[variable]
> +
>      os.environ['SUGAR_EMULATOR'] = 'yes'

This patch is straight forward, it does its job.  But the code is not 
self-explaining as it's not clear from the code, that this allows, 
gnome-keyring-daemon to start a new instance within the sugar-emulator.  So you 
need to step in and provide a comment explaining the reasoning behind.


Ok, that's it.  Nice job.  I am looking forward to more patches from you.

Caspar
_______________________________________________
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Reply via email to