Improve UI for rename argument/temporary variable and add tests by Biljcica · Pull Request #19846 · pharo-project/pharo

Biljcica · 2026-07-01T11:50:06Z

Copied StRefactoringPreviewPresenter and modified the UI to support renaming arguments/temporary variables. The updated view allows entering a new name, triggering the rename action, and displaying a preview of changes based on the selected scope. Added unit tests to verify each part of the functionality and introduced missing tests for ReLocalNameConflictCondition.

request-info · 2026-07-01T11:50:14Z

This issue has either a default title or empty body. We would appreciate it if you could provide more information. Note: I am not a very intelligent bot, I can only react to new comments. Please add a comment for me if you update the body or title.

balsa-sarenac · 2026-07-01T12:05:51Z

Thanks! Feel free to update description and explain what you did and optionally why!
In the reviewers section, you can find people you want to take a look before we get this merged, then we'll be notified, and will review the code. Happy to help with your first contribution!

balsa-sarenac · 2026-07-01T12:06:47Z

@Ducasse we want to leave this for Pharo15, right?

Ducasse · 2026-07-01T20:09:23Z

@Ducasse we want to leave this for Pharo15, right?

This is my impression like that less stress :)

Ducasse · 2026-07-01T20:10:16Z

I will change the branch when it is ready so that we have less git magic.

balsa-sarenac

This is good progress, awesome to see tests alongside!

let's iterate and try to clean the noise, there are a lot of changes/additions that are not needed. I tried to flag them with inline comments. to get this merged, we ideally want minimal code to introduce, and remove the dead code/unused code. we can discuss if something is not clear

it would be nice if for next iteration to clean the leftover halts and new empty lines in random methods (just brings noise and unnecessary history)

balsa-sarenac · 2026-07-02T17:51:24Z

-	| temp |
-	temp := 35.
+	| tmp1 |
+	tmp1 := 35.

-	^ temp
+	^ tmp1


testing leftovers

balsa-sarenac · 2026-07-05T07:20:42Z

+{ #category : 'initialization' }
+ReCompositeChange >> resetChanges [
+
+	changes := OrderedCollection new
+]
+


as discussed, let's try to do it with new model for each update, instead of cleaning the model before generating new changes
we can compare them afterwards and pick better one

balsa-sarenac · 2026-07-07T07:36:47Z

+My precondition verifies that the new name is a valid variable name and not an existing instance or a class variable name
+"
+Class {
+	#name : 'ReRenameArgumentOrTemporaryRefactoring2',


What is different from original refactoring?
Or - what you needed to change and why? - could we use the existing refactoring instead?

balsa-sarenac · 2026-07-07T07:50:30Z

+]
+
+{ #category : 'accessing' }
+ReRenameArgumentOrTemporaryDriver2 >> gatherUserInput [


let's try to clean the driver of methods it is not using, just so it's easier to understand. I'm even thinking of creating a superclass of InteractionDriver (like a Driver), and have just the entrypoint and minimal things we need - like runRefactoring and hooks we use. we can discuss this, it will remove noise for you here, and since it's new way of doing drivers, doesn't make sense to try to force it and avoid all existing defined API

balsa-sarenac · 2026-07-07T07:53:11Z

+
+"
+Class {
+	#name : 'SycRefactoringPreviewPresenter2',


maybe rename this to SycRenameArgumentOrTemporaryPresenter or something similar, since this is now specific to this refactoring

balsa-sarenac · 2026-07-26T14:52:15Z

@Biljcica we should change base branch (to which we merge) to Pharo15, as that is the new default, this will go in the next release.

… and model on each rename

balsa-sarenac · 2026-07-28T13:41:57Z

@Biljcica For behavior-preserving precondition, we have: overriding instance/class variables with the same name.
What I suggest is to below applicability validation, we show breaking change validation:

[ field to insert new name ] [update button]
- applicability validation here -
- behavior-preserving validation here -

We just need to make it nicely visible, let's touch base after you do this to see how it works/behaves

…e button

Biljcica · 2026-08-14T14:02:58Z

@balsa-sarenac I’ve implemented the behavior-preserving precondition and added the corresponding tests. I also fixed the affected tests. Let me know what you think! :)

balsa-sarenac

Nice work! It's getting closer to its final shape! Let's iterate now to clean the code and get it ready for merging!

balsa-sarenac · 2026-08-17T06:32:42Z

+{ #category : 'scripting api - executing' }
+ReAbstractTransformation >> generateTransformChanges [
+	"Prepare, check the preconditions, perform the actual transformation (i.e., creating a list of changes that can be accessible using the changes message), and return the changes. This method should not be invoked from drivers since drivers usually check in finer grained mode the preconditions - and that this stage they already checked the preconditions."
+
+	self prepareForExecution.
+	self privateTransform.
+	^ self changes
+
+
+]
+


what's missing in the current API that you need to do this? This is essentially avoiding preconditions - we had this before and removed it since it's dangerous in any place. If you want something like this -> do the steps yourself in the place you want to do it, then we can see how to improve and refactor so this might not be needed at all.

balsa-sarenac · 2026-08-17T06:33:33Z

similar question, why did we fork this refactoring? (the PR is about rename and this is remove?) anything different than the original?

balsa-sarenac · 2026-08-17T06:47:21Z

 	previewUpdateDelay ifNotNil: [ previewUpdateDelay unschedule ].

-	previewUpdateDelay := Delay forMilliseconds: 350.
+	previewUpdateDelay := Delay forMilliseconds: 300.


comment on why 300 would be nice

balsa-sarenac · 2026-08-17T06:48:21Z

you've probably mistakenly removed these?

balsa-sarenac · 2026-08-17T06:48:37Z

    (definingNode exactNodeDefines: variableName) ifTrue: [
        violators addIfNotPresent: variableName ].

+	"check if there is exting temp either above or below in AST"


Suggested change

"check if there is exting temp either above or below in AST"

"check if there is existing temp either above or below in AST"

balsa-sarenac · 2026-08-17T07:09:30Z

+		ifFalse: [
+			(topLayout children includes: behaviorPanel)
+				ifTrue: [ 
+					topLayout remove: behaviorPanel.
+					self rebuildLayout ] ]


Maybe:

Suggested change

ifFalse: [

(topLayout children includes: behaviorPanel)

ifTrue: [

topLayout remove: behaviorPanel.

self rebuildLayout ] ]

ifFalse: [ self hideBehaviorPane ]

balsa-sarenac · 2026-08-17T07:10:01Z

+		ifTrue: [
+			behaviorMessageLabel label: driver checkBehaviorPreservingPreconditions.
+
+			(topLayout children includes: behaviorPanel)
+				ifFalse: [ topLayout add: behaviorPanel expand: false ] ]


maybe:

Suggested change

ifTrue: [

behaviorMessageLabel label: driver checkBehaviorPreservingPreconditions.

(topLayout children includes: behaviorPanel)

ifFalse: [ topLayout add: behaviorPanel expand: false ] ]

ifTrue: [ self addBehaviorPane ]

balsa-sarenac · 2026-08-17T07:11:11Z

looks like this was mistakenly removed?

balsa-sarenac · 2026-08-17T07:11:52Z

What's this file? An older version?

balsa-sarenac · 2026-08-17T07:13:19Z

also this file looks like old version that is still here. can you do a pass and remove things you don't use that are still here, let's try not to bring noise and dead code in

request-info Bot added the more-information-needed label Jul 1, 2026

Biljcica marked this pull request as draft July 1, 2026 11:50

Ducasse requested a review from balsa-sarenac July 1, 2026 20:08

Biljcica marked this pull request as ready for review July 2, 2026 12:33

balsa-sarenac suggested changes Jul 7, 2026

View reviewed changes

Biljcica added 6 commits July 28, 2026 13:35

Implement argument and temporary variable renaming

b3fb42e

Add tests for ReLocalNameConflictCondition

5349ccf

Add tests for rename temporary/argument refactoring

9f172dd

Update message in ReLocalNameConflictCondition to make tests pass

5c3bf45

Fix rename refactoring preview by creating a new refactoring instance…

d64511c

… and model on each rename

Enable executing rename and apply actions with Enter key

619fe89

Biljcica force-pushed the week1 branch from d818677 to 619fe89 Compare July 28, 2026 11:40

Biljcica changed the base branch from Pharo14 to Pharo15 July 28, 2026 11:41

Biljcica added 5 commits August 7, 2026 23:03

Add behavior-preserving conditions

829d907

Improve behavior preconditions UI

fe6b5c0

Rebuild layout after removing behavior preconditions and remove Renam…

265329b

…e button

Fix display issues

caf7c11

Add behavior precondition tests and fix failing tests

f5fee58

balsa-sarenac suggested changes Aug 17, 2026

View reviewed changes

	"check if there is exting temp either above or below in AST"
	"check if there is existing temp either above or below in AST"

Uh oh!

Uh oh!

Conversation

Biljcica commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

request-info Bot commented Jul 1, 2026

Uh oh!

balsa-sarenac commented Jul 1, 2026

Uh oh!

balsa-sarenac commented Jul 1, 2026

Uh oh!

Ducasse commented Jul 1, 2026

Uh oh!

Ducasse commented Jul 1, 2026

Uh oh!

balsa-sarenac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balsa-sarenac commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balsa-sarenac commented Jul 28, 2026

Uh oh!

Biljcica commented Aug 14, 2026

Uh oh!

balsa-sarenac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Biljcica commented Jul 1, 2026 •

edited

Loading

balsa-sarenac commented Jul 26, 2026 •

edited

Loading