[Sugar-devel] [PATCH] adding $HOME/Documents to the volumes toolbar

[Sascha Silbe]

Excerpts from Walter Bender's message of Thu May 19 13:56:51 +0200 2011:

> Also attached is a new version of the patch to volumestoolbar.py since
> Gary's icon has a different name than the one I had been using.

Please send patches inline for easier review and to see them on
Patchwork, making sure we don't forget about them. The following section
from the git send-email manual page might be useful:

>> USE GMAIL AS THE SMTP SERVER
>>        Add the following section to the config file:

>>            [sendemail]
>>                    smtpencryption = tls
>>                    smtpserver = smtp.gmail.com
>>                    smtpuser = yourname at gmail.com
>>                    smtpserverport = 587

>>        Note: the following perl modules are required Net::SMTP::SSL,
>>        MIME::Base64 and Authen::SASL



> Subject: [PATCH] add user documents to volumes toolbar

> ---

Slightly more description would be nice. E.g. why we'd want to do this.


[src/jarabe/journal/volumestoolbar.py]
> @@ -181,6 +182,7 @@ class VolumesToolbar(gtk.Toolbar):
[...]
> +        gobject.idle_add(self._setup_documents_button)
>          gobject.idle_add(self._set_up_volumes)

I'm not sure whether GTK makes any guarantee on the execution order of
idle callbacks. By calling self._setup_documents_button() from inside
self._set_up_volumes() we can be sure it always gets run in the same
order, thus always appearing the same way in the Journal.
 

> @@ -188,6 +190,25 @@ class VolumesToolbar(gtk.Toolbar):
[...]
> +    def _setup_documents_button(self):
> +        documents_path = subprocess.Popen([
> +                "xdg-user-dir", "DOCUMENTS"],
> +                stdout=subprocess.PIPE).stdout.read().strip()

I'm not sure this is race-free [1]. We should also check the return code
just in case it outputs anything that isn't the documents path, but
somehow still exists (e.g. due to a cut&paste mistake creating files
with "weird" file names in the home directory):

        pipe = subprocess.Popen(['xdg-user-dir', 'DOCUMENTS'],
                                stdout=subprocess.PIPE)
        documents_path = pipe.communicate()[0].strip()
        if pipe.returncode:
            return

BTW: xdg-user-dir is a new dependency and should be documented clearly
(commit description, Release Notes). If we want to avoid this dependency
on systems that don't need ~/Documents interoperation, we could catch
OSError:

        try:
            pipe = subprocess.Popen(['xdg-user-dir', 'DOCUMENTS'],
                                    stdout=subprocess.PIPE)
        documents_path = pipe.communicate()[0].strip()
        except OSError, exception:
            if exception.errno != errno.ENOENT:
                logging.exception('Could not run xdg-user-dir')
            return


[...]
> +        position = len(self._volume_buttons) - 1
> +        self.insert(button, position)

self.insert(button, -1) should do the trick.


[...]
> +class DocumentsButton(BaseButton):
> +    def __init__(self, documents_path):
> +        self.__mount_directory_path = documents_path

Why do you hide the path from subclasses? Speaking of it, why do you
save it at all when BaseButton already populates self.mount_point with
the value you pass in in the following line:

+        BaseButton.__init__(self, mount_point=self.__mount_directory_path)

[...]
+    def get_mount_point(self):
+        return self.__mount_directory_path

Who do you export this API for? I don't see any user in your patch or
the original code. And if there were, why not just use self.mount_point?
It's public already.

Sascha

[1] https://bugs.sugarlabs.org/ticket/397
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 500 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110706/0b316f31/attachment.pgp>