[tomboy-list] WIP Note Directory Watcher Patch (was Watch for Changes Patch)

[Sandy Armstrong]

On Sun, May 3, 2009 at 8:07 AM, Michael Fletcher
<m.fletcher at theplanet.ca> wrote:
>> +             private void OnNoteSaved (Note note)
>> +             {
>> +                     note_save_times [note.Id] = DateTime.Now;
>> +             }
> Do you want to call this HandleNoteSaved?

Changed.

>> +                                     // TODO: Why are we throwing an exception here?
>> +                                     //       What are the repercussions of this?
>>                                       throw new Exception (message);
> I threw an exception because it should not be possible.  I have
> handled all the values of enum WatcherChangeType.
>
> Its a pattern I usually follow: if something impossible happens then
> die gracefully instead of continuing on.  Especially when you are
> modifying data :).

Yes, but I am not crazy about having non-essential add-ins cause
Tomboy to crash, so in this situation, I replaced the throw with a
return.

>> +                     string noteXml = null;
>> +                     try {
>> ... snipped ...
>> +                     } catch (Exception e) {
>> +                             Logger.Error ("NoteDirectoryWatcher: Update aborted, error reading {0}: {1}", note_path, e);
>> +                     }
> We need to return after logging this error.  noteXml is set to null
> and will throw a NPE shortly.

Good catch

>> +                     if (string.IsNullOrEmpty (noteXml)) {
>> ... snipped ...
>> +                             return;
>> +                     }
> Very good idea.
>
> I wonder if we shouldn't put a try-catch around the call to
> AddOrUpdateNote() and then eliminate some of the error handling code.
> I think we could get rid of three exception handlers and a couple of
> result checks.

Yes, you are right.  I am pushing it with this part as-is, for now,
simply because I've already tested it and don't have a lot of time
today.  But we should definitely clean it up as you suggest.

Thanks for the review, this is now pushed to master.  Feel free to
test it out!  :-)

Sandy