Community
Participate
Working Groups
TaskHyperlinkDetector currently only detects hyperlinks correctly for regions of 0-length. This is due to the API of java class AbstractRepositoryConnectorUi: @public IHyperlink[] findHyperlinks(TaskRepository repository, String text, int textOffset, int lineOffset)@ Note that the method takes a textOffset, with no length. The implementation of this method in BugzillaConnectorUi only returns hyperlinks that overlap the provided @textOffset@. This blocks bug 167941, which should be able to reuse the TaskHyperlinkDetector to find TaskHyperlinks. It also fails to fulfill the contract specified by IHyperlinkDetector, which takes a region as an argument.
Created attachment 110251 [details] patch that adds new API for detection of hyperlinks covering a region This patch adds new API for detection of hyperlinks in a region of text. The old findHyperlinks API is deprecated, and the new API calls the old one so that subclasses still work. The new API is implemented in BugzillaConnectorUi With this patch applied patch on bug 167941 works
Created attachment 110252 [details] mylyn/context/zip
I agree that the current hyperlink detection API is not generic enough to be reused to resolve bug 167941. I am a bit concerned about the performance impact of the changes proposed in the patch though. Invoking document.get() creates a copy of the whole document each time hyperlink detection is triggered. Considering that task hyperlinking works in any textual editor this could potentially lead to significant temporary memory allocation for large documents. There are also performance considerations when matching regular expressions on the whole document. This could be slow for complex regular expressions (e.g. JIRA) or large documents. I'm not sure what the best abstraction for the hyperlink detection is. The intention of the current API is to avoid code duplication between connectors but I find the method signature difficult to understand and to implement. Maybe it would be better to encapsulate the common code in a utility method and change the API to resemble IHyperlinkDetector more closely?
Created attachment 110354 [details] re-cut the patch avoiding document.get() I understand your performance concerns. Here's a re-do of the patch that avoids the no-arg document.get(). See TaskHyperlinkDetector for changes
Created attachment 110355 [details] mylyn/context/zip
Great! I'll merge the patch once 3.0.2 is released. We'll also need to implement a few test cases for finding multiple hyperlinks for the various connectors.
(In reply to comment #6) > Great! I'll merge the patch once 3.0.2 is released. We'll also need to implement > a few test cases for finding multiple hyperlinks for the various connectors. Agreed regarding test cases. The attached patch has no test cases, and only alters the BugzillaConnectorUi implementation: I left Trac, JIRA and all of the others alone. Let me know if you'll take care of these, otherwise I'll try to set aside some time.
Thanks for offering help with the tests. I'll have a better sense of the priorities once we take a pass at the next release planning. I think we can hold off on this for now.
*** Bug 182839 has been marked as a duplicate of this bug. ***
Created attachment 113631 [details] updated patch
Patch applied and updated BugzillaTaskHyperlinkTest. Before resolving this bug we'll need more tests for Bugzilla. I have created subtasks to track the implementation for Trac, XPlanner and JIRA.
thanks for getting this one in!
During testing I noticed a problem where the hyperlink detector will return hyperlinks that don't match the region in case the region length is 0. I think we need to validate the hyperlinks returned by connectors against the originial region. I'll try to create a test case.
Added testMatchMultipleEmptyRegion() to BugzillaTaskHyperlinkDetectorTest.
I have committed a fix for the failing test but still need to add additional tests.
While reviewing the new API I decided to revert the changes to AbstractRepositoryConnectorUi and to use the existing method for detecting hyperlinks instead of adding an additional method. The main reason being that I found it impossible to specify the new method in a way that provided full backwards compatibility while maintaining a simple signature. The default implementation of the proposed findHyperlinks() method would always pass 0 as the index into text when invoking the existing implementation of findHyperlinks(). This would cause most existing implementations to return an empty list (unless the hyperlink was actually at the beginning of the text). Clients are now expected to return all matching hyperlinks when the passed offset into text is -1. It is likely that existing implementations will return an empty list in that case but that is acceptable as detecting multiple hyperlinks will require changes to existing implementations at any rate.
