Tomdroid

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

sync-ui
Merge into sync-ui

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-08-25:

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?

> 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. 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.

>
> 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.

--
Guilherme Salgado <https://launchpad.net/~salgado>

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?

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. 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.

> 
> 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.

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

« Back to merge proposal