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

618-Bloc-Main-Thread-should-be-stopped-and-restarted-in-shutdownstartup #619

Conversation

tesonep
Copy link
Contributor

@tesonep tesonep commented Oct 10, 2024

  • Adding support for stoping and starting the Bloc thread during image startup / shutdown
  • Recovering open spaces in the same place
  • Fixing Tests
  • Showing an space that has been hide should work

…e startup / shutdown

- Recovering open spaces in the same place
- Fixing Tests
- Showing an space that has been hide should work
Copy link
Collaborator

@tinchodias tinchodias left a comment

Choose a reason for hiding this comment

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

@tesonep I put some comments but I consider okay to merge and then take eventually actions.
I'll wait until tomorrow

{ #category : #'startup - shutdown' }
BlSpace >> rememberVisibleStatus [

previousVisibleStatus := self isVisible
Copy link
Collaborator

Choose a reason for hiding this comment

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

I trust in your solution, but I didn't realize yet why isVisible is not enough, and we need a new flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need to store the flag. Once the real window is destroyed / do not exist in the starting image, the window is always not visible. We need to know how it was before saving the image. Thanks for the review. I tried to avoid it but the space only delegates this behaviour to the real window.

{ #category : #'startup - shutdown' }
BlSpaceManager >> start [

spaces do: [ :aSpace | aSpace ensureWindowOpen ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This class has the only sends of ensureWindowOpen and rememberVisibleStatus. Maybe previousVisibleStatus flag fits better here in BlSpaceManager instead of BlSpace. It is here where this previousVisibleStatus is stored and restored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The visible status is of the window (aka the space) if we put the flag in the space manager we need a collection of flags (one per each space).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. We need to store the state of the open windows if we want to reopen in the same place.

@tinchodias
Copy link
Collaborator

Fixes #618

@tesonep
Copy link
Contributor Author

tesonep commented Oct 11, 2024

@tesonep I put some comments but I consider okay to merge and then take eventually actions. I'll wait until tomorrow

👍 Thanks!

@tinchodias tinchodias merged commit fc7b72d into dev Oct 12, 2024
12 checks passed
@tinchodias tinchodias deleted the 618-Bloc-Main-Thread-should-be-stopped-and-restarted-in-shutdownstartup branch October 12, 2024 01:48
Comment on lines -357 to +375
aSpace isOpened ifTrue: [ ^ self ].
aSpace isOpened ifTrue: [
"If it has been already opened, just unhide it"
aSpace hostSpace show.
^ self ].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to review this change together with #636. Maybe what needed to happen in this case is to attach the space to the universe as if it was a host switch.

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