Skip to content

Fix code presenter Browse Implementors - #1923

Merged
jecisc merged 1 commit into
pharo-spec:Pharo14from
AlexisCnockaert:fix-call
Jun 25, 2026
Merged

Fix code presenter Browse Implementors#1923
jecisc merged 1 commit into
pharo-spec:Pharo14from
AlexisCnockaert:fix-call

Conversation

@AlexisCnockaert

Copy link
Copy Markdown
Contributor
  • So now it calls the new method browser, and we clean up the callsites

- So now it calls the new method browser, and we clean up the callsites
@AlexisCnockaert
AlexisCnockaert requested a review from jecisc June 25, 2026 09:04
@jecisc
jecisc merged commit bab244b into pharo-spec:Pharo14 Jun 25, 2026
1 of 2 checks passed
@guillep

guillep commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Hi, this fix is wrong.

Tests are breaking because they were expecting to pass through system navigation and this is now hardcoding Smalltalk, tools, and other stuff again.

I'll revert it.

ifTrue: [ self systemNavigation browse: variable value ]
ifFalse: [ self systemNavigation browseAllImplementorsOf: variableOrClassName ]
ifFalse: [
(Smalltalk tools toolNamed: #messageList) browseImplementorsOfAll: { variableOrClassName } from: self interactionModel behavior ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The right fix should be to pass though system navigation here, as it was before.

If we need to add an argument for the origin class, that should be extended there too.
And maybe we need to support the old method too for some time as a compatibility measure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the cause of the failing tests:

  • SpCodePresenterTest testDoBrowseSenders
  • SpCodePresenterTest testDoBrowseSendersOfClassNameShouldNotFindIfNotInEnvironment
  • SpCodePresenterTest testDoBrowseSendersOfClassNameWhenInPlaygroundButNotAvailable
  • SpCodePresenterTest testDoBrowseSendersOfClassPoolVariableShouldSelectClassPoolVariableUsersWhenInDebuggerButNotAvailable
  • SpCodePresenterTest testDoBrowseSendersOfClassVariableShouldSelectClassVariableUsersWhenInDebuggerButNotAvailable
  • SpCodePresenterTest testDoBrowseSendersOfClassVariableShouldSelectClassVariableUsersWhenInStaticContextButNotAvailable

@AlexisCnockaert

AlexisCnockaert commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Hi, I understand why, I did this because the new method browser works with tabs now. Therefore if someone want to browse implementors in a method browser it will not open as a tab but as a new window since it calls the system navigation.
But looking at the trade-offs it is better to revert this.

@guillep

guillep commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

My point is not that the feature is bad! My point is that we should redo it, just being careful of respecting the architecture and keeping tests green!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants