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

Fix SCM Inline commands #12295

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Fix SCM Inline commands #12295

merged 1 commit into from
Mar 17, 2023

Conversation

alvsan09
Copy link
Contributor

What it does

Fixes: #12173
Fixes: #12238

SCM inline commands were executed before adapting their arguments to its 'rpc' format.
This caused an infinite loop while trying to package and serialize argument objects before sending them to the 'rpc' channel.

The adapters exist and were originally used by context menus.

This update uses the 'MenuCommandExecutor' to channel the execution of SCM inline commands through the existing registered adapters.

How to test

Inspired by #12173

  1. start the application without @theia/git in either the browser or electron's package.json
  2. comment out the following so yarn download:plugins pulls the builtins https://github.com/eclipse-theia/theia/blob/v1.35.0/package.json#L113-L114
  3. build, download plugins, and start the application
  4. open a workspace under git version control
  5. make changes
  6. attempt to interact with the scm view with the inline toolbar item for the changes
  7. They show work as shown in the following video.
scm_inline_commands.mp4.mp4

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility scm issues related to the source control manager labels Mar 14, 2023
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.

Works for me 👍

@alvsan09
Copy link
Contributor Author

As @vince-fugnitto mentioned, the commit message accept button on the COMMIT_EDITMSG editor/title tab bar is broken since commit 81b98d6.
The issue has been reported in #12314

Any objections to merge this PR?

SCM inline commands were executed before adapting their arguments to
its 'rpc' format, causing an infinite loop while trying to serialize
argument objects before sending them to the 'rpc' channel.

The adapter's exist and were originally used by context menus.

This update uses the 'MenuCommandExecutor' to channel the execution
of SCM inline commands through the existing registered adapters.

It also replaces the use of 'CommandRegistry' by 'MenuCommandExecutor'

Signed-off-by: Alvaro Sanchez-Leon <[email protected]>
@alvsan09 alvsan09 force-pushed the asl/fix_scm_inline_commands branch from d07c072 to ecb7d3b Compare March 17, 2023 14:47
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I confirmed that:

  • inline toolbar items for resources in scm work as expected with @theia/git
  • inline toolbar items for resources in scm work as expected for the git builtins
  • the code looks correct

@alvsan09 alvsan09 merged commit de84d3a into master Mar 17, 2023
@alvsan09 alvsan09 deleted the asl/fix_scm_inline_commands branch March 17, 2023 17:54
@github-actions github-actions bot added this to the 1.36.0 milestone Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scm issues related to the source control manager vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode.git plugin issue in latest Theia rpc: maximum call stack size exceeded
3 participants