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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Tools/BlockClosure.extension.st
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Extension { #name : 'BlockClosure' }

{ #category : '*Tools' }
BlockClosure >> browse [

^ compiledBlock browse
]
20 changes: 20 additions & 0 deletions src/Tools/CompiledBlock.extension.st
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
Extension { #name : 'CompiledBlock' }

{ #category : '*Tools' }
CompiledBlock >> browse [
| method |

method := self method.

method isInstalled
ifTrue: [
^ Smalltalk tools browser
openOnClass: self methodClass
selector: self selector
highlight: self sourceNode sourceCode ]
ifFalse: [
^ Smalltalk tools browser
openOnClass: self methodClass
selector: self selector ]
Comment on lines +15 to +18
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 •
edited
Loading

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.


]