Bug 397757 - [refactoring] 'affectedProjects' variable on the refactoring participant extension points
Summary: [refactoring] 'affectedProjects' variable on the refactoring participant exte...
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.3   Edit
Hardware: All All
: P5 enhancement (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 391360
  Show dependency tree
 
Reported: 2013-01-09 07:50 EST by Karen Butzke CLA
Modified: 2013-01-11 11:10 EST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Karen Butzke CLA 2013-01-09 07:50:19 EST
Currently each of the org.eclipse.lt.core.refactoring participants extension points supports a  <with variable="..."> expression element of "affectedNatures". I am using faceted projects and would like to not enable my refactoring participants unless any of the projects have the JPA facet. Since facets are not part of the platform, I think one way to acheive this would be to have an "affectedProjects" variable element. Then in the plugin.xml I could iterate over those and test for the facet. The support needs to be added to copyParticipants, createParticipants, deleteParticipants, moveParticipants and renameParticipants.

Is this something that I could get contributed to 4.3 if I provide a patch?
Comment 1 Markus Keller CLA 2013-01-09 13:45:12 EST
How would you determine the "affectedProjects"? Isn't this just the enclosing project of the "element" variable?

Note that the set of projects that will eventually be touched by a refactoring is dynamic and depends on the refactoring processor and on other refactoring participants. Computing that set would require new APIs on refactoring processors, and that would be a major change.

The "affectedNatures" variable only contains the natures of the refactored element, not the natures of all projects that are potentially touched. The "affectedNatures" variable is not even strictly necessary. E.g. these two enablement expressions are equivalent:

   <with variable="affectedNatures">
      <equals value="org.eclipse.jdt.core.javanature"/>
   </with>

   <with variable="element">
      <adapt type="org.eclipse.core.resources.IResource">
         <test property="org.eclipse.core.resources.projectNature"
               value="org.eclipse.jdt.core.javanature"/>
      </adapt>
   </with>


You can already process the enclosing project by adding your own property tester via the org.eclipse.core.expressions.propertyTesters extension point and using it like this:

<with variable="element">
   <adapt type="org.eclipse.core.resources.IResource">
      <test property="x.y.z.hasProjectFacet" value="myProjectFacet"/>
   </adapt>
</with>

In bug 391360, it sounds like you just need to know whether there are any projects of a specific facet in the workspace. That would be even easier to implement with a property tester that just tests a global property:

<test property="x.y.z.projectWithFacetExists" value="myProjectFacet"/>
Comment 2 Karen Butzke CLA 2013-01-09 14:04:11 EST
(In reply to comment #1)

> The "affectedNatures" variable only contains the natures of the refactored
> element, not the natures of all projects that are potentially touched. The
> "affectedNatures" variable is not even strictly necessary.

This is not my impression of "affectedNatures". If you look in ResourceProcessors.computeAffectedNatures(IResource) it is returning natures for the selected Project as well as all referencing projects.  I just want that list of referencing projects. So, if that isn't really a full list of all the projects that could be affected by the refactoring in the one project, then "affectedNatures" is not really working properly.

I was able to partially fix my problem by using "affecteNatures" and just checking for the facet nature. 

<with variable="affectedNatures">
    <iterate operator="or">
	 <equals value="org.eclipse.wst.common.project.facet.core.nature"/>
    </iterate>
</with>
Comment 3 Markus Keller CLA 2013-01-09 14:20:48 EST
Ah, you're right, it's really the natures of all projects that refer to the element's project (transitively).

But you could still implement this using a property tester that does the same in code. You need to implement a property tester that can read facets anyway.

Iterating over the affected projects in code is also more efficient than getting all projects first and then invoking the facet property tester on each project.
Comment 4 Karen Butzke CLA 2013-01-09 14:38:14 EST
> But you could still implement this using a property tester that does the
> same in code. You need to implement a property tester that can read facets
> anyway.

I'll give you a little background. That's where I started, with implementing a property tester that just checked for the JPA facet on all projects in the workspace. I put this in my own plug-ins first, but since they are not yet loaded, (and the whole point is that this is a problem for people that aren't using JPA), the property tester returns NOT_LOADED which the enablement framework treats as true/enabled. If I added a "forcePluginActivation=true" to my property tester it doesn't work, because the EvaluationContext created by ParticipantExtensionPoint does not have fAllowPluginActivation set to true. Thus my plugin is not loaded. Is there something I'm missing here?

My next step was to push my property tester to the facet framework, since that plug-in is always loaded in a WTP workspace, and what I was doing was generic to the facet framework. I entered bug 397720 and provided a patch, but it was suggested that a better fix might be what I have suggested on this bug. I could change that property tester to use IProject.getReferencingProjects() and check for the facet on each of those.


> Iterating over the affected projects in code is also more efficient than
> getting all projects first and then invoking the facet property tester on
> each project.

I'm not understanding this, isn't this exactly what the "affectedNatures" variable is doing? It's getting all the projects and then in the enablement expression it iterates over to check for a particular nature. I believe that iterate will short-circuit out as soon as it finds one with the specified nature.
Comment 5 Konstantin Komissarchik CLA 2013-01-09 19:05:46 EST
I suggested to Karen to ask for a facilitation here since writing a new property tester is not optimal when you can combine existing ones. The issue here is that raw information (transitive set of projects) is not exposed. The extension point takes the raw information and pre-processes it for a particular usecase (natures). By exposing affectedProjects property, any number of existing property testers can be used on the projects. There is already a tester for natures, a tester for facets, etc.
Comment 6 Markus Keller CLA 2013-01-10 07:22:45 EST
The affectedProjects and affectedNatures variables are redundant, since their value is entirely determined by the refactored element. However, the expression language is not expressive enough to allow external implementations of arbitrary contributable transformations. The only ways to operate on the current expression object are the <test> and <adapt> expressions. <test> is a dead end, since it returns a boolean, and <adapt> should not be abused for specific transformations.

I see 3 ways forward (in order of preference):

1) rely on <test> expressions if the condition cannot be expressed easily via the expression language.

2) add a <convert> element to the expression language, which uses a user-contributable converter to convert the current object to a new object (e.g. refactored element => affectedProjects)

3) add affectedProjects as an additional variable to all refactorings. Unfortunately, all existing refactorings already use ParticipantExtensionPoint#getParticipants(..., Object element, ..., String[] affectedNatures, ...), so the affectedProjects element would only be available on refactorings that are updated to support the new variable.

Adding more variables to the refactoring processor is not a sustainable solution, and I'd like to avoid that.


(In reply to comment #4)
> If I added a
> "forcePluginActivation=true" to my property tester it doesn't work, because
> the EvaluationContext created by ParticipantExtensionPoint does not have
> fAllowPluginActivation set to true. Thus my plugin is not loaded. Is there
> something I'm missing here?

You're right, it doesn't make sense not to allow plugin activation here. I've fixed that with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=225bac9818e68359d36dcf27d90c0b84f3f8cb6b
Comment 7 Karen Butzke CLA 2013-01-11 10:59:42 EST
(In reply to comment #6)
> 
> You're right, it doesn't make sense not to allow plugin activation here.
> I've fixed that with
> http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/
> ?id=225bac9818e68359d36dcf27d90c0b84f3f8cb6b

I have gone forward with fixing bug 391360 now that plugin activation is allowed. So I have my own property tester that checks for a particular facet on each of the referencing projects.

I still think the best solution is to add a variable 'referencingProjects' that could replace the existing variable 'affectedNatures' since the 2 are redundant. Then 'affectedNatures' could be deprecated. 'referencingProjects' would be more flexible than 'affectedNatures'.

Thanks for fixing the problem where it wouldn't let me activate my plugin. I'll leave this bug open in case it's decided to make this change in the future. Thanks!
Comment 8 Markus Keller CLA 2013-01-11 11:10:00 EST
Thanks for the feedback. I'll leave this enhancement request open for now.

The allowPluginActivation fix was actually this:
http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8386392d7578ca18f7015e7f8da7d6398e34852e