Conversation
|
Let us know when this is ready for integration |
balsa-sarenac
left a comment
balsa-sarenac
left a comment
There was a problem hiding this comment.
This is awesome, very close to merge, all comments are minor. Good job!
| self | ||
| should: [ refactoring checkApplicabilityPreconditions ] | ||
| raise: Error | ||
| ] |
There was a problem hiding this comment.
Better to be explicit here:
| self | |
| should: [ refactoring checkApplicabilityPreconditions ] | |
| raise: Error | |
| ] | |
| self | |
| should: [ refactoring checkApplicabilityPreconditions ] | |
| raise: RefactoringError | |
| ] |
| refactoring := driver instantiateRefactoring. | ||
| refactoring newName: driver newName . | ||
|
|
||
| self assert: driver checkApplicabilityPreconditions isNil |
There was a problem hiding this comment.
can we have a better check for this? I'm not even sure isNil is fully correct here?
There was a problem hiding this comment.
Replaced the isNil check with applicabilityPreconditionsAreSatisfied to make the intention of the test clearer.
| driver newName: #NewClassName. | ||
| driver createAndSetNewRefactoring. | ||
|
|
||
| presenter updateBehaviorPanelVisibility: 'NewClassName'. | ||
|
|
There was a problem hiding this comment.
Why do we need to send the new name both to the driver and to the presenter?
There was a problem hiding this comment.
The new name is now handled directly by the presenter, avoiding the need to set it on the driver as well.
| failedCondition := refactoring applicabilityPreconditions | ||
| detect: [ :condition | condition check not ] | ||
| ifNone: [ nil ]. |
There was a problem hiding this comment.
We already have this implemented:
| failedCondition := refactoring applicabilityPreconditions | |
| detect: [ :condition | condition check not ] | |
| ifNone: [ nil ]. | |
| failedCondition := refactoring failedApplicabilityPreconditions. |
| detect: [ :condition | condition check not ] | ||
| ifNone: [ nil ]. | ||
|
|
||
| ^ failedCondition ifNotNil: [ :condition | condition errorString ] |
There was a problem hiding this comment.
Possibly this as well with applicabilityViolationMessage? If not, we should think of introducing one.
| ^ self refactoring breakingChangePreconditions | ||
| anySatisfy: [ :each | each check not ] |
There was a problem hiding this comment.
| ^ self refactoring breakingChangePreconditions | |
| anySatisfy: [ :each | each check not ] | |
| ^ self refactoring failedBreakingChangePreconditions isEmpty |
| StRenameClassPreviewPresenter >> performChanges [ | ||
| "the following behavior should not be kept in the UI. | ||
| It should be moved to the driver or the refactoring itself because the driver is a model of interaction. | ||
| May be this should be in the refactoring." | ||
|
|
||
| [ ReChangeManager instance | ||
| performChanges: selectedChanges ] | ||
| asJob | ||
| title: 'Refactoring'; | ||
| run |
There was a problem hiding this comment.
Agreed. It should be something like this:
| StRenameClassPreviewPresenter >> performChanges [ | |
| "the following behavior should not be kept in the UI. | |
| It should be moved to the driver or the refactoring itself because the driver is a model of interaction. | |
| May be this should be in the refactoring." | |
| [ ReChangeManager instance | |
| performChanges: selectedChanges ] | |
| asJob | |
| title: 'Refactoring'; | |
| run | |
| StRenameClassPreviewPresenter >> performChanges [ | |
| driver performChanges: selectedChanges. |
Then Driver should have something like:
ReRenameClassDriver2 >> performChanges: selectedChanges
[ ReChangeManager instance
performChanges: selectedChanges ]
asJob
title: 'Refactoring';
run
BUT, this can be also delegated to refactoring, if we want the driver to be the sole controller:
ReRenameClassDriver2 >> performChanges: selectedChanges
refactoring performChanges: selectedChanges
The refactoring already has performChagnes that does exactly what UI was duplicating. So it makes sense to have performChagnes: as well.
This can be done in future PR, let's keep this one Rename Class focued.
|
|
||
| self closeWindow ]. | ||
| self performChanges. | ||
| self updateGlobalActiveScope. |
There was a problem hiding this comment.
not sure why we need this after performing changes? This might be bug in the old system as well. We should check if it works without this and what's the bug if it doesn't work. Also, a separate PR not to be done here.
There was a problem hiding this comment.
Tested without this, and the changes are not performed. After clicking Apply, the name remains unchanged.
| applyButton := aDialogWindowPresenter addDefaultButton: 'Apply' do: [ :presenter | | ||
| self terminateUpdateProcess. | ||
| self accept. | ||
| presenter close ]. |
There was a problem hiding this comment.
here we do presenter close but we also defined closeWindow so it's a bit confusing, but ok, we can clean later
| StRenameClassPreviewPresenter >> refactoring: aRBRemoveMethodDriver [ | ||
| refactoring := aRBRemoveMethodDriver |
There was a problem hiding this comment.
Either change this to driver: or rename argument to aRenameClassRefactoring (i think the first one?)
There was a problem hiding this comment.
Renamed the argument to aRenameClassRefactoring, since a driver: method already exists.
3 participants
Improved the class rename refactoring UI with live validation and preview. The new name is validated as the user types, behavior-preserving preconditions are displayed when applicable, and the preview is updated with the current changes. Added tests for the new behavior.