Bug 153932 - [History] Custom hyperlink detectors for comments in History view
Summary: [History] Custom hyperlink detectors for comments in History view
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: CVS (show other bugs)
Version: 3.2   Edit
Hardware: PC All
: P3 enhancement (vote)
Target Milestone: 3.3 M7   Edit
Assignee: platform-cvs-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
: 58646 (view as bug list)
Depends on: 88293 156219
Blocks:
  Show dependency tree
 
Reported: 2006-08-15 12:16 EDT by Eugene Kuleshov CLA
Modified: 2007-03-29 16:05 EDT (History)
10 users (show)

See Also:


Attachments
Prototype of the decoration extension point (14.26 KB, image/gif)
2006-08-19 16:20 EDT, Eugene Kuleshov CLA
no flags Details
Prototype of the comment decoration extension point (23.39 KB, patch)
2006-08-19 16:24 EDT, Eugene Kuleshov CLA
no flags Details | Diff
patch based on platform hyperlink detectors (2.36 KB, patch)
2007-03-29 00:20 EDT, Eugene Kuleshov CLA
no flags Details | Diff
even more trivial patch using built-in features of SourceViewer (2.46 KB, patch)
2007-03-29 00:53 EDT, Eugene Kuleshov CLA
no flags Details | Diff
demo of Mylar's hyperlink detector (12.68 KB, image/gif)
2007-03-29 01:07 EDT, Eugene Kuleshov CLA
no flags Details
proper way to pass target context (1.64 KB, patch)
2007-03-29 13:20 EDT, Eugene Kuleshov CLA
no flags Details | Diff
mylar/context/zip (150.97 KB, application/octet-stream)
2007-03-29 13:20 EDT, Eugene Kuleshov CLA
no flags Details
better patch for setting target (1.77 KB, patch)
2007-03-29 15:19 EDT, Eugene Kuleshov CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Kuleshov CLA 2006-08-15 12:16:16 EDT
Please provide extension point to contribute custom decorator for commants in CVS History. So, tools like Mylar could do some text parsing and introduce hyperlinks to ussues referenced from comments like this "Fix for SPR-1982" and provide hyperlink to issue in correspond Jira repository.

Default decorator can also provide links for http urls from CVS comments, so they can be opened in platform-default browser.
Comment 1 Michael Valenta CLA 2006-08-17 14:14:38 EDT
We've looked at this before and it would be a significant amount of work. We currently have no plan to address it.
Comment 2 Eugene Kuleshov CLA 2006-08-19 16:20:24 EDT
Created attachment 48233 [details]
Prototype of the decoration extension point
Comment 3 Eugene Kuleshov CLA 2006-08-19 16:24:51 EDT
Created attachment 48234 [details]
Prototype of the comment decoration extension point

This patch include extension point management, simple implementatio of the URL hyperlinks decorator
I also hooked this into the cvs history page.

Future improvements may include better support for tooltips. Links/actions priorities, popup menu, etc.
Comment 4 Mik Kersten CLA 2006-08-23 23:14:48 EDT
*** Bug 58646 has been marked as a duplicate of this bug. ***
Comment 5 Eugene Kuleshov CLA 2006-08-24 01:18:23 EDT
Michael, is there are chance you will consider this for earlly 3.3 releases?
Comment 6 Michael Valenta CLA 2006-08-24 09:05:19 EDT
Unfortunately, the patch you provided includes new API in Team. While I think it is great that exploratory work is going on for API between third party tools and repository providers, it is far too early in the process to consider releasing code into the Team component to support such an API. In bug 150302 comment 2, I point out that the purpose of the Team component is to provide repository integration into Eclipse. The same arguments presented there apply to this request.

What you are proposing is API between a repository provider and a third party. We have looked into WVCM in the past (see bug 37705) since it has the involvement of many of the repository vendors. However, the technology is not mature enough at this stage. Only when WVCM has reached maturity (or a similar API which has the support of a broad range of repository vendors comes along), will we consider adopting such an API in Eclipse.

However, this should not stop you from proceeding. There is CVS API for accessing the complete history of a file. This will allow you to present the history and decorate comments as you wish. I understand that it is not ideal as you will not have integration with the existing CVS and other repository provider history views. However, it is the best we can do until there is a defacto standard for a Team API between repository providers and third party tools.
Comment 7 Eugene Kuleshov CLA 2006-08-24 12:02:06 EDT
Michael, this is actually chicken and egg problem. We can't hook this kind of stuff into existing CVS UI without either this API is in Team plugin and used by CVS; or we have to either drop CVS support or reimplement CVS UI that would have this feature from scratch.

What I am suggesting should be a good start that could drive basic integration and since it is optional API, repository providers can decide if to use it or not. You can even make it provisional for initial phase.

I am willing to provide support for this in Mylar and Subclipse plugins, but I don't see how it can be done without API available on the Team.
Comment 8 Mark Phippard CLA 2006-08-24 12:52:26 EDT
I agree with Eugene.  This is exactly the sort of thing the Team API should be providing.  The Team API should be providing all of the extension points that a 3rd party plugin would need to interact with a Team provider.  Otherwise, plugins have to build point to point integrations and that just does not make any sense.

I would like to see Team provide an extension point that allows you to associate an issue tracker with a project.  Of course, this would come from a list of installed issue tracker plugins.  Team would need to provide a UI and probably persist something in the .project file.

Then there are at least two things that I think a Team provider could then do with this interface:

1)  On commit dialogs, when an issue tracker is associated it should be able to add an issue id# field to the dialog, complete with prompting and validation of entered issues.  There would probably also need to be a way for the issue tracker to add this information into the commit message before it is submitted.

2)  In History views, the Team provider should be able to let an issue tracker create hyperlinks on the commit messages based on information that might appear in the message.

3)  Haven't thought this through, but there might be a way to do something in the Synch view changesets as well.

I have already done something along these lines with Subclipse and it works well -- but it is a custom integration.  This could be done really well at the Team layer, and I do not really agree that it would be a lot of code or work.  It would add a lot of value to users.  Someone like Mylar could probably provide a Bugzilla implementation and I am sure Jira and other issue trackers would support this.

I would definitely support this in Subclipse.  I actually think all of the code for this, the parts for Team API, the support for it in CVS, could be provided by new developers.  I see no reason to close this request, it seems like a good way to solicit more developers to contribute to this area of Eclipse.
Comment 9 Gunnar Wagenknecht CLA 2006-08-24 13:23:03 EDT
Mhm. As far as I remember, the Team integration started with a very simply integration. Almost all UI was provided by Eclipse. However, some SCM vendors didn't like that. They wanted to provide their own commit dialogs, etc.

I don't think that the Team API should know about the meaning of an "issue" or even an issue tracker. That's why we have Mylar. It adds the meaning of "tasks" to Eclipse.

IMHO, Mylar should be granted that status of a Team "friend", i.e. add it to the list of plug-ins that get the internal packages without access warnings or errors. Team provides, that like to support the optional API, could then do so by providing an optional bundle that simply does not get activated if Mylar is not present.

But I also agree that parts of the CVS plug-in could be refactored and shared through a common base layer for "working-copy-like" repositories or pushed down to Team UI (eg., label decoration, commit dialogs, patch, etc.). 

BTW, I don't really understand the "JSR 147" dependency in terms of the Eclipse Team UI. AFAIK, JSR 147 doesn't specify API for interacting with a commit dialog or an Eclipse label decorator or how to render issue numbers to hyperlinks in the History view.
Comment 10 Mark Phippard CLA 2006-08-24 13:31:22 EDT
(In reply to comment #9)
> I don't think that the Team API should know about the meaning of an "issue" or
> even an issue tracker. That's why we have Mylar. It adds the meaning of "tasks"
> to Eclipse.

What we need Team to do is provide the extension point.  Why should Mylar have to be installed to get a feature like this?  What does this have to do with Mylar?  Sure they might use this, but I do not think they should own it.

There would be very little code in Team.  I think it could provide a UI for associating a specific issue tracker with a specific project, but that would not be essential.  The Team Provider and Issue Tracking provider would do the rest.  Possibly CVS and SVN could see that they have a lot of common code and some of that could be refactored into Team so that other providers could reuse it.  Again, not essential though.

The Eclipse Platform could provide a new component that is like Team but is for Issue trackers, but right now the needs are so small that would be overkill.  If a component like that ever materialized, this could be moved to there.

I use a custom version of Subclipse where I have hacked it to have an extension point so that I could then contribute stuff like this from my issue tracking tool, which is also an Eclipse plugin.  The concept works, uses Eclipse concepts, and requires not too much code.  The issue is getting the base extension point in a common place so that plugins do not have to be dependent on each other.

Comment 11 Mik Kersten CLA 2006-08-24 23:41:45 EDT
Mark: I agree with Gunnar's statements--Team has provided an API for working with source repositories, and Mylar is providing a framework for task/bug/ticket/issue repositories (and implementations for Bugzilla, JIRA, and Trac).  Making Team understand issue trackers is a by no means a small task, because it involves having abstractions for tasks, task attributes, repositories, etc.  And whereas the source repository stuff needs to be coupled to resources, it's very valuable for the task repository stuff not to be coupled to resources (e.g. for the Maven folks starting to use Mylar's Tasks framework headless for manipulating issues).  For more info check out the Tasks Framework stuff at: http://wiki.eclipse.org/index.php/Mylar_Architecture  If you have complaints about having Subclipse use the Tasks Framework please do let us know what they are (e.g. don't want to couple to a Technology project, you want to use some of the Tasks UI pieces without having the Task List view installed).  Also note that as much as possible we try not to "own" this which is why we are so proactive about taking in contributions.  But Task integration is a new thing in IDE land which is why growing it out of a Technology project (and hopefully into the Platform or a Tools project) is a great way of doing it.

Michael: regarding this report, if Eugene's current approach is not feasible, how about taking the simplest version of it and accepting hyperlink detectors for the cosmment area?  In terms of work it is very easy (we did it for the Task Editor) and simply involves making the comment area a SourceViewer and setting hyperlink detectors set on it.  If the source of those detectors can't be an extension point in Team they could come from Text (e.g. via registering a content type for comments and then installing all the hyperlink detectors registered for that content type).  Bug 88293 is scheduled for 3.3 and could be a reasonable way to provide this in text viewers other than editors, similar to how content assist is available outside of editors.  Thoughts?
Comment 12 Eugene Kuleshov CLA 2006-08-24 23:55:51 EDT
(In reply to comment #11)
> Michael: regarding this report, if Eugene's current approach is not feasible,
> how about taking the simplest version of it and accepting hyperlink detectors
> for the cosmment area?  In terms of work it is very easy (we did it for the
> Task Editor) and simply involves making the comment area a SourceViewer and
> setting hyperlink detectors set on it.  

This is basically what my change for CVS history panel did. It retrive decorators/detectors from extension point and delegate to them to extract links and apply necessary styles to the syeled text used to show history comments. I guess there is some gap in the API for those detectors. E.g. it does not pass any parameters around and only working with the text. Perhaps it should pass resource, so if wanted detector can do the match to project info or issue tracking repository. The detector I implemented wasn't using this because it was only detecting urls in the comment text.

> If the source of those detectors can't
> be an extension point in Team they could come from Text (e.g. via registering a
> content type for comments and then installing all the hyperlink detectors
> registered for that content type).  Bug 88293 is scheduled for 3.3 and could be
> a reasonable way to provide this in text viewers other than editors, similar to
> how content assist is available outside of editors.  Thoughts?

We need some way to register detectors/decorators and that should come from platform. So, any plugins can participate as consumers and as providers for those decorators.
I thought that Team provider would be a good place, but if it came from Text it would give bigger opportunities and comments in CVS history still can user those detectors.
Comment 13 Mark Phippard CLA 2006-08-25 09:11:58 EDT
(In reply to comment #11)
> Mark: I agree with Gunnar's statements--Team has provided an API for working
> with source repositories, and Mylar is providing a framework for
> task/bug/ticket/issue repositories (and implementations for Bugzilla, JIRA, and
> Trac).  Making Team understand issue trackers is a by no means a small task,
> because it involves having abstractions for tasks, task attributes,
> repositories, etc. 

I was not proposing that Team should be tackling anything as grand as what you list here.  I believe that should all exist in tools like Mylar or the issue tracking tools themselves.  I am simply suggesting that Team should declare an extension point that would allow a Team provider and issue tracker to connect with each other.  

When I coded this in Subclipse, as an example, it had code in its commit dialog to add an issue tracker field to the dialog.  Subclipse is providing this code.  The extension point did two things:

1)  The presence of an associated issue tracking provider served as a trigger to Subclipse to show this stuff on the dialog in the first place.  

2)  It then provided access to an Interface that Subclipse could call methods on to validate the input, as well as respond the a Browse... button request to pick issues from a list.

The issue tracker provided an implementation of those methods, and in the case of the Browse action, a dialog to browse and select issues.  The issues themselves are all exchanged as String.

When the user commits, another interface method can be called that accepts the commit message and the issues the user entered, and returns a potentially reformatted commit message.  It really depends on if the issue tracker wants to embed the issues in the message or not.

Finally, the other bit was similar to what Eugene is proposing in this issue.  When Subclipse history view is showing a commit message, it calls an Interface method that allows the issue tracker to parse the message and provide hyperlinks for any issues that might be embedded in the issue.

Hopefully that explains "my vision" and why I think it belongs in Team.  The second question was really about Mylar.  I like Mylar but I find the scope of the project confusing.  I have historically considered Mylar to be primarily about the features it has to filter the UI to the task you are working on.  But then you also have all of this great stuff for Bugzilla that could easily stand on its own as a project in its own right.  And if I am using an issue tracker where the Eclipse plugin comes from a third party, and I am not interested in the Mylar filtering stuff, then installing Mylar to get this one simple interface seems inappropriate.

Maybe it is time for an Issue tracking project to be started within Eclipse?  It could have a base framework like Team, and then tbe Bugzilla work you have written could migrate towards that and be the reference implementation, like CVS is for Team.  In that scenario, then I could see this idea living in that new base plugin, but I do not think it should live in Mylar.
Comment 14 Eugene Kuleshov CLA 2006-08-25 15:57:28 EDT
TaskMark, I believe that Mylar is Issue tracking project. It happens that to get tasks or the Task Focused UI, it needed a task managment and integration with the issue tracking systems. If you open Mylar's update site (http://www.eclipse.org/mylar/dl.php), you will see number of fetures, so you can select only Task List feature and specific connectors. You should be able to implement connector for your proprietory issue tracking system if it not yet sypported by Mylar. See http://wiki.eclipse.org/index.php/Mylar_Integrator_Reference#Task_repository_requirements
Comment 15 Michael Valenta CLA 2006-08-28 10:33:13 EDT
As Gunnar pointed out, we tried to provide a third party repository API in 1.0 and it was a failure. We are now leaving the definition of such an API up to the repository providers and the third-party clients. If, at some point, there is agreement between a significant number of repository providers and third-party clients, we may reconsider. But for now, we do not want to introduce this type of API into the Team component.

As for the possibility of adding the comment linking to CVS as Mik suggested, we will consider CVS specific patches.
Comment 16 Mark Phippard CLA 2006-08-28 10:47:54 EDT
(In reply to comment #15)
> As Gunnar pointed out, we tried to provide a third party repository API in 1.0
> and it was a failure. We are now leaving the definition of such an API up to
> the repository providers and the third-party clients. If, at some point, there
> is agreement between a significant number of repository providers and
> third-party clients, we may reconsider. But for now, we do not want to
> introduce this type of API into the Team component.

Fair enough, but I think I must be doing a poor job describing what I am talking about as I do not think it even remotely approaches the scope of what you are talking about.  I see this as an issue of Team providing an extension point for dependency convenience.  This would not force Team providers to adopt any new API or change their plugins.  It would provide them an option of allowing an issue tracker to integrate a bit with their UI, should they decide to take it.

The problem as I see it is that the alternative is a set of point to point integrations between specfic team providers (CVS, Subclipse etc..) and specific issue trackers (Bugzilla, JIRA etc..).  I do not see how this is good for the platform.

If a patch were provided that included the Team extension point and supporting code, as well as the CVS implementation, would it at least be considered as a starting point for discussion, or is the decision final that you just do not want to consider this for Team?
Comment 17 Eugene Kuleshov CLA 2006-08-28 11:00:09 EDT
I now have feeling that initial integration should basically allow CVS provider to use externally registered hyperlink detectors and actions that would be able to match, e.g. comment text to issues in some issue tracking system (this is what half of my patch tried to do).

It also seems like those hyperlink detectors should be available to the platform, so same functionality can be awaiblable for editors and other components. So, something similar to the second half of my patch should be in platform, may happend as work on the bug 88293.

Then, another thing is to link issues in commit comment that Mark is requiesting. I believe Mik already done some similar work in Mylar using initial commit comment text, but that need to be improved. E.g. allow Team provider to pass along list of attributes that can be used to populate comment templates. Then issue tracking providers will be responsible for populating those attributes. 

Then there is an issue for issue tracker content assist that also need to be registered externally and used by CVS provider when editing commit comment.
Comment 18 Michael Valenta CLA 2006-08-28 11:07:39 EDT
Regarding comment 16, as you say, at this time, it would be more convenient for you (and possibly others) to have this API in Team. However, IMHO, in the long run, putting this API in Team will actually prove to be inconvenient to a great many clients when this API is tailored to support additional scenarios, as it undoubtedly will be (as indicated by comment 17). The question I would ask is "Does this API need to be in the Team component to be a success?". The answer is no so there is little to be gained by putting it there.

You mention that, if it is not in the Team component, then the only option will be to have point-to-point solutions. Just because the API is not in the Team component doean't mean that it cannot be provided somewhere. It would be up to the participating repository providers and third party tools to decide where that is.
Comment 19 Eugene Kuleshov CLA 2006-08-28 11:11:57 EDT
(In reply to comment #18) 
> You mention that, if it is not in the Team component, then the only option will
> be to have point-to-point solutions. Just because the API is not in the Team
> component doean't mean that it cannot be provided somewhere. It would be up to
> the participating repository providers and third party tools to decide where
> that is.

I think some of this unctionality should be available to the text component and the rest may belong to the new plugin in the Platform.
Comment 20 Mark Phippard CLA 2006-08-28 11:20:08 EDT
(In reply to comment #18)
> You mention that, if it is not in the Team component, then the only option will
> be to have point-to-point solutions. Just because the API is not in the Team
> component doean't mean that it cannot be provided somewhere. It would be up to
> the participating repository providers and third party tools to decide where
> that is.

This is not meant to be a personal criticism of you, but I suspect that you do
not have good perspective for what it is like to develop an Eclipse plugin that lives out in the "ecosystem" and is not a part of Eclipse.  

How could the Subclipse team, as an example, develop an extension point like this and have it be of value to other team providers?  If it is not of value to other team providers, where is the incentive to issue trackers to support it?

If Mylar adds it, it might work, but as an issue tracking solution provider, I
do not have any interest in distributing Mylar with my solutuon.  It is also
somewhat odd for someone using a specific issue tracking tool, and a tool like
Subclipse, to have to go get a third, seemingly unrelated, piece for them to
integrate.

You can do stuff like this with the CVS plugin because you are a de-facto
standard as you are included in Platform.  Third party plugins cannot
realistically try to be the maintainer of something like this.  Mylar, perhaps
could do it, but it would not be the ideal and would ultimately be best spun
off into its own project.
Comment 21 Mik Kersten CLA 2006-08-28 11:58:36 EDT
Mark: Mylar was forced to start growing such an API (see org.eclipse.mylar.team.providers extension point Gunnar initially contributed), and this should probably in a way that is separated enough for what you need.  We are open to suggestions on how to make this consumable by Team providers.  Regarding making it a seperate project, there's a chicken-and-egg probelm there because we would need to have enough people using the current thing API before considering that, and are currently trying to have more providers (e.g. Subclipse) use the current API.  Note that our goal since the beginning was actually to make the concrete issue tracker Connectors separate projects (i.e. Bugzilla and JIRA, which we have been incubating to drive the Tasks API to be generic enough).  So we are trying to make it easy for Team providers to use our API whether they are providing issue tracking or not.  As an example implementation we ourselves provide extensions to CVS and Subclipse, but intend those to be examples for other Team providers (see Subclispe use of org.eclipse.mylar.team.providers extension point that Gunnar contributed, currently out-of-place in the org.eclipse.mylar.team plug-in).

All: I think Mark is bringing something really important up: for any of this to work, whoever provides the additional issue tracker integration and related APIs, we do need at least minimal support from Team.  E.g. Mylar relies on Change Sets to integrate with populating messages Commit dialogs, we all need the hyperlink functionality described here to get the user back out of those commit messages in the History view.  So I really do hope that this is done in a generic way where works for any commit message in History view, not just CVS which would force the painful point-to-point dependencies Mark brings up.  As Eugene, I think that this support would ideally come from Platform/Text and Team would use that (bug 88293, bug 154119).
Comment 22 Michael Valenta CLA 2006-08-28 14:33:32 EDT
For this particular issue, the best course of action seems to be to use the API proposed by bug 88293 when it becomes available.

The discussion in this bug has also included a larger issue of providing API in the Team component to support interactions between third-party tools and repository tools. As I have stated, my opinion is that this is beyond the current scope of the Team component in Eclipse. However, this issue is large enough that I have brought it to the attention of the PMC to ask for their input.

As for the last point in comment 20, it is true that there is internal code that lives in the Team plugin that is used by the CVS plugin. There are several reasons for this ranging from functionality we thought would be API but decided against to sharing code to reduce our code maintenance effort. However, I am not aware of any code we provide in Team that provides an unfair advantage to CVS. If you know of any instances of this, please bring them to my attention and we will fix it.
Comment 23 Alexander Gurov CLA 2006-08-29 09:17:46 EDT
My opinion is that solution should be more complicated. It should respect many additional requirements from the potential contributors. For example: Team Services should provide common configurable History View and “Hyperlinks Detector” will be only one of provided abilities. 
At the same time I agree with Michael Valenta that enhancing Team Services requires significant amount of work. I checked the patch provided by Eugene and I think it is good idea to implement extensions step-by-step.

P.S.
I think it is good idea if community collect requirements first and then contribute its implementation into Eclipse Platform…
Comment 24 Eugene Kuleshov CLA 2006-08-29 09:32:04 EDT
Alexander, why do you need a complicated solution? It should be simple! :-)
Comment 25 Alexander Gurov CLA 2006-08-29 10:04:39 EDT
Yes Eugene, I agree, it should be simple, but it should provide generic extendable History View to avoid its implementation in each team provider from scratch. :)
Comment 26 Eugene Kuleshov CLA 2006-08-29 10:08:19 EDT
(In reply to comment #25)
> Yes Eugene, I agree, it should be simple, but it should provide generic
> extendable History View to avoid its implementation in each team provider from
> scratch. :)

I am sorry, but this issue is not about generic history view. Besides, generic history view is already avaialble in 3.2 API.
Comment 27 Mik Kersten CLA 2006-08-29 17:40:28 EDT
From comment#22:
> However, this issue is large enough that I have brought it to the attention of the PMC to ask for their input.

Michael: could you please report the results of that input here or point me at the appropriate forum?  We are starting to plan Mylar 1.0 and the goal of graduating to be a Tools project, so I would like to start the discussion on which parts of the Task Management framework we can provide, or consider contributing to Platform.
Comment 28 Eugene Kuleshov CLA 2007-03-26 21:30:52 EDT
Michael, will you still accept patch for 3.3m7 if I'll manage to hook text hyperlinks?
Comment 29 Philippe Ombredanne CLA 2007-03-27 01:34:53 EDT
(In reply to comment #28)
> Michael, will you still accept patch for 3.3m7 if I'll manage to hook text
> hyperlinks?
+1 :-) 

Comment 30 Michael Valenta CLA 2007-03-27 08:06:30 EDT
Unfortunately, it's too late in the cycle. All new feature work must have PMC approval for M7 and, based on the discussions I've had with them about other potential M7 feature work, this one would be considered too complex for inclusion.
Comment 31 Eugene Kuleshov CLA 2007-03-27 12:06:29 EDT
(In reply to comment #30)
> Unfortunately, it's too late in the cycle. All new feature work must have PMC
> approval for M7 and, based on the discussions I've had with them about other
> potential M7 feature work, this one would be considered too complex for
> inclusion.

Hmm. Not sure why it would be too complex. I was thinking about using the same extension point, so the only change is in the comment panel implementation.
Comment 32 Michael Valenta CLA 2007-03-28 08:41:40 EDT
Perhaps saying it is too complex is confusing. At this point, we are feature frozen and only make exceptions for special cases. If the change was trivial or the feature was a stop ship, I could see it being a candidate but I don't think this feature is either of those.
Comment 33 Eugene Kuleshov CLA 2007-03-29 00:20:11 EDT
Created attachment 62337 [details]
patch based on platform hyperlink detectors

Can you please look at this patch. I think change is quite trivial.

What I couldn't figure out is how to make TextViewer to decorate detected links, but it does detect them on mouse roll over and navigation is working. At least for the default URL detector.
Comment 34 Eugene Kuleshov CLA 2007-03-29 00:53:22 EDT
Created attachment 62340 [details]
even more trivial patch using built-in features of SourceViewer

Michael, here is even more trivial patch that provides hyperlinking functionality. This one does respect modifiers (i.e. CTRL) configured for platform hyperlink detectors and highlighting is also working.
Comment 35 Eugene Kuleshov CLA 2007-03-29 01:07:51 EDT
Created attachment 62342 [details]
demo of Mylar's hyperlink detector
Comment 36 Michael Valenta CLA 2007-03-29 08:30:14 EDT
It does look trivial. I do have some questions though. If there are thousands of hyperlink detectors registered, how is the History view restricted to only those that apply (i.e. I can imagine that running the text through a large number of detectors may have performance implications)? Or is the the case that all of them are tried and some may give you links that actually invalid given the context? Also, if there are multiple bug tracking systems that use the same pattern to identify bugs, how is it determined which one to use?
Comment 37 Eugene Kuleshov CLA 2007-03-29 10:24:22 EDT
(In reply to comment #36)
> It does look trivial. I do have some questions though. If there are thousands of
> hyperlink detectors registered, how is the History view restricted to only those
> that apply 

Right now it is registered to all detectors for hyperlinkTarget org.eclipse.ui.DefaultTextEditor

> (i.e. I can imagine that running the text through a large number of
> detectors may have performance implications)? 

It will be same as  for text editor and will use exactly same detectors, registration and activation mechanisms. Note that detectors only activated when Ctrl is pressed.

> Or is the the case that all of
> them are tried and some may give you links that actually invalid given the
> context? 

I think it is responsibility of detectors to return correct hyperlinks. Also, SourceViewer will show the first matching hyperlink, since DefaultHyperlinkPresenter registered by TextSourceViewerConfiguration return false on canShowMultipleHyperlinks() call. I think this issue can be addressed later if needed.

> Also, if there are multiple bug tracking systems that use the same
> pattern to identify bugs, how is it determined which one to use?

This is a very good question. Right now detector will only have text for parsing and in some cases it won't be sufficient for contextual matching.

So, I would like to suggest to set additional data to the text widget (i.e. IAdaptable) when document is changed. So, detector can retrieve that data if it need additional context. I can update patch if you ok with that. The basic idea is like this (IFileRevision can be adapted using adapter factory and Mylar already does that):

		viewer.addSelectionChangedListener(new ISelectionChangedListener() {
			public void selectionChanged(SelectionChangedEvent event) {
				ISelection selection = event.getSelection();
				if (selection == null || !(selection instanceof IStructuredSelection)) {
					textViewer.setDocument(new Document("")); //$NON-NLS-1$
					textViewer.getTextWidget().setData("org.eclipse.ui.DefaultTextEditor", null); //$NON-NLS-1$
					tagViewer.setInput(null);
					return;
				}
				IStructuredSelection ss = (IStructuredSelection)selection;
				if (ss.size() != 1) {
					textViewer.setDocument(new Document("")); //$NON-NLS-1$
          textViewer.getTextWidget().setData("org.eclipse.ui.DefaultTextEditor", null); //$NON-NLS-1$
					tagViewer.setInput(null);
					return;
				}
				Object o = ss.getFirstElement();
				if (o instanceof AbstractHistoryCategory){
					textViewer.setDocument(new Document("")); //$NON-NLS-1$
          textViewer.getTextWidget().setData("org.eclipse.ui.DefaultTextEditor", null); //$NON-NLS-1$
					tagViewer.setInput(null);
					return;
				}
				IFileRevision entry = (IFileRevision)o;
				textViewer.setDocument(new Document(entry.getComment()));
        textViewer.getTextWidget().setData("org.eclipse.ui.DefaultTextEditor", entry); //$NON-NLS-1$
				tagViewer.setInput(entry.getTags());
			}
		});
Comment 38 Mik Kersten CLA 2007-03-29 11:23:17 EDT
 (In reply to comment #36)
> It does look trivial. I do have some questions though. If there are thousands of
> hyperlink detectors registered...

Just to add to Eugene's good points, the good thing is that the current solution provided by Eugene's patch is the same solution that has been accepted for Java editor and other text editors which are likely to be the target of many more hyperlink detectors (for the Mylar task one see http://www.eclipse.org/mylar/doc/images/2.0M1/java-editor-hyperlink.gif ).  Most of the discussion on why this approach scales was on bug 88293.  The thing that makes me happiest about it is that in the end the control is entirely in the users hands.  This means that if we do get into a worst-case clash scenario and the link goes to the wrong place, the user can always disable the detector via Preferneces -> Editors -> General -> Hyperlinking.  (But as I discussed on that bug, for 3.4 I think that we need to consider a solution that pops up an affordance that shows a list of detectors if more than one matches.)

So I'm also wondering if Eugene's patch could be small and low-risk enough to apply in M7 because it's just getting History comments viewer to follow the convention of other source viewers like the Java editor and Properties File Editor.
Comment 39 Michael Valenta CLA 2007-03-29 12:05:30 EDT
Alright, you've convinced me. The provided patch should do for 3.3. I've released it to HEAD. We can revisit scalability issues and conflicts if the need arises.
Comment 40 Eugene Kuleshov CLA 2007-03-29 12:11:25 EDT
(In reply to comment #39)
> Alright, you've convinced me. The provided patch should do for 3.3. I've
> released it to HEAD. We can revisit scalability issues and conflicts if the
> need arises.

How about setting contextual data as suggested in comment 37?

https://bugs.eclipse.org/bugs/show_bug.cgi?id=153932#c37
Comment 41 Michael Valenta CLA 2007-03-29 12:18:20 EDT
I would need to see what API this is satisfying. Do you have a link to a description of what the "org.eclipse.ui.DefaultTextEditor" data slot on a viewer is supposed to provide (I assume this is part of the Platform/Text hyperlinking API specification)?
Comment 42 Eugene Kuleshov CLA 2007-03-29 13:20:03 EDT
Created attachment 62402 [details]
proper way to pass target context

(In reply to comment #41)
> I would need to see what API this is satisfying. Do you have a link to a
> description of what the "org.eclipse.ui.DefaultTextEditor" data slot on a
> viewer is supposed to provide (I assume this is part of the Platform/Text
> hyperlinking API specification)?

You are right. Here is more appropriate way to set target. It supports adapting to IFile and IFileRevision and also allow to use custom adapter factory to adapt the history page to anything else. I can't speak for all possible consumers, but it will allow Mylar's hyperlink detector to identify context.
Comment 43 Eugene Kuleshov CLA 2007-03-29 13:20:07 EDT
Created attachment 62403 [details]
mylar/context/zip
Comment 44 Michael Valenta CLA 2007-03-29 13:36:44 EDT
I still want to see the spec myself before I agree to the code (i.e. how do I know that the spec doesn't require the slot to be an instance of IEditorPart). Is it stated in the Platform/Text javadoc somewhere? Also, you can't just return the input if asked for an IFile or an IFileRevision. You need to make sure the input implements the requested interface.

I also don't see how providing an IFileRevision helps you. Perhaps you could describe the purpose of this.
Comment 45 Eugene Kuleshov CLA 2007-03-29 14:09:38 EDT
Michael, this logic is implemented in org.eclipse.jface.text.hyperlink.AbstractHyperlinkDetector base class, which assume that target is an instance of IAdaptable.

Right now we don't have any code that uses IFileRevision, but I think it is useful for matching resources from remote version control (i.e. CVS Repositories / Show History) with issue tracking repositories. The best part is that we already have enough information to do that in some cases (i.e. there is a metadata provider that is using Maven's project model, pom.xml). 

It is actually possible to implement custom adapter factory to adapt history page to IFileResource, but I thought this simple check would save that trouble. If you think it is not appropriate, you can take it out. As long as Platform.getAdapterManager().getAdapter(CVSHistoryPage.this, adapter) is still there, it will be possible to adapt to IFileRevision.
Comment 46 Michael Valenta CLA 2007-03-29 14:43:53 EDT
OK, I see that now. As for the adaptation to IFile or IFileRevision, I don't mind if it is in, I just don't want it to return an object that doesn't implement what was requested. There is no way that the input to implements both those interfaces so the code is clearly wrong. I leave it up to you to decide whether you want to correct it or leave it out.
Comment 47 Eugene Kuleshov CLA 2007-03-29 15:19:19 EDT
Created attachment 62422 [details]
better patch for setting target

Michael, this version checks the type of getInput() so it should address your concerns.
Comment 48 Michael Valenta CLA 2007-03-29 15:36:43 EDT
Patch committed. Thanks.
Comment 49 Eugene Kuleshov CLA 2007-03-29 16:05:23 EDT
(In reply to comment #48)
> Patch committed. Thanks.

Michael, thanks a lot for taking time for review an apply these patches.