Community
Participate
Working Groups
3.4 RC1. The 'Generate getter and setter' quick fix forces to add both. I would expect that I can also only generate the getter or the setter. Note that the quick fix calls the 'Encapsulate Field' refactoring.
*** Bug 424799 has been marked as a duplicate of this bug. ***
No need to force me to generate both. Can you please allow one of the input fields to be blank so to skip that generation?
*** Bug 433691 has been marked as a duplicate of this bug. ***
From bug 433691 comment #5: https://git.eclipse.org/r/#/c/25687/ See also bug 433691 comment #2 for the corresponding screenshot of the implementation.
Can you please clarify the implementation. The ticket was to allow either only the setter to be generated or only the getter. It appears the patch allows only the former. I was expecting to be able to just blank out one of the method names. That would be much simpler than what was patched.
(In reply to Paul Benedict from comment #5) > Can you please clarify the implementation. The ticket was to allow either > only the setter to be generated or only the getter. It appears the patch > allows only the former. I was expecting to be able to just blank out one of > the method names. That would be much simpler than what was patched. I do not see the point of generating a setter but not a getter. What use case would have a public setter no getter? My desired functionality would be to keep the setter private and generate a getter. That definitely falls under the umbrella of encapsulation, and is very common pattern in development. Leaving the field blank is not as clear as having an explicit option(checkbox) to suppress. Either way, there should be a way to do it from that dialog. If somebody with commit authority gives me the final say, I will update the patch however requested. This issue resolved would surely be welcome in my day-to-day coding.
Generating only setters is a very common use case using dependency injection. The container will inject the services but there's no need to have the getters since the code can just access the fields once injected. If blanks are not preferred, yes, a checkbox.
OK. I will put a check box to the left of each row. It will be selected by default, and if unchecked, the field will be cleared and the message to the right will update to [no getter/setter generated]. I will also update the field to have disabled behavior if either text box is blank. If the check box is reselected, the default text will be put back in the field. Will this work?
Sounds good to me. I'd like to a screen shot, if possible, when done, just to see how it all settled.
Created attachment 242582 [details] screenshot 1
Created attachment 242583 [details] screenshot 2 Clearing the text box unchecks the button Unchecking the button clears the check box Rechecking the button restore text box to previous value OK?
> update to previous comment Clearing the text box unchecks the button Unchecking the button clears the text box Rechecking the button restores the text box to previous value
I think it's great. From a keyboard perspective, how does the navigation go? I assume the getter is where the caret is initially placed. Do I have to tab past the setter checkbox to get to the setter textfield?
(In reply to Paul Benedict from comment #13) > I think it's great. From a keyboard perspective, how does the navigation go? > I assume the getter is where the caret is initially placed. Do I have to tab > past the setter checkbox to get to the setter textfield? Because clearing the text field (tab to and then delete) basically does the same think as deselecting the checkbox, there is no reason to even focus on the check boxes. The current behavior cycles through the 2 text boxes and the 'configure naming' link. I am not going to change that in this patch for fear of the debate that will follow.
One issue: If the getter or setter is not generated, by default the visibility is still changed to private; This has the potential to break code all over the place.
https://git.eclipse.org/r/#/c/25687/ has been updated.
*** Bug 436271 has been marked as a duplicate of this bug. ***
(In reply to Steven Spungin from comment #16) > https://git.eclipse.org/r/#/c/25687/ has been updated. Review comments: - Please update the patch as per the coding conventions of JDT UI project (update copyright headers of the files and follow JDT UI coding conventions, example: use "a= b" instead of "a = b"). Refer: https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions - String#isEmpty was introduced in Java 6 and JDT UI uses Java 5 => Patch results in compile errors. - Take the following example: public class Foo { static int i= 0; { System.out.println(i); i = 10; i+= 9; i++; } } class XX { { System.out.println(Foo.i); Foo.i = 10; Foo.i += 9; Foo.i++; } } => Uncheck 'Getter name' and click OK. We get: Caused by: java.lang.IllegalArgumentException: Invalid identifier : >< at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:195) at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2202) at org.eclipse.jdt.internal.corext.refactoring.sef.AccessAnalyzer.visit(AccessAnalyzer.java:153) ... => Uncheck 'Setter name' and click OK. We get: Caused by: java.lang.IllegalArgumentException: Invalid identifier : >< at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:195) at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2202) at org.eclipse.jdt.internal.corext.refactoring.sef.AccessAnalyzer.createInvocation(AccessAnalyzer.java:312) at org.eclipse.jdt.internal.corext.refactoring.sef.AccessAnalyzer.visit(AccessAnalyzer.java:224) ... => Please check for other cases also where this could happen. - (In reply to Steven Spungin from comment #15) > One issue: > If the getter or setter is not generated, by default the visibility is still > changed to private; This has the potential to break code all over the place. => If both getter and setter are unchecked, you should disable the operation and show an error message in the wizard to choose getter and/or setter to encapsulate the field. => If either getter or setter is unchecked, then probably we shouldn't change the access modifier of the field to 'private'. - The message "(getter/setter not encapsulated)" doesn't look correct. A field is encapsulated, not its getter/setter. It can probably be changed to "(getter/setter not used)" if the getter/setter already exists, and to "(no getter/setter created)" if the getter/setter doesn't already exist. This will also clear the confusion in the case where these conditions co-exist: * the getter/setter already exists * corresponding getter/setter check box is unchecked * radio button "use setter and getter" is selected Here, the uncheck refers to not using the existing getter/setter for encapsulating the field. Dani/Markus, should we also change the radio button text to "use setter", "use getter" or "use setter and getter" based on the state of check boxes? Also, please suggest if you have better alternatives for the above comments.
Created attachment 244482 [details] screenshot 3: Disable if no actions are selected
=> If either getter or setter is unchecked, then probably we shouldn't change the access modifier of the field to 'private'. How should we handle refactoring in external classes if we are not changing the access modifier? Only replace uses with the accessor we have generated, and leave the other alone?
- The message "(getter/setter not encapsulated)" doesn't look correct. A field is encapsulated, not its getter/setter. It can probably be changed to "(getter/setter not used)" if the getter/setter already exists, and to "(no getter/setter created)" if the getter/setter doesn't already exist. Even if the getter/setter exists, the user may still want to refactor and encapsulate the member. I would suggest keeping the "using existing getter/setter" instead of "getter/setter not used".
>Here, the uncheck refers to not using the existing getter/setter for encapsulating the field. This only refers to the declaring type class. The selected getter/setter will still be generated in other classes regardless of this setting. Even if a getter/setter already exists, the user can create an additional one.
Created attachment 244486 [details] screenshot 4: Warning if member visibilty will not be changed
Generating a getter/setter and encapsulating a field are related functions but not identical. Are we fixing the right part of Eclipse? There is "Generate Getters and Setters" under "Source" and "Encapsulate Fields" under "Refactor". I think we're going down the wrong path here trying to attack the refactoring aspect. This should be to enhance the "Source" dialog instead.
What I think we need is to have two quick fixes. One to generate just the getters/setters ("Source") and another to encapsulate a field ("Refactor"). When I want to generate getters and setters, I wish not to have any refactoring done -- and I want the ability to choose the access modifiers at the same time. So I really think we should present both options to the user.
> I think we're going down the wrong path here trying to attack the refactoring aspect I would rather have a single wizard with the option to encapsulate or not vs. 2 menu items to choose from.
I agree the goal of "Refactor > Encapsulate Field" is to make the field private, so this should not be changed. The quick assist was added in bug 203263 because it was "easy to do", but it didn't get a good name. For the quick fix on an unused private field, "Create getter and setter" is OK, since the field doesn't get encapsulated anyway. But the quick assist that shows up on all field names should really be called "Encapsulate field". (In reply to Paul Benedict from comment #25) > What I think we need is to have two quick fixes. One to generate just the > getters/setters ("Source") and another to encapsulate a field ("Refactor"). Offering all available source actions and refactorings as quick assists defeats one purpose of QAs (that they just show a few selected choices). We'd rather remove the existing QA than add another one for the source action. Checkboxes without a label are problematic and would require workarounds like in OptionsConfigurationBlock#addCheckBoxWithLink(..). E.g. the check state should toggle when the user clicks the text, and then there are the tab ordering and focus issues (comment 14 only tells the Mac viewpoint with restricted accessibility settings; everywhere else, the checkboxes are cumbersome). The checkboxes are not really necessary. Clearing the text fields is good enough and not too hard to discover for a user who looks for this functionality. When e.g. the getter field is cleared, the message behind it should say "(no getter used)". That implies that none is created. If a missing getter/setter will lead to a compile error, then the refactoring can report a non-fatal error. (In reply to Noopur Gupta from comment #18) > Dani/Markus, should we also change the radio button text to "use setter", > "use getter" or "use setter and getter" based on the state of check boxes? Changing the text on-the-fly would be distracting and cause layouting issues. Let's use "us&e getter/setter" as radio message. That puts the words "getter" and "setter" into the right order, and the "/" can mean "and/or".
Before I do any more work on this patch or issue, can I get some feedback regarding a single wizard to provide: 1. The ability to create setters and/or getter 2. An option to refactor usage in the source file, 3. An option to encapsulate the field (making member private and replacing with accessors in external files) If creating a consolidated wizard is -1, I would be +1 on adding 2 quick fix items, one for encapsulating and one for creating getter/setter, and renaming the current quick-fix to encapsulate field. Does anyone have usage date on how often these 2 refactoring are used? I use them 1000s of times a week.
(In reply to Markus Keller from comment #27) > I agree the goal of "Refactor > Encapsulate Field" is to make the field > private, so this should not be changed. Agreed. The goal of encapsulation is not achieved if the field doesn't become private. > The quick assist was added in bug 203263 because it was "easy to do", but it > didn't get a good name. For the quick fix on an unused private field, > "Create getter and setter" is OK, since the field doesn't get encapsulated > anyway. But the quick assist that shows up on all field names should really > be called "Encapsulate field". I think this is how the confusion started. It's really mislabeled. "Generate getter and setter" is another operation in Eclipse; this is about a refactoring operation. I agree we should call it "Encapsulate field" to match what's in the main menus. > (In reply to Paul Benedict from comment #25) > > What I think we need is to have two quick fixes. One to generate just the > > getters/setters ("Source") and another to encapsulate a field ("Refactor"). > > Offering all available source actions and refactorings as quick assists > defeats one purpose of QAs (that they just show a few selected choices). > We'd rather remove the existing QA than add another one for the source > action. I never need the refactoring operation so the current quick assist is meaningless to my work. What I need almost always is the source action. I don't think this will equate to "all available source actions" like you said. :) Here is my proposal: 1) When a field is already private and both getters/setters are not yet generated, propose the "Generate getter and setter" option but make it go to what's used in the Source menu. This is not a refactoring operation. 2) When a field is not private and in use, propose "Encapsulate field" Either one or both will show up based on what can be determined. That should satisfy developer needs pretty well, I think.
(In reply to Steven Spungin from comment #28) We should keep refactorings and Generate... actions apart. They are really different things. > Does anyone have usage date on how often these 2 refactoring are used? I > use them 1000s of times a week. I'm not aware of good usage data, and I'm not sure if the (now out-of-service) Eclipse UDC data can tell us how best to proceed. (In reply to Paul Benedict from comment #29) > Here is my proposal: > > 1) When a field is already private and both getters/setters are not yet > generated, propose the "Generate getter and setter" option but make it go to > what's used in the Source menu. This is not a refactoring operation. > > 2) When a field is not private and in use, propose "Encapsulate field" > > Either one or both will show up based on what can be determined. That should > satisfy developer needs pretty well, I think. I don't think the enablement should depend on the accessibility of the field. Currently, we e.g. show "Encapsulate field" (currently named "Create getter and setter for 'field'...") as quick fix on an unused private field, and that also makes sense. There are too many combinations of private/non-private, unused / used locally / used externally, and it's hard to predict what the user really needs in what situation. If you all think it's fine to show both "Encapsulate field 'fieldname'..." and "Generate getter and setter for 'fieldname'..." as quick assists, then I can live with that.
New Gerrit change created: https://git.eclipse.org/r/135586
Gerrit change https://git.eclipse.org/r/135586 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=14c039ac2bd442a5417436c137e4472f4fb8b99f
I just tried the example from comment #18 and I still get the mentioned exception in master branch: Caused by: java.lang.IllegalArgumentException: Invalid identifier : >< at org.eclipse.jdt.core.dom.SimpleName.setIdentifier(SimpleName.java:243) at org.eclipse.jdt.core.dom.AST.newSimpleName(AST.java:2458) at org.eclipse.jdt.internal.corext.refactoring.sef.AccessAnalyzer.createInvocation(AccessAnalyzer.java:324) ... Please confirm if all comments discussed in this bug are now addressed in the final patch.
New Gerrit change created: https://git.eclipse.org/r/139988
(In reply to Noopur Gupta from comment #33) > Please confirm if all comments discussed in this bug are now addressed in > the final patch. Also, please add N&N entry before closing the bug.
Gerrit change https://git.eclipse.org/r/139988 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=2d1003c23ba7483dad898c8f1333e65e12ae916e
I'll be closing this once I create a N&N entry. I'll file a separate bug for some issues that still should be addressed.
New Gerrit change created: https://git.eclipse.org/r/140313
Gerrit change https://git.eclipse.org/r/140313 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=a94674a6ffac8a4ba8f5694f46d9ceabd9265382
Separate bug to address other issues created, and the N&N entry has been pushed in.
verified for Eclipse 4.12 M1 using I20190408-1800.
