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

Introduce-method refactoring should suggest a method name #7551

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Jul 6, 2024

if the test field is left empty, it creates the following problem:

  • cancel button has focus, ok button is disabled until validation
  • method name validation is delayed by 200ms which can cause users to cancel the hint after filling out the method name and pressing enter quickly, giving the wrong impression of a not functioning refactoring action

This can be avoided by initializing the test field with a method name.

bonus: Fixed an issue where a name conflict did not disable the introduce field dialog's ok-button

how to test:

  • select System.out.println();
  • alt+enter, select introduce method
  • type something and hit enter quickly

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) hints ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jul 6, 2024
@mbien mbien added this to the NB23 milestone Jul 6, 2024
@neilcsmith-net
Copy link
Member

Seems an OK quick fix. Those should ideally be refactored to use validity support in dialogs? Problem seems caused by bypassing that and trying to achieve it manually? https://bits.netbeans.org/22/javadoc/org-openide-dialogs/org/openide/NotifyDescriptor.html#setValid-boolean-

Could also look at setting btnCancel.setDefaultCapable(false); ??

@mbien
Copy link
Member Author

mbien commented Jul 9, 2024

Could also look at setting btnCancel.setDefaultCapable(false); ??

this would be better than cancelling the dialog, however it could create the situation that pressing enter would do nothing the first time its pressed. If the user tries again 200ms later it would work - still not ideal

The trick here is to have the initial state of the dialog to be valid, like the other "introduce" dialogs. Those are able to suggest sensible input values, with the introduce-method refactoring this is more difficult.

This does also validate the default right away without delay, so it will disable the OK button if there is a conflict.

@neilcsmith-net
Copy link
Member

however it could create the situation that pressing enter would do nothing the first time its pressed

If the OK button is disabled, then pressing ENTER should do nothing. No issue with prefilling with a valid value. But the user should still need to use ESC to cancel the dialog.

@mbien
Copy link
Member Author

mbien commented Jul 9, 2024

If the OK button is disabled, then pressing ENTER should do nothing.

the problem is that OK is disabled. You type something and it won't apply. The fact that cancel happens to be selected is just a followup issue, which is already fixed by having a default value and initial validation. I can set btnCancel.setDefaultCapable(false) but this won't really change anything on the original problem.

edit: the other "introduce" dialogs don't set that flag either, if we should change it we should change it for all comparable dialogs and not single out this one here.

@mbien mbien removed the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Jul 16, 2024
@mbien mbien requested review from matthiasblaesing and sdedic July 16, 2024 16:15
@mbien
Copy link
Member Author

mbien commented Jul 16, 2024

rebase just to refresh since a lot happened in the meantime

@mbien mbien force-pushed the introduce-method-with-valid-default branch 2 times, most recently from e7fe9a9 to a550589 Compare July 17, 2024 20:13
@mbien
Copy link
Member Author

mbien commented Jul 17, 2024

@neilcsmith-net I looked through the hierarchy and added btnCancel.setDefaultCapable(false); to all fixes I found.

@@ -254,6 +253,7 @@ private Preferences getPreferences() {

public void setOkButton( JButton btn ) {
this.btnOk = btn;
changeSupport.run();
Copy link
Member

Choose a reason for hiding this comment

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

On other places, ChangeSupport.run is run off EDT, since validators tend to use Parsing API which might be quite slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is quite fast even in large files but I agree in principle. It is also run on focusLost() etc, so this would require more work.

Copy link
Member Author

@mbien mbien Jul 19, 2024

Choose a reason for hiding this comment

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

so i debugged this and the whole panel initialization runs off-EDT(!), this call included ;)

What I did though to not make it worse: I removed this call to run() again, and triggered (async)validation when the default value is set.

Copy link
Member Author

@mbien mbien Jul 19, 2024

Choose a reason for hiding this comment

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

well. this creates another problem i wanted to avoid. Now I probably going to have to init valid with true, otherwise it would appear to users which press enter quickly as if the keyboard isn't working.

@mbien mbien force-pushed the introduce-method-with-valid-default branch 4 times, most recently from ec2623b to 001eb85 Compare July 19, 2024 19:03
@mbien
Copy link
Member Author

mbien commented Jul 22, 2024

I think I addressed all review comments, if there is more I should do or if I overlooked something please say so. Planning to merge this soon since I don't want to wait till last minute before freeze.

if the test field is left empty, it creates the following problem:

 - cancel button has focus, ok button is disabled until validation
 - method name validation is delayed by 200ms which can cause
   users to cancel the hint after filling out the method name and
   pressing enter quickly, giving the wrong impression of a not
   functioning refactoring action

this can be avoided by initializing the panel with a method name

 - added btnCancel.setDefaultCapable(false) just in case

Fixed an issue where a name conflict did not disable the introduce
field dialog's ok-button
@mbien mbien force-pushed the introduce-method-with-valid-default branch from 001eb85 to cadf7ca Compare July 22, 2024 03:30
@mbien
Copy link
Member Author

mbien commented Jul 24, 2024

planning to merge this Thursday morning UTC a day before freeze.

@mbien mbien merged commit 87a9d8c into apache:master Jul 25, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants