Code review comment for lp:~salgado/tomdroid/sync-ui

> On Wed, 2010-08-25 at 20:26 +0000, Rodja wrote:
> > It is great to get rid of the templates but...
> >
> > 1. The tests are not passing if there are already notes in the
> > database (see two-way-branch for an setUp/tearDown example).
> >
>
> So, IIUC, you suggest that I just reset the database as part of
> setUp/tearDown? That sounds fine to me, but the sync-ui branch doesn't
> have LocalStorage (which you use in the two-way-sync branch to reset the
> DB), so I'd have to do it manually for now. Something like:
>
> activity.getContentResolver().delete(Tomdroid.CONTENT_URI, null, null);
> Preferences.putLong(Preferences.Key.LATEST_SYNC_REVISION, 0);
>
> Is that ok with you?

Yes.

> > 2. Rather then introducing a new member to flag a note as "template" I
> > would suggest to seach for the template tag each time
> > isNotebookTemplate() is called. Performance is not an issue, but later
> > refactorings will be harder with each new member variable.
>
> It's not because of performance that I made it a flag -- it's because I
> think we should avoid spending processor cycles unnecessarily, to not
> drain people's batteries.
>
> My thinking was that if you have a long list of notes and you're
> switching back and forth between any note and the notes list, tomdroid
> would be doing a good job at draining your battery.

Is it? Have you tested? I would expect Android to keep the notes list Activity running while showing a Note; and calling onResume when switching back. But guessing is never good when talking about optimizations.

> To be honest I have
> no idea how much power it'd spend while doing that, but it's easily
> avoidable so I thought it was worth it. I'd be OK with dropping the new
> flag as you suggest, if you feel it's not worth.

Power consumtion is not easily measureable. But when looking on the battery usage graph from my Motorola Droid the big consumers are wifi, display, Maps and phone. I prefer having a clean code base which is easier to change in the future than "pollute" it with optimizations.

> > 3. Similar to the second remark, I would sooner see a row for all the
> > tags changing the database when ever we feel about supporting a new
> > tag.
>
> Do you mean you think we shouldn't be changing the DB schema often? I
> don't think that's a problem (thanks to the framework's ability of
> detecting when schema upgrades are needed and doing the migration
> semi-automatically for us), specially when it's as early in the
> lifecycle of the project as we are now.

True. I'm not against changing the data base scheme in general. But sooner or later we will introduce a column to store the tags of a note. Should we remove the system:template tag befor putting the others into that column?

I've no insights about Tomboy server API decisions of having "open-on-startup" and "pinned" as extra flags and "system:template" inside the tag list, but would recommend to stick as close as possible with the already provided scheme to avoid complicated data renderings and difficulties for other Tomboy developers.