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

Introduce IDE class that manages all necessary class instances #882

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

kmagiera
Copy link
Member

@kmagiera kmagiera commented Jan 7, 2025

This PR restructures the logic for creating and accessing some core extension components like Project, DeviceManager, etc.

Before they were instantiated in webview panel or tabview controller. In this PR we are introducing a new top level class IDE that controls the lifecycle of all the listed core components and hooks them up properly as some instances require other instances to exists.

The main reason for this change is to lay a groundwork for #878 where we are introducing additional webview panels to be used to display different tools. The panels require separate webview controller instances that also need to access some details about project setup that are currently only kept in the main panel webview controller.

As a side effect of this change, we no longer re-create project when switching between UI location for the Radon IDE panel (between tab and side-panel). Therefore this change makes it more seamless for the users to play with the panel location as updating that no longer restarts the project.

For the above to work corrently, we add additional logic for handling dispose calls. When the position of the panel is updated, we first get the original panel disposed, and only then the new panel is created. When the main one is disposed, we don't know whether that would be because of the position change or due to the user closing the IDE panel by hand. In particular, it is possible for user to change the location w/o using the provided menu options, so there's no indication that we can have about the fact that the panel location is being updated. In order to handle that properly, the IDE class has static attach method used by the webview panels and also detach method that the panel uses when they are being disposed. The IDE panel doesn't get disposed immediately when detached, but it waits for 1 second and if it isn't attached to a new panel after that time, only then it calls dispose.

How Has This Been Tested:

  1. Open app in IDE
  2. Change panel location between tab/side bar/ new window – notice that the app doesn't restart when updating the location.
  3. Make sure the IDE is functional after the location is changed.
  4. Close IDE panel (i.e. just close the tab when panel is displayed as editor tab) and make sure the IDE is disposed (we print a debug log when that happens).

Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 11:21am

Copy link
Contributor

@maciekstosio maciekstosio left a comment

Choose a reason for hiding this comment

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

Changing panel location seems to work as expected, but running preview when in side panel causes following error:

image

I assume in src/panels/SidepanelViewProvider.ts:

      SidePanelViewProvider.currentProvider.webviewController.project.startPreview(
        `preview:/${fileName}:${lineNumber}`
      );

should be replaced with:

IDE.getOrCreateInstance(context).project.startPreview(`preview:/${fileName}:${lineNumber}`);

Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

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

I left inline comment

Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

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

I approve after checking it locally just two more things:

  1. I introduced additional commands in Refactor screen recording and add key bindings #875 thats why there are conflicts
  2. Because after the changes changing the IDE panel location does not reset the application we should re-think how we generate/store information about URLs in expo router. Currently we add information about visited paths to an array stored in webview and that array is reset after changing location. i created an issue to track this Refactor how URL's in url are stored/generated #887

@kmagiera kmagiera requested a review from maciekstosio January 9, 2025 11:24
@kmagiera kmagiera dismissed maciekstosio’s stale review January 9, 2025 11:25

already reviewed by Filip

@kmagiera kmagiera merged commit b511325 into main Jan 9, 2025
4 checks passed
@kmagiera kmagiera deleted the kmagiera/ideclass branch January 9, 2025 11:25
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.

3 participants