Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CodeGeneration and GetterSetterUtil from o.e.jdt.core.manipulation. #821

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Oct 4, 2018

  • With http://eclip.se/539200 complete, we can can reduce copies for
    classes like CodeGeneration and GetterSetterUtil.
  • This change brings JDT-LS back in line with JDT Core with respect to
    some differences that occured during copying of these classes
    • JDT-LS hard codes StubUtility.useIsForBooleanGetters(..) and
      StubUtility.useThisForFieldAccess(..) . Now the default JDT Core
      values will be used
    • An additional line delimeter is added after the auto-generated
      setter method's comment stub. Although this required adjusting
      some test cases, it doesn't seem to affect "real" usage because the
      code formatter is applied to the resulting stub.

Signed-off-by: Roland Grunberg [email protected]

@rgrunber
Copy link
Contributor Author

rgrunber commented Oct 4, 2018

Ways in which some methods will be subtly different in GetterSetterUtil when the copied code is removed. The '-' (minus) is what jdt.core.manipulation is using and the '+' (plus) is what your copied code has. I could look into maintaining your current options as I think they could just be set with preferences.

public static String getGetterName(IField field, String[] excludedNames) throws JavaModelException {
 -       boolean useIs= StubUtility.useIsForBooleanGetters(field.getJavaProject());
 -       return getGetterName(field, excludedNames, useIs);
 +       return getGetterName(field, excludedNames, true);
    }

public static String getGetterName(IVariableBinding variableType, IJavaProject project, String[] excludedNames, boolean isBoolean) {
-       boolean useIs= StubUtility.useIsForBooleanGetters(project) && isBoolean;
-       return getGetterName(project, variableType.getName(), variableType.getModifiers(), useIs, excludedNames);
+       return getGetterName(project, variableType.getName(), variableType.getModifiers(), false, excludedNames);
    }

public static String getSetterName(IJavaProject project, String fieldName, int flags, boolean isBoolean, String[] excludedNames) {
-       boolean useIs= StubUtility.useIsForBooleanGetters(project);
-       return NamingConventions.suggestSetterName(project, fieldName, flags, useIs && isBoolean, excludedNames);
+       return NamingConventions.suggestSetterName(project, fieldName, flags, false, excludedNames);
    }

In getSetterStub(..)

-       boolean useThis= StubUtility.useThisForFieldAccess(project);
-       if (argname.equals(fieldName) || (useThis && !isStatic)) {
+       if (argname.equals(fieldName) || !isStatic) {

@@ -1290,7 +1290,7 @@ public void testCompletion_getter() throws Exception {
assertNotNull(resolvedItem.getTextEdit());
assertTextEdit(2, 4, 7, "/**\n" +
" * @return the strField\n" +
" */\n" +
" */\n\n" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this extra line is definitely wrong. Eclipse doesn't have it. Where is it coming from?

Copy link
Contributor Author

@rgrunber rgrunber Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GetterSetterUtil, getGetterStub(), and getSetterStub(), The JDT version has it and JDT-LS removed it.

if (comment != null) {
  buf.append(comment);
- buf.append(lineDelim);
}

However, upon actually testing (create a field, hover over it, create getter/setters), the result is the same as before. This is why I was guessing that everything is ultimately fed to a formatter which corrects the issue. In fact, in GetterSetterCompletionProposal#updateReplacementString(..) of JDT-LS, you can see CodeFormatterUtil.format(..) called right after the stub is returned.

@fbricon
Copy link
Contributor

fbricon commented Oct 5, 2018

I don't see the code being reformatted. This is not good:
new-getter

@rgrunber
Copy link
Contributor Author

rgrunber commented Oct 5, 2018

I don't see the code being reformatted. This is not good:

Thanks for trying it out. Maybe the Eclipse Generic Editor is doing something in addition. Either way, I'll look into fixing it (ie. restoring the tests as they were before)

@rgrunber
Copy link
Contributor Author

rgrunber commented Oct 12, 2018

I don't see the code being reformatted. This is not good:

This is happening due to a difference between JDT UI template and JDT LS template (note the additional newline in JDT-LS).

So JDT UI added the newline in the code, after the comment, and JDT-LS added it to the template. Now that we're using the JDT UI (core.manipulation) logic, we can remove the newline from the template. The fix is simple but I'll have to go through and see if this breaks any other tests.

UPDATE : I had to remove the string = string.substring(0, string.lastIndexOf(lineDelimiter)); in SelfEncapsulateFieldRefactoring as without the newline at the very end in the template, these were destroying the template. This actually makes it more similar to JDT UI's SelfEncapsulateFieldRefactoring.

@fbricon
Copy link
Contributor

fbricon commented Oct 15, 2018

@rgrunber can you please rebase against master? This conflicts with last commit from @jjohnstn

- With http://eclip.se/539200 complete, we can can reduce copies for
classes like CodeGeneration and GetterSetterUtil.
- This change brings JDT-LS back in line with JDT Core with respect to
some differences that occured during copying of these classes
  - JDT-LS hard codes StubUtility.useIsForBooleanGetters(..) and
    StubUtility.useThisForFieldAccess(..) . Now the default JDT Core
    values will be used

Signed-off-by: Roland Grunberg <[email protected]>
@fbricon fbricon merged commit a38cef7 into eclipse-jdtls:master Oct 16, 2018
@fbricon
Copy link
Contributor

fbricon commented Oct 16, 2018

Thanks @rgrunber !

@fbricon fbricon added the debt label Oct 16, 2018
@fbricon fbricon added this to the Mid October 2018 milestone Oct 16, 2018
@rgrunber rgrunber deleted the bug-539200 branch October 25, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants