Fix refactoring descriptor recording for ExtractMethodRefactoring and InlineConstantRefactoring

[Wanderer777]

Both in ExtractMethodRefactoring and InlineConstantRefactoring, in the method populateRefactoringSpecificFields, the code line that stores the selection is commented out. As a result, the recorded refactoring descriptor for these two refactorings is incomplete, and thus, these refactorings can not be replayed. Could uncommenting the corresponding code lines fix this problem?

[reprogrammer]

This problem is not specific to ExtractMethodRefactoring and InlineConstantRefactoring. See InlineTempRefactoring and InlineMethodRefactoring.

If we uncomment the lines that set the "selection" attribute of refactoring descriptors in populateRefactoringSpecificFields, the refactoring descriptors captured by Eclipse will have the "selection" attribute and will be good to replay.

For unavailable refactorings, CodingSpectator creates a refactoring descriptor with an attribute called "selection". The "selection" attribute of an unavailable refactoring captures the offset of the beginning of the selection and the length of the selection relative to the code snippet. CodingSpectator computes the offsets relative to the code snippets because it captures only the code snippets in refactoring descriptors not the whole file. The problem here is that the Java refactoring descriptors already have an attribute called "selection", which is computed with respect to the whole file and gets used for replaying refactorings.

I suggest to rename the attribute in the descriptors of unavailable refactorings to "selection-in-codesnippet" to avoid any confusions. What do you think?

[Wanderer777]

Yes, the names of the added attributes should not conflict with any attribute names that already exist in Eclipse. I am not sure about using dashes in an atribute's name, but I think it should not cause any problems.

[vazexqi]

I am confused. I thought we identified this while we were coding in d47986bfa617cc2860d32d1970ee31e8a617ca66. That is why we created

/**
     * The offset of the selected text within the code snippet that contains it.
     */
    public static final String ATTRIBUTE_SELECTION_OFFSET= "selection-offset"; //$NON-NLS-1$

in RefactoringDescriptor so we could use that new attribute and leave the original one alone.

[reprogrammer]

@vazexqi,

You are right. I had forgot the "selection-offset" attribute. Nevertheless, I think the method WatchedJavaRefactoring.getSelection should change to return the offset and length of the selection with respect to the whole document rather than returning the selected text. If we make this change, we will no longer need to set the "selection" attribute in populateRefactoringSpecificFields because we will set this attribute populateInstrumentationData.

Besides, I still think that "selection-in-codesnippet" is a better name than "selection-offset" for it coveys that the selection is relative to the captured code snippet.

[reprogrammer]

We had added the following constant declaration to RefactoringDescriptor:

/**
 * The attribute for the selected text in a refactoring.
 */
public static final String ATTRIBUTE_SELECTION= "selection"; //$NON-NLS-1$

There are two problems with this attribute. First, an attribute with the same name had been already defined in JavaRefactoringDescriptor.

/**
 * Predefined argument called <code>selection</code>.
 * <p>
 * This argument should be used to describe user input selections within a text file. The value
 * of this argument has the format "offset length".
 * </p>
 */
protected static final String ATTRIBUTE_SELECTION= "selection"; //$NON-NLS-1$

Second, the formats and the actual contents the two attributes accept are different. The selection attribute in RefactoringDescriptor contains the selected text. But, the "selection" attribute in the JavaRefactoringDescriptor contains the offset and length of the selection. I suggest to rename the "selection" attribute in RefactoringDescriptor to "selection-text".

[Wanderer777]

I agree that "selection-in-codesnippet" is a better name.

As about setting "selection" attribute in populateInstrumentationData instead of populateRefactoringSpecificFields, I think that it is better to keep the existing attributes functionality where it was originally. Besides, this "selection" attribute is not part of the instrumentation data, so it is not clear why it should be populated in populateInstrumentationData.

[reprogrammer]

@Wanderer777,

I agree with you that "selection" attribute is not part of the instrumentation data. This is exactly the problem. The "selection" attribute in populateInstrumentationData should be renamed to "selection-text" so that it doesn't conflict with the original "selection" attribute.

[reprogrammer]

I need to change what I said before:

Nevertheless, I think the method WatchedJavaRefactoring.getSelection should change to return the offset and length of the selection with respect to the whole document rather than returning the selected text. If we make this change, we will no longer need to set the "selection" attribute in populateRefactoringSpecificFields because we will set this attribute populateInstrumentationData.

I've realized that the "selection" attribute in populateInstrumentationData is different from the one in populateRefactoringSpecificFields. Specifically, the "selection" attribute in populateInstrumentationData should become "selection-text" and I have to restore the statements that set the "selection" attribute in populateRefactoringSpecificFields.

[reprogrammer]

b97a6bd145066ab86c667dd187399aa6cda186cd restored back the "selection" attribute that had been missing from some of the refactorings whose classes extend WatchedJavaRefactoring.