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

[workspace]: Pass WorkspaceInput argument into reloadWindow/openNewWindow methods #11571

Merged

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Aug 15, 2022

What it does

This PR changes the workspace service API and passes the WorkspaceInput argument down to the openNewWindow and reloadWindow methods.

I would like to extend the functionality of the workspace service without unnecessarily copy-pasting logic that is irrelevant. I need the original WorkspaceInput to customize the app's behavior after opening a new or reloading the current window. for example, I want to reload a window and run a series of custom commands or open/reveal widgets.

How to test

I added a sample in a separate commit to demo the desired behavior.

  • Use the Reload Window and Open New Terminal command to reload the workspace and open a new terminal,
  • Use the Open New Workspace and Reveal Explorer command to open a new workspace and activate the Explorer view.
  • The examples are made-up samples and have nothing to do with the requested API changes. With the new APIs, the workspace customization can be simplified into a couple of lines of code. Please see the sample example.

Review checklist

Reminder for reviewers

@kittaakos kittaakos changed the title [workspace]: Pass WorkspaceInput argument into reloadWindow/openNewWindow` methods [workspace]: Pass WorkspaceInput argument into reloadWindow/openNewWindow methods Aug 15, 2022
@kittaakos
Copy link
Contributor Author

// CC @AlbyIanna

@colin-grant-work colin-grant-work self-requested a review August 22, 2022 14:22
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I'm perfectly happy with the changes to the WorkspacesService: I don't see any reason that information shouldn't be available to those methods.

I have some doubts about the sample API contribution, though. My impression of the new feature is that it's basically intended to be a CLI of sorts, but for the browser: a way to pass arguments that can be interpreted as the downstream application wishes. Until now, we've had one 'positional' argument, the path to the workspace, but this PR envisions (reasonably) that we might want more. If we want to go this route, might it make sense to build some infrastructure around it - the code that clears the additional arguments from the URL makes me a little nervous.

Another question is whether this is the best way to accomplish the objective. The use-case seems to be cases where one window is opened from another window, since anything else could be handled by the actual CLI. If that's the use case, then rather than passing arguments in the URL, inter-window messaging could be used, since the parent window has access to the child window and could send it a message encoding the details of its 'startup task' when the window was ready to handle it. That would lose some of the flexibility of the URL-based approach - it could never be used for the first window - but it would avoid messing with the URL at all and so confusing the existing workspace setup.

});

function bindSampleStartupTask(bind: interfaces.Bind, rebind: interfaces.Rebind): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move everything below here into its own file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm perfectly happy with the changes to the WorkspacesService: I don't see any reason that information shouldn't be available to those methods.

Thank you so much for the self-assignment.

I have some doubts about the sample API contribution, though.

I wanted to drop the example before the merge. Let me know if the API changes are OK, and I will remove the last commit.

The sole purpose of the sample was to show that custom behavior can be achieved by overriding the openNewWindow and reloadWindow with the proposed API changes. In the Arduino IDE, we would like to make the Theia version updates easy and avoid unnecessary customizations, such as doOpen and openWindow.

My impression of the new feature is that it's basically intended to be a CLI of sorts, but for the browser: a way to pass arguments that can be interpreted as the downstream application wishes.

I appreciate that you have taken the time to think this further. It is precisely the bahevior we want, but it's not yet clear how to support it properly in the long run. URLs have a limited length, so URL manipulation is not the ultimate solution.

Until now, we've had one 'positional' argument, the path to the workspace, but this PR envisions (reasonably) that we might want more. If we want to go this route, might it make sense to build some infrastructure around it - the code that clears the additional arguments from the URL makes me a little nervous.

Absolutely. When we have a distilled approach, I am happy to open a GH discussion or PR and decide on the details with the core committers. Currently, IDE2's top priority is to run in electron. Theia requires a browser and electron solution.

Another question is whether this is the best way to accomplish the objective. The use-case seems to be cases where one window is opened from another window, since anything else could be handled by the actual CLI. If that's the use case, then rather than passing arguments in the URL, inter-window messaging could be used, since the parent window has access to the child window and could send it a message encoding the details of its 'startup task' when the window was ready to handle it. That would lose some of the flexibility of the URL-based approach - it could never be used for the first window - but it would avoid messing with the URL at all and so confusing the existing workspace setup.

Great idea. I also thought about it but decided to use the URL workaround for now. Here is why:

  • Window A opens B and wants to send an "initial config" via IPC to window B,
  • While B is loading (did-finish-load not fired yet), window A closes,
  • B might never receive the message.

Please correct me if I am conceptually wrong or if you have a better idea. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to drop the example before the merge. Let me know if the API changes are OK, and I will remove the last commit.

Sounds good - if you drop that commit, I'll happily approve 👍 .

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos kittaakos force-pushed the ws-service-pass-options branch from 49f195c to c0990fa Compare September 6, 2022 15:58
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

LGTM, and sure to be useful to some adopters. 👍

@kittaakos
Copy link
Contributor Author

@colin-grant-work, could you please proceed with the merge if the PR looks good? Thank you!

@colin-grant-work colin-grant-work merged commit 523fbbf into eclipse-theia:master Sep 21, 2022
@colin-grant-work colin-grant-work added this to the 1.30.0 milestone Sep 21, 2022
@vince-fugnitto vince-fugnitto added the workspace issues related to the workspace label Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants