Re: [Sugar-devel] [PATCH] Journal entry sharing using a mass storage device

2011-02-21 Thread Simon Schampijer 

On 02/18/2011 03:25 PM, Sascha Silbe wrote:

Excerpts from Simon Schampijer's message of Mon Feb 14 23:13:40 +0100 2011:


http://wiki.sugarlabs.org/go/Features/Journal_Entry_Sharing


We should come up with a better description before we push the patch.


I would name it:

Add functionality to copy a Journal entry including the metadata to a 
removable device



[src/jarabe/journal/model.py]

+import json


As agreed on in IRC, we should stick with simplejson for 0.92.


Done.


+JOURNAL_METADATA_DIR = '.Sugar-Metadata'


Do you intend to use this from other modules?


Yes, from from [PATCH] Convert Journal entries that have been saved to 
a storage device in 0.82.



[InplaceResultSet.find()]

-for file_path, stat, mtime_, size_ in files:
-metadata = _get_file_metadata(file_path, stat)
+for file_path, stat, mtime_, metadata, size_ in files:
+if metadata is None:
+metadata = _get_file_metadata(file_path, stat)


I would prefer to handle metadata reading / construction in a single
place, i.e. to move the _get_file_metadata() call down below the
_get_file_metadata_from_json() call.


Actually, I did that but then the scanning time took longer. So I 
reverted to the previous behavior.



As mentioned on IRC we should remove the unused parts from the list. I
don't mind doing it in a follow-up patch, though.


Actually, I think you need these in order to do the filter the entries.


[InplaceResultSet._scan_a_file()]

@@ -365,7 +371,20 @@ class InplaceResultSet(BaseResultSet):

  if self._regex is not None and \
  not self._regex.match(full_path):
-return
+filename = os.path.basename(full_path)
+dir_path = os.path.dirname(full_path)
+metadata = _get_file_metadata_from_json( \
+dir_path, filename, preview=False)
+add_to_list = False
+if metadata is not None:
+for f in ['fulltext', 'title',
+  'description', 'tags']:
+if f in metadata and \
+self._regex.match(metadata[f]):
+add_to_list = True
+break
+if not add_to_list:
+return


This is getting rather deeply nested again. If we move it below the
other checks we can unconditionally parse resp. construct the metadata
and move the full text matches two indentation levels up.


I was able to remove one level. I hope this is ok now.


BTW, is it intentional that you search only a subset of the properties,
unlike what the data store does?


Before this patch you could only search for filenames on a removable 
device. Now you can as well search for title, description, tags for 
items that do have metadata. In general searches like 'Write activity 
entries' are not supported for a removable device. Maybe we could add 
this later, for now I think it is fine like that.



[InplaceResultSet._get_file_metadata()]

  def _get_file_metadata(path, stat):


We should consider renaming this to make clears it's constructing the
metadata based on file system metadata and heuristics (MIME type), as
opposed to reading the serialised metadata from disk as
_get_file_metadata_from_json() does.


The method gets now the metadata for a file, if available from the json 
file otherwise it constructs it. So I think this is the correct name now.



+def _get_file_metadata_from_json(dir_path, filename, preview=False):


The reference to preview in the code below confused me for a few
minutes. We should rename it to e.g. fetch_preview.


Good catch, done.


+Returns the metadata from the json file and the preview
+stored on the external device. If the metadata is corrupted
+we do remove it and the preview as well.


PEP 257 (referenced by PEP 8) says:


The docstring is a phrase ending in a period. It prescribes the
function or method's effect as a command (Do this, Return that),
not as a description; e.g. don't write Returns the pathname 

[...]

Multi-line docstrings consist of a summary line just like a one-line
docstring, followed by a blank line, followed by a more elaborate
description.


Done.


+metadata = None
+metadata_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
+ filename + '.metadata')
+preview_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
+filename + '.preview')
+
+if os.path.exists(metadata_path):
+try:


Please change this to guardian style to a) get rid of the indentation
and b) prevent an exception (that currently gets caught by your
catch-all except:) if the preview exists, but the metadata file doesn't.
I.e. please use:

 if not os.path.exists(metadata_path):
 return None


Done.


+metadata = json.load(open(metadata_path))
+except ValueError:


We should catch EnvironmentError (base class of IOError

Re: [Sugar-devel] [PATCH] Journal entry sharing using a mass storage device

2011-02-18 Thread Sascha Silbe 
Excerpts from Simon Schampijer's message of Mon Feb 14 23:13:40 +0100 2011:

 http://wiki.sugarlabs.org/go/Features/Journal_Entry_Sharing

We should come up with a better description before we push the patch.


[src/jarabe/journal/model.py]
 +import json

As agreed on in IRC, we should stick with simplejson for 0.92.

 +JOURNAL_METADATA_DIR = '.Sugar-Metadata'

Do you intend to use this from other modules?


[InplaceResultSet.find()]
 -for file_path, stat, mtime_, size_ in files:
 -metadata = _get_file_metadata(file_path, stat)
 +for file_path, stat, mtime_, metadata, size_ in files:
 +if metadata is None:
 +metadata = _get_file_metadata(file_path, stat)

I would prefer to handle metadata reading / construction in a single
place, i.e. to move the _get_file_metadata() call down below the
_get_file_metadata_from_json() call.

As mentioned on IRC we should remove the unused parts from the list. I
don't mind doing it in a follow-up patch, though.


[InplaceResultSet._scan_a_file()]
 @@ -365,7 +371,20 @@ class InplaceResultSet(BaseResultSet):
  
  if self._regex is not None and \
  not self._regex.match(full_path):
 -return
 +filename = os.path.basename(full_path)
 +dir_path = os.path.dirname(full_path)
 +metadata = _get_file_metadata_from_json( \
 +dir_path, filename, preview=False)
 +add_to_list = False
 +if metadata is not None:
 +for f in ['fulltext', 'title',
 +  'description', 'tags']:
 +if f in metadata and \
 +self._regex.match(metadata[f]):
 +add_to_list = True
 +break
 +if not add_to_list:
 +return

This is getting rather deeply nested again. If we move it below the
other checks we can unconditionally parse resp. construct the metadata
and move the full text matches two indentation levels up.

BTW, is it intentional that you search only a subset of the properties,
unlike what the data store does?


[InplaceResultSet._get_file_metadata()]
  def _get_file_metadata(path, stat):

We should consider renaming this to make clears it's constructing the
metadata based on file system metadata and heuristics (MIME type), as
opposed to reading the serialised metadata from disk as
_get_file_metadata_from_json() does.


 +def _get_file_metadata_from_json(dir_path, filename, preview=False):

The reference to preview in the code below confused me for a few
minutes. We should rename it to e.g. fetch_preview.

 +Returns the metadata from the json file and the preview
 +stored on the external device. If the metadata is corrupted
 +we do remove it and the preview as well.

PEP 257 (referenced by PEP 8) says:

 The docstring is a phrase ending in a period. It prescribes the
 function or method's effect as a command (Do this, Return that),
 not as a description; e.g. don't write Returns the pathname 
[...]
 Multi-line docstrings consist of a summary line just like a one-line
 docstring, followed by a blank line, followed by a more elaborate
 description.

 +metadata = None
 +metadata_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
 + filename + '.metadata')
 +preview_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
 +filename + '.preview')
 +
 +if os.path.exists(metadata_path):
 +try:

Please change this to guardian style to a) get rid of the indentation
and b) prevent an exception (that currently gets caught by your
catch-all except:) if the preview exists, but the metadata file doesn't.
I.e. please use:

if not os.path.exists(metadata_path):
return None

 +metadata = json.load(open(metadata_path))
 +except ValueError:

We should catch EnvironmentError (base class of IOError and OSError),
too. The external medium might be formatted using a non-FAT file system
(= permission denied) or be defective (many different errors).

 +os.unlink(metadata_path)
 +if os.path.exists(preview_path):
 +os.unlink(preview_path)

Dito.

 +logging.debug(Could not read metadata for file %r on  \
 +  external device., filename)

Since the file existed and gets removed by us, we should use log level
error.

 +else:
 +metadata['uid'] = os.path.join(dir_path, filename)

If we can't read the metadata (file does not exist or is corrupt), we
should call _get_file_metadata(). That will fill in several properties,
including uid.

I'm not sure if we should try reading the preview if we couldn't read
the metadata file.

 +if preview:

Again, please use guardian style.

 +if os.path.exists(preview_path):
 +try:
 +metadata['preview'] =

[Sugar-devel] [PATCH] Journal entry sharing using a mass storage device

2011-02-14 Thread Simon Schampijer 
http://wiki.sugarlabs.org/go/Features/Journal_Entry_Sharing
---
 src/jarabe/journal/model.py |  188 +++
 1 files changed, 171 insertions(+), 17 deletions(-)

diff --git a/src/jarabe/journal/model.py b/src/jarabe/journal/model.py
index 320e577..67d2f19 100644
--- a/src/jarabe/journal/model.py
+++ b/src/jarabe/journal/model.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2007-2010, One Laptop per Child
+# Copyright (C) 2007-2011, One Laptop per Child
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -20,13 +20,15 @@ import errno
 from datetime import datetime
 import time
 import shutil
+import tempfile
 from stat import S_IFLNK, S_IFMT, S_IFDIR, S_IFREG
 import re
 from operator import itemgetter
+import json
+from gettext import gettext as _
 
 import gobject
 import dbus
-import gconf
 import gio
 
 from sugar import dispatch
@@ -46,6 +48,8 @@ PROPERTIES = ['activity', 'activity_id', 'buddies', 
'bundle_id',
 MIN_PAGES_TO_CACHE = 3
 MAX_PAGES_TO_CACHE = 5
 
+JOURNAL_METADATA_DIR = '.Sugar-Metadata'
+
 _datastore = None
 created = dispatch.Signal()
 updated = dispatch.Signal()
@@ -295,8 +299,9 @@ class InplaceResultSet(BaseResultSet):
 files = self._file_list[offset:offset + limit]
 
 entries = []
-for file_path, stat, mtime_, size_ in files:
-metadata = _get_file_metadata(file_path, stat)
+for file_path, stat, mtime_, metadata, size_ in files:
+if metadata is None:
+metadata = _get_file_metadata(file_path, stat)
 metadata['mountpoint'] = self._mount_point
 entries.append(metadata)
 
@@ -324,6 +329,7 @@ class InplaceResultSet(BaseResultSet):
 
 def _scan_a_file(self):
 full_path = self._pending_files.pop(0)
+metadata = None
 
 try:
 stat = os.lstat(full_path)
@@ -365,7 +371,20 @@ class InplaceResultSet(BaseResultSet):
 
 if self._regex is not None and \
 not self._regex.match(full_path):
-return
+filename = os.path.basename(full_path)
+dir_path = os.path.dirname(full_path)
+metadata = _get_file_metadata_from_json( \
+dir_path, filename, preview=False)
+add_to_list = False
+if metadata is not None:
+for f in ['fulltext', 'title',
+  'description', 'tags']:
+if f in metadata and \
+self._regex.match(metadata[f]):
+add_to_list = True
+break
+if not add_to_list:
+return
 
 if self._date_start is not None and stat.st_mtime  self._date_start:
 return
@@ -378,7 +397,8 @@ class InplaceResultSet(BaseResultSet):
 if mime_type not in self._mime_types:
 return
 
-file_info = (full_path, stat, int(stat.st_mtime), stat.st_size)
+file_info = (full_path, stat, int(stat.st_mtime), metadata,
+ stat.st_size)
 self._file_list.append(file_info)
 
 return
@@ -401,7 +421,17 @@ class InplaceResultSet(BaseResultSet):
 
 
 def _get_file_metadata(path, stat):
-client = gconf.client_get_default()
+Returns the metadata from the corresponding file
+on the external device or does create the metadata
+based on the file properties.
+
+
+filename = os.path.basename(path)
+dir_path = os.path.dirname(path)
+metadata = _get_file_metadata_from_json(dir_path, filename, preview=True)
+if metadata:
+return metadata
+
 return {'uid': path,
 'title': os.path.basename(path),
 'timestamp': stat.st_mtime,
@@ -409,10 +439,46 @@ def _get_file_metadata(path, stat):
 'mime_type': gio.content_type_guess(filename=path),
 'activity': '',
 'activity_id': '',
-'icon-color': client.get_string('/desktop/sugar/user/color'),
+'icon-color': '#00,#ff',
 'description': path}
 
 
+def _get_file_metadata_from_json(dir_path, filename, preview=False):
+Returns the metadata from the json file and the preview
+stored on the external device. If the metadata is corrupted
+we do remove it and the preview as well.
+
+
+metadata = None
+metadata_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
+ filename + '.metadata')
+preview_path = os.path.join(dir_path, JOURNAL_METADATA_DIR,
+filename + '.preview')
+
+if os.path.exists(metadata_path):
+try:
+metadata = json.load(open(metadata_path))
+except ValueError:
+os.unlink(metadata_path)
+if os.path.exists(preview_path):
+os.unlink(preview_path)
+logging.debug(Could not read