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

[Simon Schampijer]

On 08/17/2011 02:10 PM, Simon Schampijer wrote:
> 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

I did cleanup the smaller things mentioned above and pushed the patch.

- I did push Martin's patch [1] to not have the Journal looping forever 
and freezing the UI when you want to show the content of DOCUMENTS

- 'xdg-user-dir DOCUMENTS' will return $HOME when DOCUMENTS does not 
exist, I think we should only display the DOCUMENTS option when the 
actual folder exists, hence adding a simple check like [2]

- it would be great to have copy-to-document as an option in the new 
copy-to palette/detail view and vice versa adjust the palette in the 
document palette as well

Regards,
   Simon


[1] http://lists.sugarlabs.org/archive/sugar-devel/2011-June/031781.html
[2] diff --git a/src/jarabe/journal/volumestoolbar.py 
b/src/jarabe/journal/volumestoolbar.py
index 7a34e18..e0ee79c 100644
--- a/src/jarabe/journal/volumestoolbar.py
+++ b/src/jarabe/journal/volumestoolbar.py
@@ -68,7 +68,8 @@ def _get_documents_path():
          pipe = subprocess.Popen(['xdg-user-dir', 'DOCUMENTS'],
                                  stdout=subprocess.PIPE)
          documents_path = pipe.communicate()[0].strip()
-        if os.path.exists(documents_path):
+        if os.path.exists(documents_path) and \
+                os.environ.get('HOME') + '/' != documents_path:
              return documents_path
      except OSError, exception:
          if exception.errno != errno.ENOENT: