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.

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

Add CompiledBlock>>#browse and BlockClosure>>#browse #17335

Open
wants to merge 1 commit into
base: Pharo13
Choose a base branch
from

Conversation

iglosiggio
Copy link
Contributor

@iglosiggio iglosiggio commented Nov 4, 2024

This change allows the browse action to be a bit more intuitive. The implementation highlights the text if the method is installed. If the method is not instaled then it may happen that there's not an equivalent block in the installed version, so it just tries to show a method with that name (like CompiledMethod>>#browse).

Comment on lines +15 to +18
ifFalse: [
^ Smalltalk tools browser
openOnClass: self methodClass
selector: self selector ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea, but beware of edge cases that can lead to strange behavior.
If the method does not exist, it opens on the class.
If the block comes from the playground, it opens on the UndefinedObject class.
From an inspector script, it opens on the class of the object being inspected.
While these cases make some sense because they are the "location" where the block came from, no methods are displayed, so this can feel odd (like "how did I end up on the XYZ class when browsing a block?")

Copy link
Contributor Author

@iglosiggio iglosiggio Nov 4, 2024

Choose a reason for hiding this comment

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

Agreed. I don't really know what should be done for those edgecases. The current code will behave similarly to CompiledMethod>>#browse (that's intentional).

I think we have the following cases:

  • Method is installed: Open the browser on that method and highlight the block
  • A method with the same name is installed, contains a block with the same code: Open the browser on that method. Don't highlight anything (here we might want to highlight the code 🤔)
  • A method with the same name is installe, does not contain a block with the same code: Open the browser on that method. Don't highlight anything.
  • Method is not installed, class is installed: [Currently] Open the browser on that class
  • Method is not installed, a class with the same name is installed: Don't know what happens, it should be the same as with CompiledMethod>>#browse
  • Method is not installed, class is not installed: Don't know what happens, it should be the same as with CompiledMethod>>#browse

Copy link
Contributor

Choose a reason for hiding this comment

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

A big difference with CompiledMethods is that they are usually installed in a class, unless we are working at a meta level.
Blocks appear much more often, and they are never installed when we work in a "scripting" context (playground, inspector, when we modify and execute code without saving (debugger, code browser...), etc.).

If the block is installed, browsing the code where it is defined sounds like a good idea.
If not, none of the options seem satisfactory to me.
Also, I personally always interpreted the "browse" action as "browse the class of the object", so I'm already thrown off when "browsing" methods, packages, and classes.

I'm not sure if it's possible to highlight the block when opening the code browser...?
I know it's supported, for example when using Find & Replace, but I've never seen Calypso opened with code pre-highlighted.

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.

2 participants