Code review comment for lp:~plaxx/tomdroid/storage-redesign

On Sun, Oct 18, 2009 at 9:19 AM, Benoit Garret <
<email address hidden>> wrote:

> The main point of having an updateContent method was to make sure the xml
> content and the displayable content are always in sync. The caching thing
> was added as an afterthought and I agree it is not the best solution. What
> did you dislike in it: the fact that xml parsing happens in the Note object
> or the update mechanism itself? If it's the former, I'm not against
> delegating the xml parsing to another class, like NoteBuilder.
>
>
Yes, it was the former.

> In fact, I don't like the idea of having two different methods to get and
> set what are in fact two representations of the same content. It would be
> far too easy to set one and not the other and wonder why the content that
> gets synced isn't the one that is displayed.
>

Totally agree with you on this one.

>
> Here's what I'd also like to do (not related to my previous point):
>
> 1/ Remove the load note from web feature. It might seem backwards to remove
> features, but we both know that this one is quite useless and that no one
> will miss it. Moreover, it adds a fair bit of ugly code to handle a code
> path that will get removed sooner than later.
>

Yes, and I saw that you did remove it in a later message. Although, I would
have preferred a new branch or a patch (but I'm being an ass here). I'll
merge or cherry-pick, I agree with you: its time to get rid of that

>
> 2/ Create a NoteManager to abstract the complexity of dealing with the
> content provider, with the following interfaces:
> public Note getNote(UUID guid, Handler callback) // gets a note from the
> content provider
> public Note getNote(URL url, Handler callback) // gets a note from a remote
> url
> public void putNote(Note note) // puts a note in the content provider
> public ListAdapter getListAdapter()
> public ArrayList getTitles() // gets the titles of the notes present in the
> db, used in ViewNote.buildLinkifyPattern()
> public TransformFilter getTitleToUrl() // returns the transform filter used
> for the note titles
>
> Point 2 amounts to get rid of references to the content provider everywhere
> except in the NoteManager. This would clean the code a lot and avoid having
> connections to the content provider, which is far from optimal.
>

I don't mind, actually, I like it, its cleaner. Just being curious: how is
linking to the content provider inefficient?