[Sugar-devel] [PATCH] adding $HOME/Documents to volumestoolbar

[Simon Schampijer]

On 08/16/2011 05:08 PM, Walter Bender wrote:
> On Tue, Aug 16, 2011 at 9:09 AM, Simon Schampijer<simon at schampijer.de>  wrote:
>> Hi Walter,
>>
>> thanks for the patch! Comments inline:
>>
>> On 08/15/2011 05:16 PM, Walter Bender wrote:
>>>
>>> From: Walter Bender<walter.bender at gmail.com>
>>>
>>> This patch adds $HOME/Documents to the volume toolbar in the Journal view.
>>> The rationale is to make it easier for people to move files in and out of
>>> the data store from the file system -- a feature oft requested by
>>> teachers.
>>> It also means that Sugar activities can more readily access files
>>> generated
>>> outside of Sugar -- another feature requested by teachers.
>>>
>>> This resubmission of the patch includes a suggestion by Silbe that the
>>> xdg-user-dir call be factored out in a separate (global) function
>>>
>>> Note that this patch requires the inclusion of a new icon, user-documents,
>>> that is included in a separate patch.
>>>
>>> See 1313419398-9452-1-git-send-email-walter at sugarlabs.org
>>>
>>> ---
>>>   src/jarabe/journal/volumestoolbar.py |   42
>>> ++++++++++++++++++++++++++++++++++
>>>   1 files changed, 42 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/src/jarabe/journal/volumestoolbar.py
>>> b/src/jarabe/journal/volumestoolbar.py
>>> index 72b5918..e1d1721 100644
>>> --- a/src/jarabe/journal/volumestoolbar.py
>>> +++ b/src/jarabe/journal/volumestoolbar.py
>>> @@ -16,6 +16,8 @@
>>>
>>>   import logging
>>>   import os
>>> +import subprocess
>>> +import errno
>>>   import statvfs
>>>   from gettext import gettext as _
>>>
>>> @@ -53,6 +55,19 @@ def _get_id(document):
>>>           return None
>>>
>>
>> The _get_documents_path() has the wrong indentation.
>>
>>> +def _get_documents_path():
>>
>> Please add a docstring here.
>>
>>> +        try:
>>> +            pipe = subprocess.Popen(['xdg-user-dir', 'DOCUMENTS'],
>
> Will take care of this.
>
>>
>> You are introducing a new dependency here: xdg-user-dirs. Any specific
>> reason why you do not want to use the HOME env variable?
>>
>>  From a quick survey: I guess relying on the env variable HOME like you do in
>> your other patch would be fine as well. You could as well do
>> "os.getenv('XDG_DATA_HOME', os.path.expanduser('~/'))" so once a
>> xdg_data_home is set we do not fail (we do this as well in the toolkit), but
>> I would maybe fix this up in another go if we want to.
>
> The issue is not so much HOME as DOCUMENTS. Let me investigate further.
>
> thanks for the review.
>
> -walter

Ahh, I actually misunderstood a subtle detail all the time: you only 
make $HOME/DOCUMENTS available. Indeed for this case it makes sense to 
use xdg and not a hardcoded path based on $HOME. The xdg [1] way does 
cope with the use case where a user changes the language and as well his 
directory names (e.g. s/Documents/Dokumente).

Regards,
    Simon

[1] http://www.freedesktop.org/wiki/Software/xdg-user-dirs