apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

Introduce-method refactoring should suggest a method name #7551

Closed mbien closed 1 month ago

mbien commented 2 months ago

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

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:

neilcsmith-net commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

rebase just to refresh since a lot happened in the meantime

mbien commented 1 month ago

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

mbien commented 1 month ago

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.

mbien commented 1 month ago

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