pharo-vcs / iceberg

Iceberg is the main toolset for handling VCS in Pharo.
MIT License
134 stars 85 forks source link

Reload raised an error because context is nil #1779

Closed Ducasse closed 10 months ago

Ducasse commented 11 months ago
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 commented 11 months ago

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.

Ducasse commented 11 months ago

I was thinking to first add

application 
  ^ StPharoApplication current 

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

daniels220 commented 11 months ago

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 commented 11 months ago

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.