Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 235222

Summary: [wikitext] integrates Texile-J with bugzilla task editor
Product: z_Archived Reporter: Jingwen 'Owen' Ou <jingweno>
Component: MylynAssignee: Jingwen 'Owen' Ou <jingweno>
Status: RESOLVED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: greensopinion, mik.kersten, steffen.pingel
Version: dev   
Target Milestone: 3.0   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
Jira wiki integration step 1
none
Team Project Set of Mylyn+TextileJ
none
integration step 1
none
wiki integration step 2 - Textile J part
none
wiki integration step 2 - Mylyn part
none
patch against sandbox for Textile-J contributed editor
none
wiki integration step 3
none
patch that solves the 'jumping around editor' issue
none
updated patch
none
mylyn/context/zip
none
layout issues fixed again
none
patch fixes layout issues and cut/copy/paste actions, and preview scrollbar none

Description Jingwen 'Owen' Ou CLA 2008-06-02 17:02:33 EDT
implement the most simple proto-type that integrates Texile-J with the JIRA task editor
Comment 1 Jingwen 'Owen' Ou CLA 2008-06-03 04:58:27 EDT
Created attachment 103264 [details]
Jira wiki integration step 1

1. "Source" and "Preview" tab
2. AbstractRederingEngine model
3. source <-> preview action
Comment 2 David Green CLA 2008-06-03 11:58:02 EDT
There are a few issues with the JIRA connector that will need to be addressed:

* the JIRA connector retrieves comments (and possibly other information) using RSS, and thus receives HTML content.  The editor will therefore never see the original Confluence markup.
** the connector also strips HTML tags from the markup, so the markup is completely absent from the content by the time it gets to the editor.

Also if you're creating patches using Textile-J, you should be working against the branch found here: https://textile-j.dev.java.net/svn/textile-j/branches/mylyn.wikitext/java/

we should coordinate on today's conference call.  See related patch in bug# 234210
Comment 3 David Green CLA 2008-06-03 12:04:22 EDT
I took a look at your patch and it looks like you are working against the right branch -- great!

After taking a quick look at the JIRA connector I think that we should consider a different target for the first prototype integration.  Perhaps the Bugzilla connector, since it uses the AbstractTaskEditorPage.
Comment 4 Steffen Pingel CLA 2008-06-04 03:06:12 EDT
Owen, can you attach a Team Project Set that contains the Textile-J projects that I need to make your patch compile?
Comment 5 Steffen Pingel CLA 2008-06-04 03:41:02 EDT
That's a great start Owen! I will try out your patch tomorrow and if we make the implementation based on the extension point on bug 234210 I should be able to apply this to the sandbox.

(In reply to comment #3)
> After taking a quick look at the JIRA connector I think that we should consider
> a different target for the first prototype integration.  Perhaps the Bugzilla
> connector, since it uses the AbstractTaskEditorPage.

Yes, that's a good point. Hopefully the editor framework is sufficiently generic to allow reuse of the wikitext implementation for all editors that extends AbstractTaskEditorPage. 
Comment 6 Jingwen 'Owen' Ou CLA 2008-06-04 14:14:36 EDT
I will be working to integrate the extension point in bug 234210.


> That's a great start Owen! I will try out your patch tomorrow and if we make the
> implementation based on the extension point on bug 234210 I should be able to
> apply this to the sandbox.
> 
> (In reply to comment #3)
> > After taking a quick look at the JIRA connector I think that we should
> consider
> > a different target for the first prototype integration.  Perhaps the Bugzilla
> > connector, since it uses the AbstractTaskEditorPage.
> 
> Yes, that's a good point. Hopefully the editor framework is sufficiently generic
> to allow reuse of the wikitext implementation for all editors that extends
> AbstractTaskEditorPage.
Comment 7 Jingwen 'Owen' Ou CLA 2008-06-04 14:19:47 EDT
Created attachment 103615 [details]
Team Project Set of Mylyn+TextileJ
Comment 8 Steffen Pingel CLA 2008-06-06 03:05:00 EDT
Owen, I suggest the following as a next step:

Implement an AbstractTaskEditorPageFactory that replaces the page contributed by BugzillaTaskEditorPageFactory. Instead contribute your own page that extends BugzillaTaskEditorPage and replaces the description part with a richt text enabled part as you did in your patch for the JIRA editor. As a first pass an editable description will do. It would be great if it used the extension point on bug 234212.

In a second pass it would be great to add the preview. The smaller the patches the easier (and faster) it will be for me to review and apply them and they are less likely to break due to some of the refactorings that are still being applied to head.   
Comment 9 Jingwen 'Owen' Ou CLA 2008-06-06 20:18:53 EDT
I noticed you added part of David's patch to org.eclipse.mylyn.sandbox.ui.editors. Do I assume other changes from David's patch are also made, e.g. AbstractRepositorySettingsPage, TasksUiExtensionReader and RichTextAttributeEditor? I think these objects are the most essential parts that apply the patch.

If we stick to David's extension point mechanism, "replacing the description part" is no longer needed. Here is how we roll now: add a new AbstractTaskEditorExtension and corresponding task editor will support the wiki stuff, including the description/summary/comment, since the patch fixed the RichTextAttributeEditor which is used to display text in all the task editor. 

> Owen, I suggest the following as a next step:
> 
> Implement an AbstractTaskEditorPageFactory that replaces the page contributed by
> BugzillaTaskEditorPageFactory. Instead contribute your own page that extends
> BugzillaTaskEditorPage and replaces the description part with a richt text
> enabled part as you did in your patch for the JIRA editor. As a first pass an
> editable description will do. It would be great if it used the extension point
> on bug 234212.
> 
> In a second pass it would be great to add the preview. The smaller the patches
> the easier (and faster) it will be for me to review and apply them and they are
> less likely to break due to some of the refactorings that are still being
> applied to head.
Comment 10 Steffen Pingel CLA 2008-06-06 21:38:30 EDT
> I noticed you added part of David's patch to
> org.eclipse.mylyn.sandbox.ui.editors. Do I assume other changes from David's
> patch are also made, e.g. AbstractRepositorySettingsPage, TasksUiExtensionReader
> and RichTextAttributeEditor? I think these objects are the most essential parts
> that apply the patch.

I want to hold off making changes to the tasks framework for a bit longer. My sense is that it is too early to commit to an API. I would like to explore other ways of extending the framework first even if that might seem to be slightly more involved.

> If we stick to David's extension point mechanism, "replacing the description
> part" is no longer needed. Here is how we roll now: add a new
> AbstractTaskEditorExtension and corresponding task editor will support the wiki
> stuff, including the description/summary/comment, since the patch fixed the
> RichTextAttributeEditor which is used to display text in all the task editor.

Yes, it is certainly the goal but we need to additional work, e.g. outline support, content assist integration, hyperlinking etc. before putting this into the tasks framework. 

As a first pass I would suggest a global preference to enable or disable the rich text editor integration, i.e. the page factory described in comment 8 would return false in canCreatePageFor() when not enabled. What do you think?
Comment 11 David Green CLA 2008-06-07 00:11:50 EDT
(In reply to comment #10)
> I want to hold off making changes to the tasks framework for a bit longer. My
> sense is that it is too early to commit to an API. I would like to explore
> other ways of extending the framework first even if that might seem to be

That sounds good to me.  Do you have any ideas as to what those other ways might look like?

> slightly more involved.
> 
> > If we stick to David's extension point mechanism, "replacing the description
> > part" is no longer needed. Here is how we roll now: add a new

Agreed, although for short-term experimentation replacing the description part might be a good start

> > AbstractTaskEditorExtension and corresponding task editor will support the wiki
> > stuff, including the description/summary/comment, since the patch fixed the
> > RichTextAttributeEditor which is used to display text in all the task editor.
> 
> Yes, it is certainly the goal but we need to additional work, e.g. outline
> support, content assist integration, hyperlinking etc. before putting this into

Content assist integration won't be necessary -- we'll get it for free by way of the command mechanism and the SourceViewerConfiguration.  Same with hyperlinking.  All that we'll need to do here is set the context id with the IContextService when the ISourceViewer gets focus, and set it back when the focus goes somewhere else.

As for outline support, while we should take that into consideration in my opinion it could come later.  We'll only get additional outline items if people use headings in their markup anyways (I've never seen that before).

> the tasks framework. 
> 
> As a first pass I would suggest a global preference to enable or disable the
> rich text editor integration, i.e. the page factory described in comment 8
> would return false in canCreatePageFor() when not enabled. What do you think?
> 

I think that a global preference sounds good.  In my opinion the best way to integrate would be to substitute a subclass of AttributeEditorFactory that returns an extension-based AttributeEditor instead of RichTextAttributeEditor when the preference is enabled.  This would not only be the least amount of work, it would also be closer to the desired outcome.  It would also mean that editors continue with their current behaviour until the preference is enabled, which I think is what we want in the short-term.
Comment 12 Jingwen 'Owen' Ou CLA 2008-06-07 08:57:18 EDT
Created attachment 104084 [details]
integration step 1

I don't know how to fix the following problem, may need help:

* in MarkupTaskEditorExtension, SWT.WRAP does not work and ENTER key does not work
* in MarkupTaskEditorExtension,  FastMarkupPartitioner does not work, indicating "Class not found" in runtime

Future work items:

* fixing the code in sandbox.ui.editors.bugzilla, mostly copy from the superclasses
* more wiki support, e.g. outline, presentation, etc.
Comment 13 David Green CLA 2008-06-07 22:56:15 EDT
(In reply to comment #12)
> Created an attachment (id=104084) [details]
> integration step 1
> 
> I don't know how to fix the following problem, may need help:
> 
> * in MarkupTaskEditorExtension, SWT.WRAP does not work and ENTER key does not
> work
> * in MarkupTaskEditorExtension,  FastMarkupPartitioner does not work,
> indicating "Class not found" in runtime
> 
> Future work items:
> 
> * fixing the code in sandbox.ui.editors.bugzilla, mostly copy from the
> superclasses
> * more wiki support, e.g. outline, presentation, etc.
> 

Owen, please attach your eclipse error log 
Comment 14 Steffen Pingel CLA 2008-06-08 00:32:26 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > I want to hold off making changes to the tasks framework for a bit longer. My
> > sense is that it is too early to commit to an API. I would like to explore
> > other ways of extending the framework first even if that might seem to be
> 
> That sounds good to me.  Do you have any ideas as to what those other ways might
> look like?

The only way I can think of is to replace task editor pages but there might be more extensibility built-in to the framework that I am missing.

> Agreed, although for short-term experimentation replacing the description part
> might be a good start

Yes, this would be nice for creating new tasks where the description is editable. Owen, it should be fairly straight forward to make this change for new comments and the description. 

> > Yes, it is certainly the goal but we need to additional work, e.g. outline
> > support, content assist integration, hyperlinking etc. before putting this
> into
> 
> Content assist integration won't be necessary -- we'll get it for free by way of
> the command mechanism and the SourceViewerConfiguration.  

The task editor has it's own content assist implementation for referring to other bug reports. I am not sure but we might have to do some work to integrate this with the default content assist provided by Textile-J?

> Same with
> hyperlinking.  All that we'll need to do here is set the context id with the
> IContextService when the ISourceViewer gets focus, and set it back when the
> focus goes somewhere else.

Yes, this should work if Textile-J picks up the default hyperlink detectors from platform. Mylyn contributes detectors for bug ids and Java stack traces.

> As for outline support, while we should take that into consideration in my
> opinion it could come later.  We'll only get additional outline items if people
> use headings in their markup anyways (I've never seen that before).

That is definitely low priority but if there is time I would like to investigate an extensible outline model for the task editor. The current impelementation is very limited (see bug 165859).

> I think that a global preference sounds good.  In my opinion the best way to
> integrate would be to substitute a subclass of AttributeEditorFactory that
> returns an extension-based AttributeEditor instead of RichTextAttributeEditor
> when the preference is enabled.  This would not only be the least amount of
> work, it would also be closer to the desired outcome.  It would also mean that
> editors continue with their current behaviour until the preference is enabled,
> which I think is what we want in the short-term.

Agreed. We could also consider adding a property to TaskAttributeMetaData that specifies a content  type for an attribute. This could work well for JIRA where wiki syntax is not enabled on a per-repository basis but can be configured on a JIRA project basis.
Comment 15 Steffen Pingel CLA 2008-06-08 00:42:48 EDT
The patch is looking good. It still seems to reference Textile-J from Mylyn. Can you split it up into two parts: A patch against Textile-J that implements the extension point (i.e. makes the required to the existing MarkupTaskEditorExtension in Textile-J) and a second patch against Mylyn that extends the Bugzilla editor and does not add a (hard wired) dependency on Textile-J to Mylyn? 

Here is a suggestion for improvement: Try to extend BugzillaTaskEditorPage and RichTextEditorPart instead of copying the whole implementation. We can change the visibility of methods or extract methods in the tasks framework if that improves re-usability. As long as changes preserve semantics and only affect internal classes there should be no problem doing this.
Comment 16 David Green CLA 2008-06-08 16:30:53 EDT
(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I want to hold off making changes to the tasks framework for a bit longer. My
> > > sense is that it is too early to commit to an API. I would like to explore
> > > other ways of extending the framework first even if that might seem to be
> > 
> > That sounds good to me.  Do you have any ideas as to what those other ways might
> > look like?
> 
> The only way I can think of is to replace task editor pages but there might be
> more extensibility built-in to the framework that I am missing.

It seems to me that integration at the AttributeEditorFactory level is a much better solution than replacing task editor pages.  The patch that I submitted previously uses this approach and it is very effective.  It works for both view-only markup and the editor for new comments and task description.

> 
> > Agreed, although for short-term experimentation replacing the description part
> > might be a good start
> 
> Yes, this would be nice for creating new tasks where the description is
> editable. Owen, it should be fairly straight forward to make this change for
> new comments and the description. 

Keep in mind that my previous patch made this work without needing to change the task editor page.... the description editor is replaced with the Textile-J-contributed editor via the AttributeEditorFactory.

> 
> > > Yes, it is certainly the goal but we need to additional work, e.g. outline
> > > support, content assist integration, hyperlinking etc. before putting this
> > into
> > 
> > Content assist integration won't be necessary -- we'll get it for free by way of
> > the command mechanism and the SourceViewerConfiguration.  
> 
> The task editor has it's own content assist implementation for referring to
> other bug reports. I am not sure but we might have to do some work to integrate
> this with the default content assist provided by Textile-J?

That sounds like an interesting problem to solve.  Textile-J uses a MultiplexingContentAssistProcessor class that dispatches to multiple delegates.  It may be a simple matter of referencing the Mylyn one from there.  Is the Mylyn IContentAssistProcessor implementation public API?  (if not, perhaps it should be)

> 
> > Same with
> > hyperlinking.  All that we'll need to do here is set the context id with the
> > IContextService when the ISourceViewer gets focus, and set it back when the
> > focus goes somewhere else.
> 
> Yes, this should work if Textile-J picks up the default hyperlink detectors
> from platform. Mylyn contributes detectors for bug ids and Java stack traces.

The current behaviour of Textile-J is to use the default hyperlink detectors from the platform, so this one should be easy.

> 
> > As for outline support, while we should take that into consideration in my
> > opinion it could come later.  We'll only get additional outline items if people
> > use headings in their markup anyways (I've never seen that before).
> 
> That is definitely low priority but if there is time I would like to
> investigate an extensible outline model for the task editor. The current
> impelementation is very limited (see bug 165859).
> 
> > I think that a global preference sounds good.  In my opinion the best way to
> > integrate would be to substitute a subclass of AttributeEditorFactory that
> > returns an extension-based AttributeEditor instead of RichTextAttributeEditor
> > when the preference is enabled.  This would not only be the least amount of
> > work, it would also be closer to the desired outcome.  It would also mean that
> > editors continue with their current behaviour until the preference is enabled,
> > which I think is what we want in the short-term.
> 

On second thought a global preference may be redundant.  My patch already contributes a per-repository preference.

> Agreed. We could also consider adding a property to TaskAttributeMetaData that
> specifies a content  type for an attribute. This could work well for JIRA where
> wiki syntax is not enabled on a per-repository basis but can be configured on a
> JIRA project basis.

This might be useful, however it may get too complicated.  Some users may wish to use markup for descriptions etc. even though it's not enabled on the server.  In my opinion the per-repository setting should be enough
Comment 17 David Green CLA 2008-06-08 16:34:09 EDT
(In reply to comment #15)
> Here is a suggestion for improvement: Try to extend BugzillaTaskEditorPage and
> RichTextEditorPart instead of copying the whole implementation. We can change

Great suggestion.  Owen, may I also suggest that the extended BugzillaTaskEditorPage simply override createAttributeEditorFactory() and return a subclass of the default factory.  The subclassed AttributeEditorFactory could work much the same way as in the original patch.
Comment 18 David Green CLA 2008-06-08 16:35:48 EDT
(In reply to comment #12)
> * in MarkupTaskEditorExtension, SWT.WRAP does not work and ENTER key does not
> work
> * in MarkupTaskEditorExtension,  FastMarkupPartitioner does not work,
> indicating "Class not found" in runtime

Owen, I've been doing some project renaming and package renaming.   I think that you probably should get everything (Textile-J) from scratch, considering some of the project names have changed.
Comment 19 Jingwen 'Owen' Ou CLA 2008-06-09 19:59:46 EDT
Created attachment 104233 [details]
wiki integration step 2 - Textile J part

**done**

* moved MultiSourceViewer to org.eclipse.mylyn.wikitext.ui.viewer
* added syntax highlight
* fixed the previous problems with FastMarkupPartitioner, turned out I only imported a specific package without making the dependency, stupid mistake...

**future work**

* integrating more support from Textile-J
Comment 20 Jingwen 'Owen' Ou CLA 2008-06-09 20:08:06 EDT
Created attachment 104234 [details]
wiki integration step 2 - Mylyn part

**done**

* extended BugzillaTaskEditorPage to invoke WikiTextAttributeEditor which extends RichTextAttributeEditor
* renamed MarkupTaskEditorExtension to AbstractMarkupTaskEditorExtension
* renamed the extension point in TaskeditorExtensionReader
* more beautiful code

**future work**

* Mylyn should have no knowledge of Textile-J
* standby for more orders...:)
Comment 21 Jingwen 'Owen' Ou CLA 2008-06-09 20:30:22 EDT
thanks Steffen and David for the super helpful pointers. 

Steffen: I tried to satisfy that Mylyn doesn't refer to Textile-J. But if I isolated the two, I have to move all the patches to the Textile-J part. The key problem is the *getControl* method in *WikiTextAttributeEditor* which refers to *MultiSourceViewer* in the Textile-J side. I have to do this reference since I need to return the parent control (CTabFolder) for resizing, e.g. line 136 of ??TaskEditorRichTextPart??. 

Also in *WikiTextAttributeEditor*, it will be cool that ??RichTextAttributeEditor?? has a protected method to create the SourceViewer (e.g. ??RepositoryTextViewer??), then subclasses can then create its own SourceViewer without overriding ??createControl?? and copying the code in it. Any thoughts?

David: in the editor part of AbstractTaskEditorExtension, could you give me an example on using TaskRepository as an parameter of createViewer and createEditor, e.g. like the one you did in ur ??MarkupTaskEditorExtension??? I still don't know what it does. I didn't refer to it since I don't think its necessary. Thanks.

BTW, probably you guys have noticed I used some wiki syntax in this comment. Feel *dull* about the style? Go apply my patch and see Mylyn+Textile-J in action :).
Comment 22 Jingwen 'Owen' Ou CLA 2008-06-09 20:37:20 EDT
David, the parser might have some formating typos, the space between normal text and the *strong* text disappear, e.g. "the *getControl* method in *WikiTextAttributeEditor*" becomes "the getControlmethod in WikiTextAttributeEditor". 


> thanks Steffen and David for the super helpful pointers.
> 
> Steffen: I tried to satisfy that Mylyn doesn't refer to Textile-J. But if I
> isolated the two, I have to move all the patches to the Textile-J part. The key
> problem is the *getControl* method in *WikiTextAttributeEditor* which refers to
> *MultiSourceViewer* in the Textile-J side. I have to do this reference since I
> need to return the parent control (CTabFolder) for resizing, e.g. line 136 of
> ??TaskEditorRichTextPart??.
> 
> Also in *WikiTextAttributeEditor*, it will be cool that
> ??RichTextAttributeEditor?? has a protected method to create the SourceViewer
> (e.g. ??RepositoryTextViewer??), then subclasses can then create its own
> SourceViewer without overriding ??createControl?? and copying the code in it.
> Any thoughts?
> 
> David: in the editor part of AbstractTaskEditorExtension, could you give me an
> example on using TaskRepository as an parameter of createViewer and
> createEditor, e.g. like the one you did in ur ??MarkupTaskEditorExtension??? I
> still don't know what it does. I didn't refer to it since I don't think its
> necessary. Thanks.
> 
> BTW, probably you guys have noticed I used some wiki syntax in this comment.
> Feel *dull* about the style? Go apply my patch and see Mylyn+Textile-J in action
> :).
Comment 23 David Green CLA 2008-06-09 21:31:41 EDT
 (In reply to comment #22)
> David, the parser might have some formating typos, the space between normal text
> and the *strong* text disappear, e.g. "the *getControl* method in
> *WikiTextAttributeEditor*" becomes "the getControlmethod in
> WikiTextAttributeEditor".

please create a seperate bugzilla issue for this bug
Comment 24 David Green CLA 2008-06-09 21:36:38 EDT
Created attachment 104240 [details]
patch against sandbox for Textile-J contributed editor

 (In reply to comment #21)
> thanks Steffen and David for the super helpful pointers.
> 

Nice work! 

> Steffen: I tried to satisfy that Mylyn doesn't refer to Textile-J. But if I
> isolated the two, I have to move all the patches to the Textile-J part. The key
> problem is the *getControl* method in *WikiTextAttributeEditor* which refers to
> *MultiSourceViewer* in the Textile-J side. I have to do this reference since I
> need to return the parent control (CTabFolder) for resizing, e.g. line 136 of
> ??TaskEditorRichTextPart??.

I'm not sure but this patch that I've been using locally may solve your problem.

> 
> Also in *WikiTextAttributeEditor*, it will be cool that
> ??RichTextAttributeEditor?? has a protected method to create the SourceViewer
> (e.g. ??RepositoryTextViewer??), then subclasses can then create its own
> SourceViewer without overriding ??createControl?? and copying the code in it.
> Any thoughts?

I don't think that the RichTextAttributeEditor is intended to be subclassed -- and it probably shouldn't be.  My patch shows a very simple AttributeEditor implementation that may be of some use.
> 
> David: in the editor part of AbstractTaskEditorExtension, could you give me an
> example on using TaskRepository as an parameter of createViewer and
> createEditor, e.g. like the one you did in ur ??MarkupTaskEditorExtension??? I
> still don't know what it does. I didn't refer to it since I don't think its
> necessary. Thanks.

See the attached patch.
> 
> BTW, probably you guys have noticed I used some wiki syntax in this comment.
> Feel *dull* about the style? Go apply my patch and see Mylyn+Textile-J in action
> :).

Cool!
Comment 25 David Green CLA 2008-06-09 21:38:42 EDT
 (In reply to comment #21)
> David: in the editor part of AbstractTaskEditorExtension, could you give me an
> example on using TaskRepository as an parameter of createViewer and
> createEditor, e.g. like the one you did in ur ??MarkupTaskEditorExtension??? I
> still don't know what it does. I didn't refer to it since I don't think its
> necessary. Thanks.
> 

The idea here is that the extension may need to access per-repository configuration, which is available from the TaskRepository.  If we don't pass it in, then the extensions won't ever be able to have their own configuration.
Comment 26 Steffen Pingel CLA 2008-06-09 23:22:14 EDT
> Steffen: I tried to satisfy that Mylyn doesn't refer to Textile-J. But if I
> isolated the two, I have to move all the patches to the Textile-J part. The key
> problem is the *getControl* method in *WikiTextAttributeEditor* which refers to
> *MultiSourceViewer* in the Textile-J side. 

I am not sure where the CTab should go that contains the preview and editor. Is it possible to move it to Mylyn and construct the widgets for the tabs through the extension point? You could try to avoid sub-classing SourceViewer and use composition instead.

As David pointed out it might be easier to sub-class AbstractAttributeEditor for now and we'll generalize RichTextAttributeEditor later. You can just hard code a sensible size hint for now, e.g. widthHint=500 and heightHint=150.

> BTW, probably you guys have noticed I used some wiki syntax in this comment.
> Feel *dull* about the style? Go apply my patch and see Mylyn+Textile-J in action
> :).

Will do :).
Comment 27 David Green CLA 2008-06-09 23:48:02 EDT
Great job on the patch Owen.  I'm impressed by what you've accomplished in such a short period of time.
Now that I've had a chance now to review it in more detail, here are some suggestions on your contibution (using Textile markup):

* In @WikiTextAttributeEditor@ you won't need to override getControl() if you set the control properly in the createControl() method
* Perhaps your @WikiTextAttributeEditor@ should do the work of hooking up the tabs and preview versus editor rather than the @AbstractMarkupTaskEditorExtension@, that way any classes implementing @AbstractTaskEditorExtension@ will get the benefit of a preview tab
** this should also solve your 'MultiSourceViewer' typecasting for resize problem
* I've updated Textile-J to implement the sandbox extension point and abstract class, so there should be no need to do so in the sandbox itself.
* The @TaskEditorWikiTextPart@ should use @editor.getControl()@ rather than @editor.getViewer().getTextWidget()@ and ensuing typecasts
* Classes names should not refer to 'WikiText', since the extension is not about 'WikiText' -- it's a taskEditorExtension.  Only classes that are in the org.eclipse.mylyn.wikitext packages should have 'WikiText' in their name.

Some things to note about my earlier patch:

* comments are displayed in using the wikitext viewer
** some integration will be needed if we're going to use markup in the Bugzilla viewer, since the Textile markup parser doesn't recognize Bugzilla-style quoted text
* for some reason the 'new comment' editor and the 'description' attributes aren't showing up in the page
* the patch relies on the markup language being set for a specific repository, however the UI for doing that is in the original patch on bug# 234210 (which was only partially applied to the sandbox) so there's currently no way to 'turn it on'.
** to circumvent this problem for now, the wikitext.textile.ui plugin registers Textile as the default for Bugzilla.  We'll have to remove this setting later.
* the patch fixes a couple of bugs in the TaskEditorExtensionReader and how it gets initialized

You've done a great job of addressing the issues of getting it all working, nice work.

Comment 28 Jingwen 'Owen' Ou CLA 2008-06-11 19:05:13 EDT
Created attachment 104576 [details]
wiki integration step 3

_please ignore my previous patches, this one includes everything cool_

*done*

* settled down the prototype, I think its pretty stable now
* only changed two classes (@BugzillaTaskEditorPageFactory@ and @BugzillaTaskEditorPage@)
* have to still subclass RichTextAttributeEditor although its not intended to be, check out line 137 of @AttributeEditorToolkit@
* Mylyn has no knowledge of Textile-J now (based on the latest implemetation of Textile-J which extends @AbstractTaskEditorExtension@ on its side)
* provide IContextService in RichTextAttributeEditor2

*future work*

* fix the awkward look of the "Status Whitebroad" in the "Attributes" area
* more Textile-J integration in the Mylyn side
* wait for more requests
Comment 29 Jingwen 'Owen' Ou CLA 2008-06-11 19:19:50 EDT
I tried to make as few changes as possible to achieve the goal which are good for Mylyn's refactoring right now. I only made two changes of the Mylyn code (@BugzillaTaskEditorPageFactory@ and @BugzillaTaskEditorPage@) and added one class (@RichTextAttributeEditor2@).

Steffen: could you update the Mylyn head for @TaskEditorExtensions@ and @TaskEditorExtensionReader@ (both are included in my latest patch, David made some significant improvement) ? 

David: thanks for the hints. It works! Also I fixed the naming problems that you brought out on comment 27.

I propose we use our developped tool to develop our tool: try to use WikiText as much as possible during our development lifecycle before it goes to the public, like what people in the inductry are doing, i.e. "IBM's Jazz's project":http://jazz.net. 

   
Comment 30 David Green CLA 2008-06-11 20:24:21 EDT
Great job with the patch!  The code looks much, much better.

Here are some observations:

# content-assist works well in the source editor, as does the cheat-sheet. (good)
# the source editor displays text properly (good)
# the preview tab needs a vertical scroll bar
# copy/cut/paste and other common commands aren't working at all in the source editor
# when editing in the source editor, the editor jumps around as you type.  I think that the 'folder' containing the editor needs to have a layout manager that makes the source editor occupy all of the available space.


Comment 31 David Green CLA 2008-06-11 20:27:10 EDT
Created attachment 104592 [details]
patch that solves the 'jumping around editor' issue

a patch based on Owen's work that sets a layout manager on the source editor folder, causing the source editor to occupy all available space in the folder.
Comment 32 Steffen Pingel CLA 2008-06-11 21:07:22 EDT
Great work! I'll take a look at the patches this evening.
Comment 33 David Green CLA 2008-06-12 00:07:29 EDT
another thought: the preview tab takes up a _lot_ of screen real-estate.  Perhaps we should do away with preview altogether, or alternatively put the preview button up in the header (next to "New Comment" twistie).
Comment 34 Steffen Pingel CLA 2008-06-12 00:54:41 EDT
David: Yes, that might be worth investigating, see bug 213131.
Comment 35 Steffen Pingel CLA 2008-06-12 06:53:19 EDT
Created attachment 104650 [details]
updated patch
Comment 36 Steffen Pingel CLA 2008-06-12 06:53:22 EDT
Created attachment 104651 [details]
mylyn/context/zip
Comment 37 Steffen Pingel CLA 2008-06-12 07:07:59 EDT
Great stuff! I have applied the patch with minor modifications (see the patch I attached):

Renamed BugzillaTaskEditorPage2 to ExtensibleBugzillaTaskEditorPage and other classes likewise. I have move the classes to the editor package. There are only few classes in the package and I am expecting that the implementation will move into the tasks framework rather that we end up creating additional packages for other connectors in the future.

Owen, please review my changes of the page factory. The wikitext enabled page now replaces the original page if an extension is registered. I am still contemplating whether a global option should be added to disable the functionality. I'll leave it as is (right now checking textile-j out from source is the only way to get an implementor of the extension) but we may need to add this.

I have changed createAttributeEditorFactory() in the task editor page to delegate to the "super" factory in case the attribute is not handled. This avoids some code duplication. I have also only enabled the handling for TYPE_LONG_RICH_TEXT type attributes for now. Using wiki syntax in bug summaries is not well supported by tasks ui right now, e.g. when task summaries are rendered in the task list or other dialogs.

David, I am not sure what your last patch changed. Could you re-cut the patch or describe the change you made?
Comment 38 David Green CLA 2008-06-12 09:43:30 EDT
Created attachment 104678 [details]
layout issues fixed again

reapplied editor layout fix to Steffen's patch
Comment 39 David Green CLA 2008-06-12 12:09:00 EDT
Created attachment 104721 [details]
patch fixes layout issues and cut/copy/paste actions, and preview scrollbar

a new patch that fixes the following:

* editor layout
* standard text-editor actions weren't working
* preview needs a scrollbar
Comment 40 Steffen Pingel CLA 2008-06-12 16:21:28 EDT
Thanks David. I have applied your patch. 

It looks like we are done here and have a first working prototype! I'll mark this bug resolved and we can create new bugs for any remaining nits and ideas how extend the current functionality: bug 236962 and bug 236961.
Comment 41 Jingwen 'Owen' Ou CLA 2008-06-12 16:35:41 EDT
oh wait...the reason that copy and paste does not work is STILL, the setControl method of the AbstractAttributeEditor (I set the control to CTabFolder), check out getTextViewer of EditorUtil and consequent calling method, e.g. canPerformAction. The task editor does not provide copy and paste action except for a TextViewer.

Probably we should really consider not applying the CTabFolder way, but only an editing source viewer with the preview button up in the top right header.

Any thoughts?
Comment 42 Jingwen 'Owen' Ou CLA 2008-06-12 16:38:24 EDT
my bad, didn't notice...David's patch fixed my concern, nice work