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

Add terminal profile support #37

Merged
merged 4 commits into from Aug 8, 2021
Merged

Add terminal profile support #37

merged 4 commits into from Aug 8, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jul 8, 2021

Closes #33.

@ghost ghost marked this pull request as draft July 8, 2021 22:39
@ghost
Copy link
Author

ghost commented Jul 8, 2021

Awaiting @types/vscode@v1.58.0 bump

@ghost ghost marked this pull request as ready for review July 9, 2021 07:23
@apommel
Copy link
Owner

apommel commented Jul 9, 2021

I feel like this PR tries to do too much things together. Maybe it's better to just focus on the terminal profile here, and have a look at other changes in other PR?

@ghost
Copy link
Author

ghost commented Jul 9, 2021

Sure, can roll back to the terminal profile commit

I bet you'll have the first extension using this new feature 😄

@ghost
Copy link
Author

ghost commented Jul 9, 2021

Done!

@ghost
Copy link
Author

ghost commented Jul 14, 2021

Any ETA on merging this?

@apommel
Copy link
Owner

apommel commented Jul 16, 2021

Yeah sorry I have been busy lately. I'll try to have a look this weekend and merge it if everything is fine.

@ghost
Copy link
Author

ghost commented Jul 27, 2021

It's no problem, hope you can get to it when you're ready

@ghost
Copy link
Author

ghost commented Aug 4, 2021

I've noticed this branch is still in my list, any dice? I've been testing it a lot and it works great

@apommel
Copy link
Owner

apommel commented Aug 7, 2021

Hey sorry for the delay. I was able to take a look at it today. It does not work for me, I guess you did not push the latest version: some variables have wrong names I think, I'll review it.

@ghost ghost requested a review from apommel August 7, 2021 17:46
@apommel
Copy link
Owner

apommel commented Aug 7, 2021

Okay it works now but I found an issue in the new feature: if I try to open a MATLAB terminal from the integrated terminal without having activated the extension before (so without being on a .m file for example), it fails with the following error No terminal profile provider registered for id "matlab-interactive-terminal.terminal-profile".
I guess it was to be expected. Isn't there a way to activate the extension when the terminal is created through this feature?

@ghost
Copy link
Author

ghost commented Aug 7, 2021

We'd make it activate on * and limit the commands with when clause contexts

package.json Outdated Show resolved Hide resolved
Co-authored-by: Aurélien Pommel <[email protected]>
@apommel
Copy link
Owner

apommel commented Aug 8, 2021

It's fine with me like this. I'll merge it. However if at some point there is an activation event such as "onTerminalProfile" (I saw there was such requests) it would be better to revert the changes to extension activation and add this.
Thanks for the work!

@apommel apommel merged commit 8f323a3 into apommel:master Aug 8, 2021
@ghost ghost deleted the terminal-profile branch August 8, 2021 09:17
@ghost
Copy link
Author

ghost commented Nov 19, 2021

https://code.visualstudio.com/updates/v1_57 mentions onTerminalProfile as working, let's use it!

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.

Add to New Terminal menu
1 participant