Created attachment 86528 [details] [review]
Simple clean-up plugin

This patch adds a CleanBrokenLinksPlugin which adds a "Clean up broken links" into the Tools menu of a note.  The only drawback of this plugin (besides not being very clean placement of a default system plugin), is that it only works on one note at a time.  This might be desirable though.

Is it too ugly to have a UI like this:

Tools
-Clean Broken Links
--Just for this note
--For all notes

Personally I'd only find it irritating if I were going through a lot of notes and selecting "Clean Broken Links -> Just for this note".  But that's probably an edge case, since most people cleaning a lot of notes would want to select "Clean Broken Links -> For all notes".

I think that might be a valid way of implementing this.  The other thing we may want to introduce is a Tools menu in the search window so that you could operate on all of the filtered notes or selected notes.  This would tie in well with tags; filter down the list of notes and select something like "Create Single Note from Selected Notes" or "Print Selected Notes as one" etc.

Wow, that's a really cool idea Boyd.  I think it also introduces a new "extension point" for Tomboy (in the lingo of Mono.Addins).  We (and others) could write plugins that operate on a filtered set of notes instead of on individual notes.

*** Bug 430078 has been marked as a duplicate of this bug. ***

I think this is a good gnome-love candidate, and am marking it with that keyword.  I know it's something I'd like to see fixed, and it should be easier now with Mono.Addins.

I don't believe this needs a plugin or additional entries in the menus, but some usability design considerations:

1- "Link" (the action after clicking the button) should be undoable by a simple Ctrl-Z

2- Selecting a text with a link should turn the "Link" button into "Unlink" (or similar).

3- Deleting the note created by clicking on the link button should delete the link.

4- If there's still a broken link, some automatic mechanism should be added to help detect it. Broken links should simply not exist. I understand external links are out of scope, but links to other Tomboy notes can be checked and if they're broken they should be deleted. (This would be unnecessary if #3 is properly done, but would help when upgrading from an older version of Tomboy.)

(In reply to comment #7)

Aleve, I think I agree with numbers 3 and 4 here (not as sure about 1 and 2, though).

Does anybody find broken links to be valuable?

Point 1 and 2 are just expected behaviour from any application, not just Tomboy. It's a "desktop coherence" issue. A non-destructive action should be reversible (I'm pretty sure there's some rule like this in Gnome's HIG. If not, there should be :-))

I understand this situation might occur, though:

- You select the word "new", click on "Link" and a new note appears.

- You undo your action (or reselect the word "new" and click on "Unlink") and now you have a note which is not linked to any other note. An "orphan" note, so to speak.

- Now you select the word "new" again and click on "Link".

What should be the behaviour now? Well, I guess the word "new" should link to the second note and the first note should stay orphan. But I agree this might be the subject of further discussion.

In response to your question: I can't imagine the value of a broken link.

(In reply to comment #8)
> Does anybody find broken links to be valuable?

Broken links are Wiki-style. They mean that there was a note with that name or the note has not been written yet. Broken links are also necessary for the Highlight WikiWords feature.

Then I guess a decision must be made: is this a Wiki or a note taking software?

There's an option "Highlight WikiWords". If this option is not checked (which is the default AFAIK), just remove broken hyperlinks automatically. In the other case, don't remove them.

Setting the default assignee and QA Contact to "tomboy-maint@gnome.bugs".

(In reply to comment #7)
> 2- Selecting a text with a link should turn the "Link" button into "Unlink" (or
> similar).

I'm puzzled as to why this feature wasn't in Tomboy from the beginning. If you can linkify something you should be able to unlink it, not just referentially but cosmetically as well. This is possible in HTML and several flavors of wiki markup I've used, so why not in Tomboy?

In my case, I have lots of notes written for instructive purposes which contain filesystem paths and these paths aren't meant to point or link to actual locations in the filesystem of the machine I use Tomboy on. For instance, when I have the following fstab line in a note documenting server setups:

/dev/vg0/portage   /usr/portage   reiserfs   user_xattr   0 2

then I don't want "/dev/vg0/portage" and "/usr/portage" to be hyperlinks because the targets they refer to don't exist on my Tomboy system and never will. I have dozens of such unwanted links (not all are pathnames), and since there's no way to unlink them they clutter up my notes and sometimes get accidentally clicked. Very frustrating.

I'd like to be able to right-click on any link and select "Unlink", or select the linked text and click the Link button in the toolbar to unlink it. Either way, the source XML will probably require a new tag like <unlink></unlink> so that the rendering code will know for sure not to linkify the specified content. This should also allow a user to selectively prevent WikiWords from being linkified, even when the Highlight WikiWords option is set.

I am also very distracted with every pathnames being links as described in #14. 

Moreover I think that even if we were talking only about links to another notes even then it is very hard to automatically decide whether the word should or should not be a link because it always depends on the context in which the word is used. 
So we need Unlink button to remove links which were wrongly created because tomboy did not understood the context in which the word was used.

I think that autolink is very useful. Just sometimes it needs a little help from the user.

Created attachment 140395 [details] [review]
Add contextual menu items for opening/creating/deleting note links (BGO #384034)

Not complete...doesn't differentiate between regular broken links and
WikiWord broken links. Also needs menu items in Tools to operate on the
entire note, and possibly on the entire note collection.

Was going to try to get this in before UI freeze, but didn't quite make it.  That patch is work-in-progress, but I think it's a nice way to handle broken links on a link-by-link basis.

Just noticed an obvious problem in the patch...I added an item for Create Linked Note, but there's already in item in the popup menu for that.

And a problem with Boyd's patch is that it, too, affects WikiWords indiscriminately from other broken links.  Marking both as "needs-work"...would like to just merge them together and fix the few remaining issues.

Created attachment 173825 [details] [review]
Proposed fix - new RemoveBrokenLinks addin

Introduces an addin to Tomboy that is able to remove all broken links from a note (Tools menu). If "Highlight WikiWords" is enabled in preferences - WikiWord broken links are respected and not removed. Tested on Win7 & OpenSUSE 11.3.

Next thing would probably be to enhance it to work on Search results as well, not only on a single note.

(In reply to comment #18)
> Created an attachment (id=173825) [details] [review]
> Proposed fix - new RemoveBrokenLinks addin

Yay, thanks Alex!  It looks good to me, I look forward to trying it out.

If anyone subscribed to this bug would like a binary of this add-in to try out, just let me or Alex know.

> Introduces an addin to Tomboy that is able to remove all broken links from a
> note (Tools menu). If "Highlight WikiWords" is enabled in preferences -
> WikiWord broken links are respected and not removed. Tested on Win7 & OpenSUSE
> 11.3.

As you say in the comments, the wikiword handling could be improved if we refactored some stuff in Tomboy.

> Next thing would probably be to enhance it to work on Search results as well,
> not only on a single note.

Yeah.  You should be able to operate on search results (which, by default, is all notes), or an arbitrary selection of notes.  Should be able to make an ApplicationAddin class that puts that in the Tools menu of the Search window.

I just accidentally made a note called "Tomboy", which as you might guess has caused quite a broken link problem for me.

I tried out the add-in and it worked very nicely.  I'm not sure about the Ctrl+R keybinding, though.  Will have to mull that over.

Let me know if you're planning on adding multiple note support soon.  If not I may have to do it this weekend to get my note collection in order.

I think we should ship this add-in until we have a community add-ins project like Banshee has, and then move it there.  Arguments for not shipping it are welcome.

(In reply to comment #20)

> I tried out the add-in and it worked very nicely.  I'm not sure about the
> Ctrl+R keybinding, though.  Will have to mull that over.

Sure, that was just the first one that poped up in my mind (Ctrl+Remove), any other ideas are welcome (e.g. if I missed any generic approach to hotkeys in Tomboy/Gnome, etc).

> Let me know if you're planning on adding multiple note support soon.  If not I
> may have to do it this weekend to get my note collection in order.

Yes, that was something I wanted to tackle exactly this weekend.

> I think we should ship this add-in until we have a community add-ins project
> like Banshee has, and then move it there.  Arguments for not shipping it are
> welcome.

Cool, glad to read that :)

Need an advice - tried to create an ApplicationAddin as Sandy suggested,but it looks like to be able to add new items to the Search Window menu I need to modify some of the Tomboy's code (e.g. ActionManager.cs), which is not something a plugin is expected to need.

Also I'm not sure how to use the Search results - they seem to be pretty much contained within the RecentChanges.cs from the visibility prospective.

I guess that is actually possible and the above comes from my [yet] very basic knowledge of Tomboy's internals, any advice here on what the basic flow would be? 

I've studied the code for NotD and Sync plugins (ApplicationPlugin in their roots) and the former is rather straightforward but it doesn't do many interactions with other parts of the app. And the former does that, but some of its parts or "helpers" are heavily integrated into Tomboy's other files, i.e. something like my first point above says.

---Alex

(In reply to comment #22)
> Need an advice - tried to create an ApplicationAddin as Sandy suggested,but it
> looks like to be able to add new items to the Search Window menu I need to
> modify some of the Tomboy's code (e.g. ActionManager.cs), which is not
> something a plugin is expected to need.

The sync addin adds a menu item to the tools menu with AddUiFromString and a menu xml (SyncManager.cs:169). Maybe that will work for you too.

(In reply to comment #23)

Thanks Stefan, while that was exactly one of the places I already crawled through studying the code, your suggestion made me look at it from a bit different angle & revisit a couple of things &  I got what was needed there :)

2 all:
I've got the next version that is able to work on Search result sets, the patch is attached (the initial patch is assumed to be applied) - your comments are more than welcome and please let me know if you want a binary.

Created attachment 173969 [details] [review]
Refactored RemoveBrokenLinks & added capability to work on Search results

I had to add one field to Tomboy's own NoteRecentChanges class which does all the Search job, to be able to get the Search results from the outside.

Also, as long as there are now two Addin classes used (Note- and Application-) I refactored the code and moved common items to a separate class.

Comments are welcome.

So shouldn't we mark this bug as closed now that we have a fix?

(In reply to comment #26)
> So shouldn't we mark this bug as closed now that we have a fix?

No, it will be closed when we have a fix in Tomboy git, or have decided to definitely not fix in Tomboy git.  Things have been hectic lately and I haven't gotten to finish reviewing your add-in.  But I will be looking at this soon, thanks for staying on top of it!

> But I will be looking at this soon,
> thanks for staying on top of it!

Maybe it makes sense for me to create a separate project for this plugin on e.g. Sourceforge and you'd just close this bug with a reference to it? This would probably remove the formal need for you to review it :)

*** Bug 639888 has been marked as a duplicate of this bug. ***

Any chances for this being reviewed?

Though it's now available as a project on SF I think its supportability will be higher if included into base set, plus the functionality is rather foundational (IMO).

Review of attachment 173969 [details] [review]:

Cool! Tested it out and it removed my broken links. However, there are some issues you need to address.

Triggering the multi-note cleanup will update every note regardless of whether or not it contained a broken link. This causes every note to save and Last Modified time to be updated which is not desired. It's really Note.RemoveTag's fault since it isn't smart enough to do anything different whether or not it actually found a tag to remove. I can't comment on whether or not that's a bug with Note.RemoveTag.

I ran it on all my notes (~ 250) and it took over 20 seconds to complete, during which Tomboy was frozen and not usable. Avoiding the save on every note would at least reduce the duration.

::: Tomboy/Addins/RemoveBrokenLinks/RemoveBrokenLinksApplicationAddin.cs
@@ +67,3 @@
+		public override void Shutdown ()
+		{
+			initialized = false;

You also need to remove the menu entries here, otherwise when the addin is disabled, they still remain (and work).

::: Tomboy/Addins/RemoveBrokenLinks/RemoveBrokenLinksUtils.cs
@@ +25,3 @@
+			Gtk.TextIter note_start, note_end;
+			
+			Logger.Debug ("About to remove broken links from a note: " + note.FilePath);

Logging the Note.Title is preferred. And to be complete, I'd rather it only log when a link was actually removed with the text of the link.

@@ +45,3 @@
+			// It turns WikiWords back into broken links after sweeping all broken links,
+			// but only in case WikiWords are enabled.
+			// Most probably there's more elegant way of doing this.

Since wikiwords is part of the core app and not an addin, maybe it makes sense to expose some functionality so that we can have a "more elegant" solution here.

::: Tomboy/RecentChanges.cs
@@ +45,3 @@
+		/// Stores notes set that is a result of a search & currently shown to user
+		/// </summary>
+		public List<Note> CurrentMatches;

How about using a method GetFilteredNotes() which returns this list instead. In the method, you should be using the sort_filter and just pull all the notes out of it and populate the List.

Otherwise, you're really just duplicating what the GtkTreeModelFilter is doing, which we want to avoid.

bump: comment 20 suggests pushing the single-note version of the addin until the multi-note version is polished.

How do I remove myself from this bug? I can't find a way to do it.

Disregard what I said about Note.RemoveTag, it was wrong on several levels.

@Aleve: you can edit the CC list below the comment box.  I am removing you now.

@Aaron: good point on the last-modified-time stuff.  Removing dead links should (arguably) not impact the user-visible last-modified-time, or affect the sort order in the Search window or tray menu.  Probably better to use SaveType.OtherDataChanged (or whatever the proper option is, I don't have the code up in front of me).

So is there a solution to this "bug" or a workaround of this lack of feature in my opinion?

I tried the RemoveBrokenLinks v0.2 add-in but it doesn't work.

If I am in search all notes and select the note with broken links then I go to Tools->Remove broken links Tomboy crashes.

If I open the note then go click on "Use tools in this note" button and select remove broken links nothing happens.

I'll take a look at it. What are your Tomboy and OS versions?

Tomboy
Version 1.6.0

OS

Linux Mint 11 "Katya"

uname -a
Linux 2.6.38-8-generic #42-Ubuntu SMP Mon Apr 11 03:31:50 UTC 2011 i686 i686 i386 GNU/Linux

Created attachment 189262 [details]
Compiled DLL for robertdq - rebased RemoveBrokenLinks on top of 1.7.0 

robertdq - this is a quick'n'dirty rebase of the proposed patches on top of the current version (though it's 1.7.0, not 1.6.0 as you have). Checked to work for me on Win, both in note and Search results. Please check it out. I'm a bit short on time to implement deeper changes as discussed in the bug, but still have this on my list. If it still doesn't work for you - I'll try to carve out some time and fix it.

Review of attachment 173825 [details] [review]:

Thanks for the patch, due to the interest we'll take the NoteAddin now for 1.7.0. Glad to see you're still working on the ApplicationAddin part.

commit 768b218ac38c35bee9d539a2e712e01eaad01cfb

I'm almost done with all the amendments, the last point left is that note "last modified" time change upon tag removal.

AFAICS eventually, the Buffer.RemoveTag causes the event handler in Note.cs to fire up and it has the following inside

void BufferTagRemoved (object sender, Gtk.TagRemovedArgs args)
{
    if (NoteTagTable.TagIsSerializable (args.Tag)) {
    DebugSave ("BufferTagRemoved queueing save: {0}", args.Tag.Name);
    QueueSave (ChangeType.ContentChanged);
    }
}

That "ContentChanged" causes Note's ChangeDate update. But in the ChangeDate definition, it says "Does not include tag/notebook changes (see MetadataChangeDate)" - which suggests that ContentChanged should be replaced with OtherDataChanged in that handler, just like Sandy suggested.

The only thing I think of is that tags define the formatting and therefore to some extent may be treated as the note contents, not metadata.

Thoughts?

Better late than never :) I still want to have this implemented and still would like to get your comments to my last question.

I'm in prgress transferring my previous work on this from one machine to another, so I can't check whether there were any major changes that would affect the soltuion, but quick look through the git log doesn't reveal anything like that, so my guess is that the question is still valid.

BTW, can I maybe get this bug assigned to me to make it easier to identify the owner?

Created attachment 210557 [details] [review]
RemoveBrokenLinks with an ability to work on search results - corrected after Aaron's comments+rebased for current version

Here's what I came up with after implementing Aaron's comments. Does the job for me. Still there's that question about last modified time, but otherwise it works as intended. Comments are more than welcome.

Review of attachment 210557 [details] [review]:

The patch looks fine to me. Too bad that we have to make changes to RecentChanges.cs, but nothing comes to mind as another way around.

Any thoughts from Aaron?

Review of attachment 210557 [details] [review]:

committed r5927d38b863e