Community
Participate
Working Groups
Changes in bug 164221 provide recognition of patterns like bug#164221 comment#24. Bugzilla also recognizes the pattern "comment #24" making a link to the comment in current task. Mylyn does not seem to understand that yet. Perhaps a shortcut like bug 164221#24 would be nice to have too.
Created attachment 161975 [details] patch V1 to get this work I must change TaskMarkupSourceViewerConfiguration and RepositoryTextViewerConfiguration to store an AbstractTask that can we pass to findHyperlinks Please review!
Created attachment 161976 [details] mylyn/context/zip
I'll need some time to review this patch and won't get to that before EclipseCon. We generally can't remove existing API and only add new things so you probably need an additional method in AbstractRepositoryConnectorUi to find hyperlinks and to create rich editor extensions instead of changing the existing one.
(In reply to comment #3) > I'll need some time to review this patch and won't get to that before > EclipseCon. We generally can't remove existing API and only add new things so > you probably need an additional method in AbstractRepositoryConnectorUi to find > hyperlinks and to create rich editor extensions instead of changing the > existing one. Should I change the patch before review?
(In reply to comment #4) > Should I change the patch before review? Yes, that would be helpful.
Created attachment 163739 [details] updated Patch Steffen, here is a new version. Hope that this is what you me to do.
Created attachment 163740 [details] mylyn/context/zip
Created attachment 163742 [details] patch V2 Sorry the previous patch was not complete. Here the corrected version
Created attachment 163743 [details] mylyn/context/zip
Thanks Frank. A couple of thoughts from a quick review of the non Buzilla specific parts: * Keep the existing constructors in RichTextEditor and simply delegate to the new ones passing null for the task. This is to support backwards compatibility. Even though it's not API there might be clients reuing RichTextEditor. * The new method in AbstractRepositoryConnectorUi should be called findHyperlinks for consistency and delegate to the existing findHyperlinks method. There shouldn't be a need for a supportGetHyperlinks method. All client code should always invoke the new method. * Use a similar pattern for MarkupTaskEditorExtension, i.e. add new methods as needed and make existing methods delegate to the new methods. If you can create a new patch this week we should be able to get this into 3.4.
Created attachment 169262 [details] patch V3 (In reply to comment #10) > Thanks Frank. A couple of thoughts from a quick review of the non Buzilla > specific parts: > > * Keep the existing constructors in RichTextEditor and simply delegate to the > new ones passing null for the task. This is to support backwards compatibility. > Even though it's not API there might be clients reuing RichTextEditor. OK > * The new method in AbstractRepositoryConnectorUi should be called > findHyperlinks for consistency and delegate to the existing findHyperlinks > method. There shouldn't be a need for a supportGetHyperlinks method. All client > code should always invoke the new method. OK > * Use a similar pattern for MarkupTaskEditorExtension, i.e. add new methods as > needed and make existing methods delegate to the new methods. Sorry I get problems while doing this! So this patch did not include a fix for this. Should I look into this tonight? > > If you can create a new patch this week we should be able to get this into 3.4.
Created attachment 169263 [details] mylyn/context/zip
> > * Use a similar pattern for MarkupTaskEditorExtension, i.e. add new methods as > > needed and make existing methods delegate to the new methods. > Sorry I get problems while doing this! > So this patch did not include a fix for this. > Should I look into this tonight? > > > > If you can create a new patch this week we should be able to get this into > 3.4. Yes, that would be great. Another thing that should be changed is the signature for findHyperlinks(). It's important to uses ITask instead of AbstractTask here since it's API. Also in the documentation it should say that task maybe null. Once you have posted the new patch I'll review and apply it.
Created attachment 169589 [details] patch V4 (In reply to comment #13) > Yes, that would be great. Another thing that should be changed is the signature > for findHyperlinks(). It's important to uses ITask instead of AbstractTask here > since it's API. Also in the documentation it should say that task maybe null. > > Once you have posted the new patch I'll review and apply it.
Created attachment 169590 [details] mylyn/context/zip
Created attachment 169597 [details] patch v5
Created attachment 169598 [details] bugzilla changes
Thanks Frank! I have committed the framework specific part of the patch with a minor modifications to minimize the amount of API changes and to allow for future extensibility. The remaining changes that are not yet committed are in the last patch. David, the changes affected the task editor extension in WikiText which now gets passed a context for the hyperlink detector. It would be great if you could review the changes to MarkupTaskEditorExtension and let me know if there are any concerns. Rob, please do a quick check of the Bugzilla changes (last patch) and post if it's okay for Frank to commit.
> Rob, please do a quick check of the Bugzilla changes (last patch) and post if > it's okay for Frank to commit. Clicking links like "comment #12" in the comment *viewers* results in an error dialog stating that Task Data cannot be found.
Bugzilla's bits committed.
(In reply to comment #19) > > Rob, please do a quick check of the Bugzilla changes (last patch) and post if > > it's okay for Frank to commit. > > Clicking links like "comment #12" in the comment *viewers* results in an error > dialog stating that Task Data cannot be found. Good catch. The context argument didn't get passed down to the comment viewers if WikiText was used. Should be fixed in head now. Looks like we are done here. Great work Frank!
(In reply to comment #18) > David, the changes affected the task editor extension in WikiText which now gets > passed a context for the hyperlink detector. It would be great if you could > review the changes to MarkupTaskEditorExtension and let me know if there are any > concerns. Thanks Steffen, the changes look fine.
Reopening. Shawn discovered that certain patterns can take an extremely long time to parse.
It turns out that it is sufficient to enter "bug 123[20 whitespace characters]bug 456" in a task editor to reproduce the problem. I suspect that this was due to the way the regular expression was constructed with every part of the expression being optional and some parts reading whitespaces in a greedy way. I have rewritten the expression to resemble the hyperlinking behavior of Bugzilla and created a new test class with minimal coverage since the old one is very difficult to debug. Some of the old tests are failing now, some failures are due to false assumptions others possibly due to regressions. Rob, Frank, please review the attached patch and let me now how to proceed with the failing tests. I have committed the patch since the UI lock ups were a blocker for the RC2 release.
Created attachment 169926 [details] fix for UI lock up
Created attachment 169927 [details] mylyn/context/zip
Bug patterns: bug 16 bug#16 bug #16 bug# 16 bug # 16 bug123 Comment patterns: comment 1 comment#1 bug 16 comment 1 bug 16 # comment 1 bug 16 comment # 1 Attachment patterns: Attachment #61 [details] Attachment 61 [details] Multiple patterns: bug 16#comment#1 bug 16 attachment #61 [details] Invalid comments patterns: c 1 c#1 #c1 comment #c1 bug 16#c1 bug 16#1 bug#16#c1 bug#16#1 bug#16c#1
With the changes, the lockup seems fixed.
Created attachment 170100 [details] fix for failing junit tests I fixed the following BugzillaHyperlinkDetectorTest 1) testFindHyperlinksDuplicateOf: wrong size of link 2) testFindHyperlinksNoAttachment: assertHyperlinks("Create attachment 123 [details]"); is an legal link so I remove it. BugzillaTaskHyperlinkDetectorTest 1) the char 'c' for comment is not an legal pattern so remove the tests 2) testDuplicate: 'duplicate of' is now part of the pattern so we have an new offset 3) testAttachmentNew: 'Created attachment' is now part of the pattern so we have an new offset 4) testAttachmentOld: 'Created an attachment (id=' is now part of the pattern so we have an new offset 5) testCommentLotsOfWhitespace: in this case we have two links so we need to change the test
Created attachment 170101 [details] mylyn/context/zip
junit test are running without failures
Thanks for looking into this Frank! Looks like we are good now.
