WIP Note Directory Watcher Patch (was Watch for Changes Patch)
|
View:
New views
11 Messages
—
Rating Filter:
Alert me
|
 |
WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Michael Fletcher-3
::
Rate this Message:
This is an updated version of the Note Directory Watcher Patch. This
version is now useful. It can detect changes to notes in ~/.tomboy. It CANNOT detect new notes or notes that have been deleted. I will finish that tonight or next week. [watch.patch] diff --git a/Tomboy/Addins/Makefile.am b/Tomboy/Addins/Makefile.am index 57f5f56..9520b67 100644 --- a/Tomboy/Addins/Makefile.am +++ b/Tomboy/Addins/Makefile.am @@ -13,5 +13,6 @@ SUBDIRS = \ SshSyncService \ StickyNoteImport \ Tasque \ + NoteDirectoryWatcher \ WebDavSyncService diff --git a/Tomboy/Addins/NoteDirectoryWatcher/Makefile.am b/Tomboy/Addins/NoteDirectoryWatcher/Makefile.am new file mode 100644 index 0000000..8250fd2 --- /dev/null +++ b/Tomboy/Addins/NoteDirectoryWatcher/Makefile.am @@ -0,0 +1,39 @@ +include $(top_srcdir)/Makefile.include + +CSFLAGS = \ + -debug \ + -define:DEBUG \ + -target:library + +ASSEMBLIES = \ + $(LINK_TOMBOY_EXE) \ + $(GTKSHARP_LIBS) \ + $(LINK_MONO_ADDINS) \ + -r:Mono.Posix + +ADDIN_NAME = NoteDirectoryWatcher +TARGET = $(ADDIN_NAME).dll +CSFILES = \ + $(srcdir)/NoteDirectoryWatcherApplicationAddin.cs +RESOURCES = \ + -resource:$(srcdir)/$(ADDIN_NAME).addin.xml + +$(TARGET).mdb: $(TARGET) + +$(TARGET): $(CSFILES) $(top_builddir)/Tomboy/Tomboy.exe + $(CSC) -out:$@ $(CSFLAGS) $(ASSEMBLIES) $(CSFILES) $(RESOURCES) + + +addinsdir = $(pkglibdir)/addins +addins_DATA = \ + $(TARGET) \ + $(TARGET).mdb + +EXTRA_DIST = \ + $(CSFILES) \ + $(srcdir)/$(ADDIN_NAME).addin.xml + +CLEANFILES = \ + $(TARGET).mdb \ + $(TARGET) + diff --git a/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcher.addin.xml b/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcher.addin.xml new file mode 100644 index 0000000..6bfd500 --- /dev/null +++ b/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcher.addin.xml @@ -0,0 +1,22 @@ +<Addin id="NoteDirectoryWatcher" + namespace="Tomboy" + name="Note Directory Watcher" + author="Tomboy Project" + description="Watch your Tomboy note directory for changes to your notes." + category="Tools" + defaultEnabled="false" + version="0.1"> + + <Runtime> + <Import assembly="NoteDirectoryWatcher.dll" /> + </Runtime> + + <Dependencies> + <Addin id="Tomboy" version="0.10" /> + </Dependencies> + + <Extension path="/Tomboy/ApplicationAddins"> + <ApplicationAddin type="Tomboy.NoteDirectoryWatcher.NoteDirectoryWatcherApplicationAddin" /> + </Extension> + +</Addin> diff --git a/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcherApplicationAddin.cs b/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcherApplicationAddin.cs new file mode 100644 index 0000000..c63cf93 --- /dev/null +++ b/Tomboy/Addins/NoteDirectoryWatcher/NoteDirectoryWatcherApplicationAddin.cs @@ -0,0 +1,160 @@ + +using System; +using System.Collections.Generic; +using System.IO; + +using Tomboy; + +namespace Tomboy.NoteDirectoryWatcher +{ + class NoteFileChangeRecord { + public DateTime last_change; + public bool deleted; + public bool changed; + } + + public class NoteDirectoryWatcherApplicationAddin : ApplicationAddin + { + private FileSystemWatcher file_system_watcher; + private bool initialized; + + private Dictionary<string, NoteFileChangeRecord> file_change_records; + + public override void Initialize () + { + string note_path = Tomboy.DefaultNoteManager.NoteDirectoryPath; + + file_change_records = new Dictionary<string, NoteFileChangeRecord>(); + + file_system_watcher = new FileSystemWatcher (note_path); + + file_system_watcher.Changed += HandleFileSystemChangeEvent; + file_system_watcher.Deleted += HandleFileSystemChangeEvent; + file_system_watcher.Created += HandleFileSystemChangeEvent; + file_system_watcher.Renamed += HandleFileSystemChangeEvent; + + file_system_watcher.Error += HandleFileSystemErrorEvent; + + // Setting to true will starts the FileSystemWatcher. + file_system_watcher.EnableRaisingEvents = true; + + initialized = true; + } + + public override void Shutdown () + { + file_system_watcher.EnableRaisingEvents = false; + + initialized = false; + } + + public override bool Initialized + { + get { + return initialized; + } + } + + private void HandleFileSystemErrorEvent (Object sender, ErrorEventArgs arg) { + // TODO Rescan the local notes in case some of them have changed. + Console.WriteLine("Handling error event: " + arg); + } + + private void HandleFileSystemChangeEvent (Object sender, FileSystemEventArgs arg) { + // Record that the file has been added/changed/deleted. adds/changes trump + // deletes. Record the date. + lock (file_change_records) + { + string note_id = GetId(arg.FullPath); + + NoteFileChangeRecord record = null; + + if (file_change_records.ContainsKey(note_id)) { + record = file_change_records[note_id]; + } else { + record = new NoteFileChangeRecord(); + file_change_records[note_id] = record; + } + + if (arg.ChangeType == WatcherChangeTypes.Changed) { + record.changed = true; + } else if (arg.ChangeType == WatcherChangeTypes.Created) { + record.changed = true; + } else if (arg.ChangeType == WatcherChangeTypes.Deleted) { + if (!record.changed) { + record.deleted = true; + } + } else { + throw new Exception("Unexpected WatcherChangeType " + arg.ChangeType); + } + + record.last_change = DateTime.Now; + } + + GLib.Timeout.Add(5000, new GLib.TimeoutHandler(HandleTimeout)); + } + + private bool HandleTimeout() { + lock (file_change_records) { + List<string> keysToRemove = new List<string>(file_change_records.Count); + + foreach (KeyValuePair<string, NoteFileChangeRecord> pair in file_change_records) { + if (pair.Key != "") { // TODO + if (DateTime.Now > pair.Value.last_change.Add (new TimeSpan(4000)) ) { + Console.WriteLine(pair); + if (pair.Value.deleted) { + DeleteNote(pair.Key); + } else { + AddOrUpdateNote(pair.Key); + } + + keysToRemove.Add(pair.Key); + } + } + } + + foreach (string note_id in keysToRemove) { + file_change_records.Remove(note_id); + } + } + + return false; + } + + private static void DeleteNote(string note_id) { + // TODO Delete the note. + } + + private static void AddOrUpdateNote(string note_id) { + bool found = false; + + foreach (Note note in Tomboy.DefaultNoteManager.Notes) { + if (note.Id == note_id) { + found = true; + + string note_path = Tomboy.DefaultNoteManager.NoteDirectoryPath + + Path.DirectorySeparatorChar + note_id + ".note"; + string note_url = "note://tomboy/" + note_id; + + NoteData data = NoteArchiver.Instance.ReadFile(note_path, note_url); + + if (data.Text != note.XmlContent) { + note.XmlContent = data.Text; + note.Title = data.Title; + } + } + } + + if (!found) { + // TODO Add the note. + } + } + + private static string GetId(string path) { + int lastSlash = path.LastIndexOf(Path.DirectorySeparatorChar); + int firstPeriod = path.IndexOf('.', lastSlash); + + return path.Substring(lastSlash + 1, firstPeriod - lastSlash - 1); + } + } +} diff --git a/configure.in b/configure.in index ab9b951..8563e58 100644 --- a/configure.in +++ b/configure.in @@ -333,6 +333,7 @@ Tomboy/Addins/SshSyncService/Makefile Tomboy/Addins/StickyNoteImport/Makefile Tomboy/Addins/Tasque/Makefile Tomboy/Addins/WebDavSyncService/Makefile +Tomboy/Addins/NoteDirectoryWatcher/Makefile test/Makefile po/Makefile.in ]) _______________________________________________ Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Sandy Armstrong
::
Rate this Message:
On Sun, Apr 26, 2009 at 12:36 PM, Michael Fletcher
<m.fletcher@...> wrote: > This is an updated version of the Note Directory Watcher Patch. This > version is now useful. > > It can detect changes to notes in ~/.tomboy. It CANNOT detect new > notes or notes that have been deleted. I will finish that tonight or > next week. Nice work, Mike. I've got some comments for you: * When updating a note, just use Note.LoadForeignNoteXml, and pass in the complete file contents. It is *probably* best to pass in ChangeType.OtherDataChanged * Recommend using NoteManager.FindByUri instead of doing your own loop (it does the same thing, but could be optimized in the future). * I'm curious about the 5 second delay. What's the point? To ensure that Tomboy "wins" if a note queued to be saved also has its note file edited? * Shouldn't you filter on *.note? Or are you watching all files because of that tmp file stuff that happens when a note is saved? * Please use the Logger class instead of Console.WriteLine * Some coding standards stuff, like needing spaces between methods and open parens, needing to put opening brace of method on its own line, etc Will have to try it out soon. :-) Sandy _______________________________________________ Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Everaldo Canuto-5
::
Rate this Message:
Hey Mike,
Any chance to get it working for deleted and new notes? How hard is to support delete/new ? Everaldo. On Mon, Apr 27, 2009 at 11:18 AM, Sandy Armstrong <sanfordarmstrong@...> wrote: > On Sun, Apr 26, 2009 at 12:36 PM, Michael Fletcher > <m.fletcher@...> wrote: >> This is an updated version of the Note Directory Watcher Patch. This >> version is now useful. >> >> It can detect changes to notes in ~/.tomboy. It CANNOT detect new >> notes or notes that have been deleted. I will finish that tonight or >> next week. > > Nice work, Mike. I've got some comments for you: > > * When updating a note, just use Note.LoadForeignNoteXml, and pass in > the complete file contents. It is *probably* best to pass in > ChangeType.OtherDataChanged > * Recommend using NoteManager.FindByUri instead of doing your own loop > (it does the same thing, but could be optimized in the future). > * I'm curious about the 5 second delay. What's the point? To ensure > that Tomboy "wins" if a note queued to be saved also has its note file > edited? > * Shouldn't you filter on *.note? Or are you watching all files > because of that tmp file stuff that happens when a note is saved? > * Please use the Logger class instead of Console.WriteLine > * Some coding standards stuff, like needing spaces between methods and > open parens, needing to put opening brace of method on its own line, > etc > > Will have to try it out soon. :-) > > Sandy > _______________________________________________ > Tomboy-list mailing list > Tomboy-list@... > http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com > Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Sandy Armstrong
::
Rate this Message:
I think Mike said he's work on that this week. :-)
Sandy On Mon, Apr 27, 2009 at 7:47 AM, Everaldo Canuto <ecanuto@...> wrote: > Hey Mike, > > Any chance to get it working for deleted and new notes? How hard is to > support delete/new ? > > Everaldo. > > On Mon, Apr 27, 2009 at 11:18 AM, Sandy Armstrong > <sanfordarmstrong@...> wrote: >> On Sun, Apr 26, 2009 at 12:36 PM, Michael Fletcher >> <m.fletcher@...> wrote: >>> This is an updated version of the Note Directory Watcher Patch. This >>> version is now useful. >>> >>> It can detect changes to notes in ~/.tomboy. It CANNOT detect new >>> notes or notes that have been deleted. I will finish that tonight or >>> next week. >> >> Nice work, Mike. I've got some comments for you: >> >> * When updating a note, just use Note.LoadForeignNoteXml, and pass in >> the complete file contents. It is *probably* best to pass in >> ChangeType.OtherDataChanged >> * Recommend using NoteManager.FindByUri instead of doing your own loop >> (it does the same thing, but could be optimized in the future). >> * I'm curious about the 5 second delay. What's the point? To ensure >> that Tomboy "wins" if a note queued to be saved also has its note file >> edited? >> * Shouldn't you filter on *.note? Or are you watching all files >> because of that tmp file stuff that happens when a note is saved? >> * Please use the Logger class instead of Console.WriteLine >> * Some coding standards stuff, like needing spaces between methods and >> open parens, needing to put opening brace of method on its own line, >> etc >> >> Will have to try it out soon. :-) >> >> Sandy >> _______________________________________________ >> Tomboy-list mailing list >> Tomboy-list@... >> http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com >> > Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Everaldo Canuto-5
::
Rate this Message:
Ops,
Sorry :) Everaldo On Mon, Apr 27, 2009 at 12:04 PM, Sandy Armstrong <sanfordarmstrong@...> wrote: > I think Mike said he's work on that this week. :-) > > Sandy > > On Mon, Apr 27, 2009 at 7:47 AM, Everaldo Canuto <ecanuto@...> wrote: >> Hey Mike, >> >> Any chance to get it working for deleted and new notes? How hard is to >> support delete/new ? >> >> Everaldo. >> >> On Mon, Apr 27, 2009 at 11:18 AM, Sandy Armstrong >> <sanfordarmstrong@...> wrote: >>> On Sun, Apr 26, 2009 at 12:36 PM, Michael Fletcher >>> <m.fletcher@...> wrote: >>>> This is an updated version of the Note Directory Watcher Patch. This >>>> version is now useful. >>>> >>>> It can detect changes to notes in ~/.tomboy. It CANNOT detect new >>>> notes or notes that have been deleted. I will finish that tonight or >>>> next week. >>> >>> Nice work, Mike. I've got some comments for you: >>> >>> * When updating a note, just use Note.LoadForeignNoteXml, and pass in >>> the complete file contents. It is *probably* best to pass in >>> ChangeType.OtherDataChanged >>> * Recommend using NoteManager.FindByUri instead of doing your own loop >>> (it does the same thing, but could be optimized in the future). >>> * I'm curious about the 5 second delay. What's the point? To ensure >>> that Tomboy "wins" if a note queued to be saved also has its note file >>> edited? >>> * Shouldn't you filter on *.note? Or are you watching all files >>> because of that tmp file stuff that happens when a note is saved? >>> * Please use the Logger class instead of Console.WriteLine >>> * Some coding standards stuff, like needing spaces between methods and >>> open parens, needing to put opening brace of method on its own line, >>> etc >>> >>> Will have to try it out soon. :-) >>> >>> Sandy >>> _______________________________________________ >>> Tomboy-list mailing list >>> Tomboy-list@... >>> http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com >>> >> > Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Michael Fletcher-3
::
Rate this Message:
No worries.
I have it written it already. I'm going to do the rest of the cleanups over lunch today and then sent to to the list. On Mon, Apr 27, 2009 at 9:20 AM, Everaldo Canuto <ecanuto@...> wrote: > Ops, > > Sorry :) > > Everaldo > > On Mon, Apr 27, 2009 at 12:04 PM, Sandy Armstrong > <sanfordarmstrong@...> wrote: >> I think Mike said he's work on that this week. :-) >> >> Sandy >> >> On Mon, Apr 27, 2009 at 7:47 AM, Everaldo Canuto <ecanuto@...> wrote: >>> Hey Mike, >>> >>> Any chance to get it working for deleted and new notes? How hard is to >>> support delete/new ? >>> >>> Everaldo. >>> >>> On Mon, Apr 27, 2009 at 11:18 AM, Sandy Armstrong >>> <sanfordarmstrong@...> wrote: >>>> On Sun, Apr 26, 2009 at 12:36 PM, Michael Fletcher >>>> <m.fletcher@...> wrote: >>>>> This is an updated version of the Note Directory Watcher Patch. This >>>>> version is now useful. >>>>> >>>>> It can detect changes to notes in ~/.tomboy. It CANNOT detect new >>>>> notes or notes that have been deleted. I will finish that tonight or >>>>> next week. >>>> >>>> Nice work, Mike. I've got some comments for you: >>>> >>>> * When updating a note, just use Note.LoadForeignNoteXml, and pass in >>>> the complete file contents. It is *probably* best to pass in >>>> ChangeType.OtherDataChanged >>>> * Recommend using NoteManager.FindByUri instead of doing your own loop >>>> (it does the same thing, but could be optimized in the future). >>>> * I'm curious about the 5 second delay. What's the point? To ensure >>>> that Tomboy "wins" if a note queued to be saved also has its note file >>>> edited? >>>> * Shouldn't you filter on *.note? Or are you watching all files >>>> because of that tmp file stuff that happens when a note is saved? >>>> * Please use the Logger class instead of Console.WriteLine >>>> * Some coding standards stuff, like needing spaces between methods and >>>> open parens, needing to put opening brace of method on its own line, >>>> etc >>>> >>>> Will have to try it out soon. :-) >>>> >>>> Sandy >>>> _______________________________________________ >>>> Tomboy-list mailing list >>>> Tomboy-list@... >>>> http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com >>>> >>> >> > > Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Michael Fletcher-3
::
Rate this Message:
> * When updating a note, just use Note.LoadForeignNoteXml, and pass in
> the complete file contents. It is *probably* best to pass in > ChangeType.OtherDataChanged > * Recommend using NoteManager.FindByUri instead of doing your own loop > (it does the same thing, but could be optimized in the future). Cool, I was really unsure about the best way to do these things. A big new API :).! > * I'm curious about the 5 second delay. What's the point? A file change is not usually a single operation. For example this is what it looks like when Tomboy writes a note to disk. gedit is similar but with a different file extension. fd = open("{guid}.note.tmp", "rw") write(fd, byte) -> OnChange raised write(fd, byte) -> OnChange raised write(fd, byte) -> OnChange raised write(fd, byte) -> OnChange raised flush(fd, byte) -> OnChange raised (maybey, if there were bytes left) close(fd, byte) -> OnChange raised (this operating system specific, sometimes metadata shows up as OnChange events) delete("{guid}.note.tmp") rename("{guid}.note.tmp", "rw") The pattern of writes, renames, etc is dependent on the program writing the file, the operating system, the size of buffers used, the size of the note being written, FileSystemMonitor backend, the filesystem (or network file system). The best way to too watch for any kind of change and then wait for things to settle down. Once its settled down the file has been completely written. All of this is based on real world experience. The 5s was arbitrary. It could be 10s or 3s. Maybe not smaller that 2s. > To ensure > that Tomboy "wins" if a note queued to be saved also has its note file > edited? No. I compare the new note to the old note "if (data.Text != note.XmlContent) " and see if it changes. Unfortunately this patch notices every change that Tomboy makes but then ignores it. > * Shouldn't you filter on *.note? Or are you watching all files > because of that tmp file stuff that happens when a note is saved? See above. If I filtered on "*.note" all I would see is an OnDeleted and OnRenamed event. > * Please use the Logger class instead of Console.WriteLine > * Some coding standards stuff, like needing spaces between methods and > open parens, needing to put opening brace of method on its own line, > etc > > Will have to try it out soon. :-) > > Sandy > > Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Sandy Armstrong
::
Rate this Message:
On Mon, Apr 27, 2009 at 10:19 AM, Michael Fletcher
<m.fletcher@...> wrote:>> * Shouldn't you filter on *.note? Or are you watching all files >> because of that tmp file stuff that happens when a note is saved? > See above. If I filtered on "*.note" all I would see is an OnDeleted > and OnRenamed event. Okay, then is it alright to filter on *.note and *.note.tmp? Technically, Tomboy should work fine with notes that have non-GUID names, so I'd like to get rid of the "if (note_id.Length != 36)" check and replace it with a file extension check. Working on better not reloading with LoadForeignNoteXml right now. :-) Sandy _______________________________________________ Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
|
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Michael Fletcher-3
::
Rate this Message:
> * A lot of try/catching and error-checking to catch bad note file
> edits (needs more testing) > * Add/Delete notes using proper Tomboy API > * Use LoadForeignNoteXml to update more than just note title and content > * Save DateTimes for Note.Saved events, use them to ignore file > changes that happen within 3 seconds of such an event. I didn't do > this with Note deletion events because they error out pretty > gracefully already. > * Prefix all log statements with "NoteDirectoryWatcher: " > > This will be part of Monday's release, so if you have time to review > that would rock. > > Thanks, > Sandy > > public class NoteDirectoryWatcherApplicationAddin : ApplicationAddin > { >+ // TODO: Use environment variable here? > private static bool VERBOSE_LOGGING = false; Um either way is good. It will spit out a ton of log messages if you set this to true. > + private void OnNoteSaved (Note note) > + { > + note_save_times [note.Id] = DateTime.Now; > + } Do you want to call this HandleNoteSaved? > + // 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 :). > + if (!File.Exists (note_path)) { > + // TODO: Any need to handle a deletion here? > ... snipped ... > + return; > + } Its very unlikely this would happen. And you have handled this in the try-catch below. > + 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. > + 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. _______________________________________________ Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
|
|
Re: WIP Note Directory Watcher Patch (was Watch for Changes Patch)
by Sandy Armstrong
::
Rate this Message:
On Sun, May 3, 2009 at 8:07 AM, Michael Fletcher
<m.fletcher@...> 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 _______________________________________________ Tomboy-list mailing list Tomboy-list@... http://lists.beatniksoftware.com/listinfo.cgi/tomboy-list-beatniksoftware.com |
| Free embeddable forum powered by Nabble | Forum Help |
