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

Reload raised an error because context is nil #1779

Closed
Ducasse opened this issue Dec 10, 2023 · 4 comments · Fixed by #1781
Closed

Reload raised an error because context is nil #1779

Ducasse opened this issue Dec 10, 2023 · 4 comments · Fixed by #1781
Labels

Comments

@Ducasse
Copy link
Collaborator

Ducasse commented Dec 10, 2023

IceTipStandardAction >> basicExecute

	| result |
	 context application
		informUser: self message
		during: [ result := actionBlock cull: self context ].
	successAnnounceBlock 
		ifNotNil: [ Iceberg announcer announce: successAnnounceBlock value ].
	^ result

The problem is that it is unclear how to reach the current application.

@daniels220
Copy link
Contributor

Yes, many users of IceTipStandardAction have this problem. Chasing it down for a bit, it seems many of them have been migrated to a new pattern where the calling IceTipCommand asks for a #newWhateverAction then sends it executeWithContext:, rather than calling a method which makes and immediately evaluates the IceTipStandardAction. See for instance IceTipDeleteBranchCommand>>execute, making use of IceTipBranchModel>>newDeleteAction.

I believe I've managed to chase down all the remaining cases—PR incoming.

daniels220 added a commit to shoshanatech/pharo-iceberg that referenced this issue Dec 16, 2023
…hContext: pattern (fixes pharo-vcs#1779)

Fixes nil DNU #application errors for package reload/remove/unload, tag delete, and method version load.
Also remove extraneous IceRepositoryModified when the action already has #onSuccessRepositoryModified.
Add missing ^return in IceTipVersionHistoryModel>>retrieveHistory

Remove IceTipStandardAction>>execute: as execution without a context will ~always fail and the pattern is to assign the action first, then the context at time of execution. No references remain in Iceberg packages, and in fact all remaining references are self-sends, so clearly aren't sent to IceTipStandardAction.
@Ducasse
Copy link
Collaborator Author

Ducasse commented Dec 16, 2023

I was thinking to first add

application 
  ^ StPharoApplication current 

in the superclass of action.
Then later we can pass an application.

@daniels220
Copy link
Contributor

I thought of something like that, but by the time I determined what the right default value for application would even be (StPharoApplication current sounds right, yes), I had discovered that the vast majority of commands were already using the #newXAction + #executeWithContext: pattern. And all the cases I found were easily migrated to that pattern, so no need for a hard-coded default.

@Ducasse
Copy link
Collaborator Author

Ducasse commented Dec 16, 2023

Ah then even better. Now since we want to remove the dependencies to UIManager the "UI" should be passed via the Application. This way we will one single point of control.

guillep added a commit that referenced this issue Jan 19, 2024
Update users of IceTipStandardAction to use #newXAction + #executeWithContext: pattern (fixes #1779)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants