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

Do not dispose dialogs on close #14456

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tsmaeder
Copy link
Contributor

What it does

Fixes #12093

The about dialog in Theia is reopened and must no be disposed on close. This PR overrides the onCloseRequest method from BaseWidget to not invoke dispose(). In order to minimize impact for clients of the API, I've added an optional parameter to the open() method that controls whether the dialog is disposed when it is closed.
This change might be breaking in the case where clients reuse dialog instances and do not rely on open() to show the dialog. In that case, they will have to dispose the dialog themselves now. That should be a very rare case, though.

Contributed on behalf of STMicroelectronics

How to test

  • Open and close the about dialog at least three times. Make sure there are not messages and exceptions in the browser log.
  • Test behavior of both dismissing and accepting dialogs, for example
    • ConfirmationDialog: I used the "Clear editor history" commnand
    • Add a function breapoint in the "breakpoints" view
    • Make an editor dirty and close it: there is a dialog asking whether to save
      Ensure these scenarios work correctly in the browser and Electron case.

Follow-ups

Review checklist

Reminder for reviewers

Fixes eclipse-theia#12093

The about dialog in Theia is reopened and must no be disposed on close.

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder changed the title Do not dispose dialogs on close. Do not dispose dialogs on close Nov 15, 2024
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder requested a review from msujew November 15, 2024 13:06
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder requested a review from sdirix November 15, 2024 15:12
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

I tested all mentioned scenarios in the browser. There I could not see any errors.

I also tested all mentioned scenarios in Electron. There I was able to reproduce the following error when opening the about dialog multiple times: ERROR Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it. The other test scenarios did not produce any errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

Invalid lifecycle for ReactDialog
2 participants