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.

Sign up for GitHub

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

Jump to bottom

Fix so that Rename Method refactoring does not start with a disabled text input #10390

Merged
merged 1 commit into from Nov 29, 2021
Merged

Fix so that Rename Method refactoring does not start with a disabled text input #10390

merged 1 commit into from Nov 29, 2021

Conversation

miguelcamp
Copy link
Contributor

Fix for issue #10332

kasperosterbye
kasperosterbye approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@kasperosterbye kasperosterbye left a comment

Choose a reason for hiding this comment

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

I think it is good to go. I added a few detailed comments which should be simple to correct, but works fine without.

Is there a way for you to add a test to make sure the error do not sneak back in? That would be really nice.

src/SystemCommands-RefactoringSupport/SycMethodNameEditorPresenter.class.st
@@ -346,7 +358,7 @@ SycMethodNameEditorPresenter >> initializeWidgets [
addButton label: '+'.
selectorInput text: methodName selector.
previewResult label: methodName methodName.
selectorInput beNotEditable.
"selectorInput beNotEditable."
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this line should be deleted (not leaving code in comments)

src/SystemCommands-RefactoringSupport/SycMethodNameEditorPresenter.class.st
@@ -136,6 +135,19 @@ SycMethodNameEditorPresenter class >> openOn: aMethod withInvalidArgs: aSet canR
^ temp openBlockedDialogWithSpec
]

{ #category : #specs }
SycMethodNameEditorPresenter class >> openOn: aMethod withInvalidArgs: aSet canRenameArgs: aBoolean1 canRemoveArgs: aBoolean2 canAddArgs: aBoolean3 canEditName: aBoolean4 [
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps some better argument names?

@MarcusDenker MarcusDenker linked an issue Nov 26, 2021 that may be closed by this pull request
Renaming a Method starts with a disabled editbox. #10332
Closed
@Ducasse
Copy link
Member

Ducasse commented Nov 29, 2021

Miguel this is really annoying that we cannot use rename anymore in Pharo.
When you push a change please make sure that it works!
And if you break something (this is ok) makes sure that you react fats to comments so that we can integrate them fast.

@Ducasse
Copy link
Member

Ducasse commented Nov 29, 2021

I will integrate these changes and I hope that they work because to me I cannot use Pharo now.

@Ducasse Ducasse merged commit d27ef08 into pharo-project:Pharo10 Nov 29, 2021
@MarcusDenker MarcusDenker mentioned this pull request Dec 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasperosterbye kasperosterbye kasperosterbye approved these changes

Assignees
No one assigned
Labels
Status: Tests passed please review!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renaming a Method starts with a disabled editbox.
4 participants
@miguelcamp @Ducasse @kasperosterbye @MarcusDenker