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

Removed invalid 'clear' from Runner #46

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

Conversation

JakubSchwenkbeck
Copy link
Contributor

Motivation

Removed the invalid call of clear for the terminal when running a Effekt-file via the extension
Resolves #41

Changes

  • removed invalid sendText("clear"), which caused an error as clear is not a valid windows command
  • added a VSCode API call executeCommand('workbench.action.terminal.clear'), which should perform the clear operation platform independent

src/extension.ts Outdated
Comment on lines 59 to 60
// Clear the terminal using API Call
await vscode.commands.executeCommand('workbench.action.terminal.clear');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused as to how this works -- how does VSCode know which terminal to use?
Does this work on your machine? 🤔 Can we test this on a normal windows cmd terminal that doesn't support clear?

Copy link
Contributor Author

@JakubSchwenkbeck JakubSchwenkbeck Jan 21, 2025

Choose a reason for hiding this comment

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

Oh, did I mix up the terminal the problem occurs in? Are we talking about the integrated terminal which runs inside VScode?

Copy link
Contributor Author

@JakubSchwenkbeck JakubSchwenkbeck Jan 21, 2025

Choose a reason for hiding this comment

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

This API-call works for the integrated terminal, platform independent.
As I read in extension.ts#L1343, the [currently] "active instance" is cleared.

I think this is nicer than something like :

const clearCommand = process.platform === "win32" ? "cls" : "clear";
terminal.sendText(clearCommand);   

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, but it would really need to be tested. Though even for an external terminal, I'd still love to know how VSCode itself can clear a terminal (since I think it's able to do so)... and hopefully they do have some API for such an action too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, which I don't have any answer for currently 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I looked into the codebase for a few minutes and moved several clearBuffer()calls down in the abstraction level, but haven't found an ending yet. By time, I'm happy to investigate further:)

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 ITerminalInstance itself uses two .clearBuffer() https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts#L1341
=> XTerm and a _process, one of them should hold the actual commands...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I think that's not necessarily the level we'll be able to reach (since that API is most likely private [?]), that's managing the terminal interface via xterm.js, I'd assume.
Instead, I think we need to search "up" now to see what we can use in the public API that in turn triggers the .clearBuffer().

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 can't find a way to use the public API to call .clearBuffer(). There doesn’t seem to be a way to explicitly set which terminal is active (might be worth raising an issue in the VS Code repo?). The workaround of using terminal.show() does update onDidChangeActiveTerminal, but the activeInstance used in the API call doesn’t update immediately, which causes unexpected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe VScode Issue #2238507 will gather us more helpful information :)

@JakubSchwenkbeck
Copy link
Contributor Author

I recommend sticking with terminal.show() for now. While it might not cover every use case, it keeps terminal actions explicit and user-driven, which is important for both security and reliability.

from microsoft/vscode#238507 (comment)

@JakubSchwenkbeck
Copy link
Contributor Author

JakubSchwenkbeck commented Jan 24, 2025

This solution would suggest using terminal.show() to move focus to the EffektRunner terminal and then using the API await vscode.commands.executeCommand('workbench.action.terminal.clear');. When tested, I experienced that there needs to be some sort of sleep to ensure the activeInstance is updated. This feels wrong as a solution.

Edit: It doesn't seem reliable at all after some testing - even with the sleep.

@JakubSchwenkbeck
Copy link
Contributor Author

I also recommend using Terminal.show(), but then polling the state of activeTerminal for a short period to see when it changes. I expect this should be fairly reliable.

Well it's again this variant, I can look into a viable solution with terminal.show() if we decide against the simple ternary ?

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.

Runner trying to use invalid 'clear' command on Windows
2 participants