> Big rough comment I quickly wrote down when I checked the commits. Its great > work! Don't let my bad comments depress you in any way, I am a picky bastard. > All of these items are easy to fix. Its just a matter of not losing track. I'm aware that the branch could use some work as I'm far from being an experienced programmer. But I'm comfortable with critics, keep them coming ;-). > foreseable bugs > * it doesn't do the right thing with a tomdroid/ filled sdcard right now > (upgrade path broken) The current behaviour is to overwrite the existing note with the same guid. This means it replaces the notes in the database with the ones on the SD card. The problem I can see with this approach is that it doesn't delete the notes that don't exist anymore on the SD card. Apart from that, I don't see what would be broken in case of an upgrade. > * no mechansim to add notes in the list activity as of revno 144 (but its not > necessary yet) The NoteProvider is used as the list store. This means the main list automatically picks up the notes that are inserted in the provider, there is no need to add them manually. > * revno 146, links directly from an activity to an external class that can > take its time, bad. Always thread work that can be blocking (see > http://developer.android.com/guide/topics/fundamentals.html) Agreed, I put a TODO to put this code in a thread but I forgot about it. > open design questions > * who should responsable of adding a note to the NoteProvider? Note, Async.. > or NoteProvider? see revno 147 Good question. What I have in mind is an abstraction of the provider which is currently a bit cumbersome to use. Something like a NoteManager, responsible for dealing with the provider (ie. inserting notes, getting a Note object from the provider, etc). This would make these operations easier to do as its interfaces would only deal with Note objects, but I don't know if adding something like this would be overkill or not. > * Tomdroid UI with file exceptions.. I don't like that, I think we should > abstract that away using a static class in util package maybe? revno 149 I completely agree with that. Moreover, I'm not comfortable with presenting the errors as modal popups in the application. Something less intrusive would be to use the notification area and only present the full error message if the user clicks on the notification. > * is Java's guid methods working the same way as mono ones? revno 153, 154 I believe so, the java and mono implementation are based on a RFC. The mono one is based on the draft[0] while the java one is based on the actual RFC[1]. [0]http://anonsvn.mono-project.com/viewvc/trunk/mcs/class/corlib/System/Guid.cs?revision=HEAD [1]http://developer.android.com/reference/java/util/UUID.html > issues > * revno 148, what's up with NoteHandler's job? Synchronization services like Snowy expect the note content to be sent as raw xml so I thought it would be a good idea to store it in the database. The SpannableStringBuilder is now created lazily in the Note object which means that once the initial import is done, no xml parsing is required to show the main list. > * revno 149, onListItemClick, we should do ID instead of filenames now? (maybe > fixed by 151) Yes, rev 151 passes the note with a id-based uri. > revno 152 > * Note = new note() and setting content inside of ViewNote, that's not the > spirit, ViewNote should fetch a note based on some criteria and ask it to show > itself, it shouldn't have to deal with a note's details See my previous comment about a NoteManager. It would be responsible for such an operation and return a filled Note object. > * buildlinkify pattern, should it really end up there? Maybe in the Note object, which would then do everything related to the SpannableStringBuilder construction? > why is revno155 and 156 necessary? what is it solving? Rev 155: special characters like < and > have to be escaped, else it is not valid XML. Rev 156: I got the parsing logic wrong, it was adding a tag at the beginning but didn't add the closing one. This commit fixes this mistake.