CodeCrictics in Spec2 port (add new classes using Spec2)

[AleCossioCh]

Fixes issue #4026

Changes:

1. Deprecated the classes of this package 'Tool-CriticBrowser' (these were the classes: CriticBrowser, CriticBrowserRulesGroup, CriticToolbar, CriticWorkingConfiguration, CriticsCache, ResetWindow, SelectBrowser, SelectPackageBrowser, SelectRuleBrowser, SingleCodeCriticResultList)

2. Added the following classes: CBRewriteCritiqueChangesBrowser, CBUnifiedDiffChangesMorph, CBUnifiedDiffJoinSection for the option to automatically resolve a critique, when the critique has transform.

3. Updated the UI to select packages and rules, these are the new classes: CBSelectPackage, CBSelectRule.

4. Solved some issues during the migration, some of them were reported as the issues CodeCritics - Update button wrong behavior #9449 and CodeCritics - Add other options to Code comparision #9307.

5. Updated UI in the flow and usability of the tool in the CBCritiqueBrowser class.

6. Add package: 'Tool-CriticBrowser-Tests'

[Ducasse]

Thanks a LOT.
I will have a look.

[Ducasse]

Hi alejandra

First this is super cool :).
Thanks a lot for this!

Here is a list of possible improvements.

• I like to name presenter with presenter at the end.

• Since all the new tools in Pharo starts with St it would be good that all the presenters for the new tools
  are following this convention.

• You could package the code in package named: NewTool-CodeCritics like that we can include it once it is done in NewTools of Pharo.

Now some details

• I could not understand why we have deprecated classes.

• May be
  CBSelectRule should be named
  StCritiquePackageSelectorPresenter
  CBSelectRule
  StCritiqueRuleSelectorPresenter

Check for the critic vs. Critique (the PhD of Yuriy should use the right one)

CBSingleCodeCritiqueResultList
 	StSingleCritiqueResultListPresenter

---

setUp
super setUp
empty here

is not needed

copy1
^ copy
It means that copy is not really a good name since it overrides copy from Object

———
For the colors we should ask esteban
May be we should ask the StApplication or the style.

we should avoid
defaultColor
^ Color red

---

formatting looks bad

applyChangesInUnified
changeTree selectedItem ifNotNil: [
diffView := SpMorphPresenter new morph: (CBUnifiedDiffChangesMorph
from: changeTree selectedItem oldVersionTextToDisplay
to: changeTree selectedItem textToDisplay).
self update].

newUnifiedRadioButton
^ self newRadioButton
label: 'Unified Diff';
state: false.

newSplitRadioButton
^ self newRadioButton
label: 'Split Diff';
state: false.

connectPresenters

super connectPresenters.
	changeUnifiedOption whenActivatedDo: [ self applyChangesInUnified ].
	changeSplitOption whenActivatedDo: [ self applyChangesInSplit ].
changeUnifiedOption state: false.
changeSplitOption state: true.

banRule

| crit |
					  crit := criticsModel selectedItem.
					  crit guidedBan

rationaleOfRule

| crit |
					  crit := criticsModel selectedItem.
					  crit popDescriptionUp.
					  ReSystemAnnouncer uniqueInstance
						  notifyCritique: crit
						  descriptionViewedFor: crit sourceAnchor entity 

——
May providing a printOn: method on
CBCritiqueBrowsweRulesGroup

(should be renamed CBCritiquesRuleGroup).

———

We should check this

setTextModelForClassOrMethod: aCritique

| entity |
entity := aCritique sourceAnchor entity.
self flag:
	'Another hack. We need a better way to display entities (because now we may get not only classes and methods. Maybe something like GTInspector presentations could work)'.
sourceCodeModel text: ((entity respondsTo: #definition)
		 ifTrue: [ entity definitionString ]
		 ifFalse: [ entity asString ]).
sourceCodeModel behavior: (entity isCompiledMethod
		 ifTrue: [ entity methodClass ]
		 ifFalse: [ nil ]).
aCritique sourceAnchor providesInterval ifTrue: [ 
	sourceCodeModel selectionInterval: aCritique sourceAnchor interval ]

——

We should check because may be Spec2 by using TaskIt can do a better job.

updateList

self criticsOf: rule.
thread ifNotNil: [ thread terminate ].
thread := [ UIManager default defer:  [ criticsModel updateList ]] fork.

——
addFalsePositiveRule: aRule forPackage: aPackage

| fp |
(falsePositiveRules includesKey: aPackage) 
	ifFalse: [ falsePositiveRules at: aPackage put: Set new ].
(falsePositiveRules at: aPackage) add: (aRule class uniqueIdentifierName). 
fp := (critics at: aRule ifAbsent: [^ self])
			select: [ :c |  (self packageOf: c) package name = aPackage packageName ].	
fp do: [ :c | self addFalsePositive: c forRule: aRule ]

=>

addFalsePositiveRule: aRule forPackage: aPackage

| fp |
(falsePositiveRules includesKey: aPackage) 
	ifFalse: [ falsePositiveRules at: aPackage put: Set new ].
(falsePositiveRules at: aPackage) add: aRule class uniqueIdentifierName. 
fp := (critics at: aRule ifAbsent: [^ self])
			select: [ :c |  (self packageOf: c) package name = aPackage packageName ].	
fp do: [ :c | self addFalsePositive: c forRule: aRule ]

---

I would do the changes above and afetr we can
have a look at the usability

- the style should be revisited
because we do not understand that the code pane display the potential changes

[AleCossioCh]

Thank you for your feedback Steph! I will work on this points ;)

[AleCossioCh]

I will do the PR with the new changes in NewTools :)