[Sugar-devel] New Chapter in Make Your Own Sugar Activities needs review

[Bert Freudenberg]

On 28.03.2010, at 04:04, James Simmons wrote:
> 
> In the last version of the book I had some content on working with the
> Journal in the "Adding Refinements" chapter but it wasn't much and
> didn't belong there.  After finishing Sugar Commander I felt that I
> had a good example to use for a whole chapter on working with the
> Journal, so I removed the content from "Adding Refinements" and added
> the new chapter.  The code in Sugar Commander works, but I'd still
> appreciate a review of this new information to be certain that I have
> the details right.

Nice chapter :)

There is one serious flaw in your Sugar Commander code:

	datastore.find({})

You should never call that find method without narrowing the results, at least in production code. Since you only use specific properties, you should tell the datastore to only return these. Otherwise, it will send megabytes of unused preview data (and arbitrary meta data you cannot even anticipate) over D-Bus if there are hundreds of Journal entries, which is not at all uncommon. So the right way would be to write

	ds_objects, num_objects = datastore.find({}, properties=['uid', 'title', 'mime_type', 'mountpoint'])

assuming those are the properties you need, which is not that much more complicated. You always have to include 'uid', otherwise the find() method breaks. Which is a bug IMHO ;)

For filtering by mountpoint, it's better to give this as a query argument rather than fetching all entries and discarding the unwanted entries later:

	query = {}
	if mountpoint_id is not None:
		query['mountpoints'] = [ mountpoint_id ]
	ds_objects, num_objects = datastore.find(query, properties=['uid', 'title', 'mime_type'])

This is more efficient, but not as important as specifying the returned properties. 

The real Journal also employs the technique of only fetching the visible entries, a page at a time, by adding 'limit' and 'offset' arguments to the find call. This would complicate the discussion so it's okay to leave out I guess. But narrowing the find() results to the needed properties is essential.

Later down you write "Metadata entries for Journal objects alwayts [sic] contain strings" which is only true for Sugar 0.82. In Sugar 0.84 and later the entries are actually byte arrays, not strings. This also makes this later statement incorrect: "... since the Journal has never been able to store binaries as metadata ...". In fact, newer Sugar versions _do_ store the preview as binary, not base64 encoded.

This is also not quite correct: "the [preview] image file is always 320 x 240 pixels". The preview can actually be of arbitrary size, the recommended resolution is (was?) 300 x 225 pixels, a quarter in each dimension of the XO's 1200x900 screen size.

I also noticed some typos, but found the chapter too entertaining to write them down ... just run a spell checker ;)

- Bert -