Groovy Plugin doesnt support rename participant callback

This patch looks great. There are a few things however.

1. Could you please add ASL licenses to the top of each created file? Feel free to add your own name to the top. For example, this:
/*

• Copyright (C) 2009 <name>
  *
• Licensed under the Apache License, Version 2.0 (the "License");
• you may not use this file except in compliance with the License.
• You may obtain a copy of the License at
  *
• http://www.apache.org/licenses/LICENSE-2.0
  *
• Unless required by applicable law or agreed to in writing, software
• distributed under the License is distributed on an "AS IS" BASIS,
• WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
• See the License for the specific language governing permissions and
• limitations under the License.
  */

2. Does you patch include any copied code? I am specifically looking at GroovyParticipantExtensionPoint. If this is copied from EPL or ASL code, please include the original license at the top of the code. If this is another license, then it is unlikely that we can use this code.

3. Again, with the GroovyParticipantExtensionPoint, there is no corresponding exsd file. Did you forget to include it?

4. Your RenameMethodProvider has a reference to the JavaEditor. This will be null if the refactoring is triggered from outside the editor (eg- the package explorer). Also, I'd rather not see that kind of reference in the core part of the code. It looks like the only reason you need to use the JavaEditor is to extract the ITypeRoot from it. I think you should be able to pass the IJavaElement directly into the RenameMethodProvider. This is easy if you are coming from a GroovyEditor. There is a method called getGroovyCompilationUnit(). Or you could always call EditorUtility.getEditorInputJavaElement(this, false); if not in a GroovyEditor.

Let me know if you have any questions.

Hi Andrew,
Thanks for your comments. Here is my response to each one.

1. I have added the license to each newly created file.

2. Although i have copied GroovyParticipantExtensionPoint code, but i have made some changes which make sense for groovy refatcoring. However i have included the original license with the file

3. GroovyParticipantExtensionPoint uses the existing extension point exposed by eclipse ltk, that's why i have not included any exsd file with it.

4. I cannot pass IJavaElement to RenameMethodProvidor because i need to get a handle on SourceMethod selected by the user, for this purpose i need handle to editor, if i try to get a handle to selected method node within GroovyRenameAction then it gives me an empty text selection, getGroovyCompilationUnit() only gives handle to ITypeRoot but not to the SourceMethod selected by the user for rename, please let me know if you can think of some other workaround for this.

I have attached a new patch here

Attached updated patch - patch_2_groovy_rename_participant_callback_support_submitted_by_Ajay.patch as per Andrew's comments 1 and 2

Thanks for the updated patch.

The textSelection is available from RenameDispatcherAction. The textSelection and the containing IJavaElement can be passed in to the constructor instead of the editor. And if rename is executed from the package explorer (or other view), the textSelection can be the location of the name of the IJavaElement. (Currently, this is not implemented, but I plan on doing this soon.)

I think this will work, but let me know if there is some reason it won't.

Attaching latest updated patch as per andrew's comments on text selection

Hi Andrew,
I have updated patch as per your comments on RenameDispatcherAction, please have a look at patch_3_groovy_rename_participant_callback_support_submitted_by_Ajay.patch

Thanks,
Ajay

This looks much better I've applied the patch and will commit later today. I may make a few small changes to remove things like unused variables.

When the change is committed, I will close the bug.

What is it that you need this piece for?

Hi Andrew,

I work in Twist dev team, and we have twist scenarios which are mapped to backing java or groovy code, the requirement is such that both needs to be in sync, so if scenario is refactored then corresponding groovy code needs to be modified and vice-versa, for this purpose i needed groovy plugin to support rename participant callbacks to include scenario changes in the same transaction.

Thanks for your time, let me know once you commit the changes.

This is ready to be committed, but I am holding off until tomorrow so that the next build of STS can complete.

Now available in trunk, but unrelated tests are failing, so snapshot is not available yet.

build is now available. Thanks for the patch.

Attaching a new patch with updated changes, andrew please apply this patch

Refactoring done by Andrew seems to have broken actual functionality, attached a new patch for the same

Ajay, why is it necessary to do the codeSelect at the end of the ASTNode location, rather than at the beginning?

The code that I have committed right now performs codeSelect at the beginning for a very important reason. Sometimes the ASTNode end locations are off slightly. They sometimes include trailing whitespace. And so, doing selectedASTNode.getStart()+selectedASTNode.getLength() may not point to the expected IJavaElement.

Also, since I am not exactly sure what behavior you are looking for, can you submit a test case to ensure that nothing will break in the future?

there was a slight mistake in the patch, it should be selectedASTNode.getStart() , selectedASTNode.getLength() instead.

The functionality works fine now

Hi Andrew,

I am reopening this bug, as i need the similar functionality for rename refactoring a groovy class as for a method level refactoring, i have attached a patch herewith. Please take a look at it.

Thanks,
Ajay

I'll have a look at this, but it will not be able to make it into 2.0.

In your most recent patch, you changed this line in RenameMethodProvider.java:

if (input == null || selectedASTNode == null) {

to this line:

if (input == null && selectedASTNode == null) {

And there is a similar line in RenameClassProvider.java. This can cause NPEs. Is there a reason why you changed this?

Attaching a new updated patch

Hi Andrew,
I believe you are right about NPE, it was a miss from my side, i have attached a new updated patch here, please have a look at it.

Thanks,
Ajay

Hi Andrew,
Did you got a chance to look at the patch? It would be great if we could close this as soon as possible.

Ajay

Not yet. 2.0 final is due out on Friday. I expect to have your patch in shortly after that.

Fix is applied internally. Will release changes after 2.0 final.

Will you also require rename participants for field renames?

As of now, i dont require this feature, but there is no harm in having this functionality, if you want i can submit a patch for that as well.

Closed. New patch is available in latest snapshot.

Hi Andrew,
I am getting NPE while using the refactoring API, it required a small null check to resolve this issue, I have attached a new patch here, please have a look at it.
Thanks,
Ajay

Attaching a new patch

I have already fixed this problem in trunk. No need to apply the patch.

Please open a new issue for any more changes that you need.