GNOME Bugzilla – Bug 509147
Renaming template notes can cause problems
Last modified: 2011-08-22 22:00:00 UTC
While the problems aren't really that serious, renaming a template note can cause unexpected behavior. Background: There are two kinds of template notes that exist in Tomboy: 1) the default template note, 2) a notebook's template note. Template notes are marked with a special system tag (I think it's "system:template"). This tag is used by the UI to prevent template notes from appearing in the recently changed menu, the search UI, etc. Additionally, notebook template notes are marked with a "system:notebook:<notebook name>" system tag. Additionally (and I think this could be the real problem), template notes are searched for by name. NoteManager returns the template note by searching for the note named, "New Note Template". Notebook template notes are searched for by looking for a note named, "<Notebook Name> Template Note". Problem: Since template notes are really just standard Tomboy notes, the UI does not prevent the user from changing the title of the note. In one case, the user may actually WANT to change the template note's title thinking that it will adjust the titles of all the new notes created with the template (and this might be something we want to provide in the future). Unfortunately, this isn't how it currently works. When you change a template note's title, the title will change (since it's just a standard note), but the special "system:template" and "system:notebook:<notebook name>" tags are not removed. Since we prevent template notes from appearing in the search UI list, as soon as a user closes the note, they'll never be able to get it to re-appear. They'll have an extra *.note file on their system which they can't access. Workaround: The user can create a new template note by right-clicking the notebook/"All Notes" item and selecting "Open Template Note." This will recreate a new template note with the proper tags and title. Possible solution: We ought to search for template notes based on the tags instead of the name. For example, we'd look for a notebook's template note by looking for the note that contains both the "system:template" and "system:notebook:<notebook name>" tag. For the default template note, we'd look for the note that only has "system:template" and no corresponding "system:notebook:<notebook name>" tag. Any other suggestions/ideas?
You mentioned that the user might want to modify the title. Maybe we should support that. We were already talking about changing the default title to be a datetime stamp. Maybe the default title for a new notebook note should be the the template note title (assuming it's not the default title) with a datetime stamp appended? As for your actual problem...originally I liked the idea that a template was just a note with a particular title. This is very simple, and consistent in terms of support. However, as we've seen, it's not very discoverable, and so we've ended up adding preferences and menu items and all that because it's necessary to get the feature to the user. Given that, and also that we're already treating the note differently (in the search UI), and also the possibility of letting the user muck with the default title...I think you're right, and this should be handled by system tags. I think your solution is simple and elegant.
Setting the default assignee and QA Contact to "tomboy-maint@gnome.bugs".
Created attachment 165239 [details] [review] Create new notes based on template title Let me know if I'm on the right track here. To get the template note, find the note that * has the tag "system:template" * has the tag "system:notebook:<notebook name>" (in case of a notebook template) * has no notebook tags (in the default case) Make the title unique Note creation will just append id++ to the template title where id is the total number of notes in existence (or in the specific notebook). Future Considerations We currently return the first note found matching the above criteria. I could see a user wanting a list of template notes to choose from, so the criteria above is not a unique note and works fine for now. Caveats One thing we lose here is the highlighting of body in the default template. Since the template will always exist, we'll never highlight the body (except the first time the default template is created). This can be handled well if we implement enhancement 527183 Also, given this bug, there are already users who might have several hidden templates. Since we'll return the first one, they might get an old hidden template note. We should have some way to cleanup the hidden notes before pushing this patch
Created attachment 165558 [details] [review] Create new notes based on template title Realized I wasn't creating notebook notes quite right.
Review of attachment 165239 [details] [review]: On the whole this looks good, but I don't think this bug is the place for new or changed features. ::: Tomboy/NoteManager.cs @@ +495,2 @@ + if (String.IsNullOrEmpty(title)) + title = GetUniqueName (template_note.Title, notes.Count); Is this change necessary in any way to fix this bug? I see it as a new feature that I'd rather have implemented separately. Besides, it would result in all new notes having names like "New Note Template 123", which doesn't make any sense. Please s/template_Note.Title/Catalog.GetString("New Note") @@ +500,2 @@ string xml_content = + template_note.XmlContent.Replace (XmlEncoder.Encode (template_note.Title), In this Replace call, you should use the local title variable, as it may have changed. @@ +512,2 @@ Note new_note = CreateNewNote (title, content, guid); I don't understand why you had to get rid of the initial selection code here? @@ +560,3 @@ + foreach (Note note in template_tag.Notes) { + template_note = note; + foreach (Tag tag in note.Tags) { A simpler check would be NotebookManager.GetNotebookFromNote (note) == null ::: Tomboy/Notebooks/Notebook.cs @@ +148,3 @@ } + public Note CreateNotebookNote () Thanks for adding this method to eliminate ugly code duplication in random places.
*** Bug 575338 has been marked as a duplicate of this bug. ***
By the way, I'm working on some new template features in a branch here: http://git.gnome.org/browse/tomboy/log/?h=better-templates It includes: 1) a big bar that informs the user that the note is a template note, and explains a bit about what that means 2) a button for un-templating the note More features planned are: 3) ability to customize new note titles 4) option to save default text selection 5) option to save default window size
I moved my branch elsewhere so I could rebase it and force push non-fast-forward stuff: http://gitorious.org/~sandy/tomboy/sandys-tomboy/commits/better-templates
Cool, thanks Sandy I'll take a look at your comments. What are your plans for the new note titles? I am actually working on an addin with that very task in mind. My idea was the user could drop "tokens" in the title or body which would expand to values when the note is created. For example you could drop a date token which would expand to today's date when a note is created based on that template. I implemented it with dynamic note tags and ran into problems when trying to drop tags in the title. I'll take a look at your branch.
Review of attachment 165239 [details] [review]: ::: Tomboy/NoteManager.cs @@ +495,2 @@ + if (String.IsNullOrEmpty(title)) + title = GetUniqueName (template_note.Title, notes.Count); Fair enough. I will change this so the notes are based on "New Note". I'll open a new bug for the feature of creating new note titles based on the template title. Then we can change the default template title so it makes more sense as a note title. @@ +500,2 @@ string xml_content = + template_note.XmlContent.Replace (XmlEncoder.Encode (template_note.Title), Hmm, I don't follow. Here we want to replace the existing template title (template_note.Title) with our new title (local variable). Are you refering to NoteTemplateTitle? This is string never changes but the template_note.Title could have changed which is why we would want to use it. I'd like to rename "NoteTemplateTitle" to "DefaultTemplateTitle" since it would only be used on template creation. @@ +512,2 @@ Note new_note = CreateNewNote (title, content, guid); Hmm, I'm not completely sure why I removed this. In this code path, the body is being passed in as an argument and I felt the desire was not to edit this text. "Describe your note here..." to me says this text should be replaced, but content from anywhere else (template, or user action) is something you mostly would want to keep. I'll add the code back. @@ +560,3 @@ + foreach (Note note in template_tag.Notes) { + template_note = note; + foreach (Tag tag in note.Tags) { Haha, yes, I'm still getting used to the code. Thanks!
Created attachment 167302 [details] [review] Don't lose template notes on rename Updated from code review comments. Thanks!
Added bug 626296 to address the feature enhancement of creating note titles based on template title.
Corresponding Gnote Bug 627539. I also ported the provided patch, it looks OK to me.
I looked at the ported version of patch one more time and pushed it. It seems to work well in Gnote.
Thanks! I'll commit once a Tomboy dev gives the OK (I think that's the policy?).
bump: bug as been reviewed by Aurimas Černius
There is a small problem with this patch - the initial text for new note ("Describe here...") is not selected when creating new note. This was the problem for new notes in notebooks, now the same happens for new notes without notebook. I have just updated Gnote patches for this problem and am going to push them in few days. See Bug 639175.
Created attachment 185575 [details] [review] Allow renaming of template notes without losing them I've redone this patch to focus on the bug. Additionally cleanup around creating notes using templates will be done later in the milestone.
Review of attachment 185575 [details] [review]: Not sure about this yet. ::: Tomboy/Note.cs @@ +674,3 @@ public bool ContainsTag (Tag tag) { + if (tag != null && data.Data.Tags.ContainsKey (tag.NormalizedName) == true) "== true" == yuck...just delete that part while you're here ::: Tomboy/NoteManager.cs @@ +585,3 @@ + Tag template_tag = TagManager.GetOrCreateSystemTag (TagManager.TemplateNoteSystemTag); + foreach (Note note in template_tag.Notes) { + if (Notebooks.NotebookManager.GetNotebookFromNote (note) == null) { I don't get this logic. Why are we checking for null here? ::: Tomboy/RecentChanges.cs @@ +1304,3 @@ if (templateNote != null) { // Use the body from the template note + string xmlContent = templateNote.XmlContent.Replace (XmlEncoder.Encode (templateNote.Title), Duplication is a code smell. Maybe this needs to go somewhere common.
(In reply to comment #21) > Review of attachment 185575 [details] [review]: > > Not sure about this yet. > > ::: Tomboy/Note.cs > @@ +674,3 @@ > public bool ContainsTag (Tag tag) > { > + if (tag != null && data.Data.Tags.ContainsKey (tag.NormalizedName) > == true) > > "== true" == yuck...just delete that part while you're here Will do. > ::: Tomboy/NoteManager.cs > @@ +585,3 @@ > + Tag template_tag = TagManager.GetOrCreateSystemTag > (TagManager.TemplateNoteSystemTag); > + foreach (Note note in template_tag.Notes) { > + if (Notebooks.NotebookManager.GetNotebookFromNote (note) == > null) { > > I don't get this logic. Why are we checking for null here? This was your suggestion :P we're looking for the default note template, which will have the template note tag and will belong to exaclty zero notebooks... which means that GetNotebookFromNote() == null. > ::: Tomboy/RecentChanges.cs > @@ +1304,3 @@ > if (templateNote != null) { > // Use the body from the template note > + string xmlContent = templateNote.XmlContent.Replace > (XmlEncoder.Encode (templateNote.Title), > > Duplication is a code smell. Maybe this needs to go somewhere common. Agreed, this gets cleaned up in another patch. Just trying to focus on the bug for this one.
Created attachment 194312 [details] [review] Allow note template renaming An updated patch, added comments where appropriate.
Review of attachment 194312 [details] [review]: Tested and works. I used these three patches 0001-sandys-base-template-bar.patch 7e8f42d792cc6e3c42ac861ec877fc6d 0001-Allow-renaming-of-template-notes.patch 672266c147f758f9660393be51d188e9 template-bar.patch fd339108c66f368ba3eac53f42dab47e
Comment on attachment 194312 [details] [review] Allow note template renaming Commited as faa29b032abb4b8ffd330dfcf76d42678812078c This will be included in Tomboy 1.7.4 Thanks for the review!