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 startupActions to SUI #8812

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Jan 17, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

image

I wasn't able to add parsed value yet as the parsing logic is in TerminalApp
and thus not accessible from Editor.

@ghost ghost added Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jan 17, 2021
@Don-Vito
Copy link
Contributor Author

Don-Vito commented Jan 19, 2021

@carlos-zamora, @zadjii-msft - marked it as draft because I didn't implement parsed command TextBlock. If it makes sense for 1.6 preview we can probably add this commit and to introduce parsing afterwards. WDYT?

I addition I was not sure if we want to limit the length or to add some scrolling.

BTW the only reason I didn't introduce parsing is because the parsing logic belongs to the Terminal App which is of course not a dependency of the Editor.

@carlos-zamora
Copy link
Member

@carlos-zamora, @zadjii-msft - marked it as draft because I didn't implement parsed command TextBlock. If it makes sense for 1.6 preview we can probably add this commit and to introduce parsing afterwards. WDYT?

BTW the only reason I didn't introduce parsing is because the parsing logic belongs to the Terminal App which is of course not a dependency of the Editor.

Alright, had a discussion offline with the team. Since it's a fairly advanced feature, we'll definitely want some validation/parsing on that TextBox. So we'll push this Settings UI change out of the v1.6 release, but keep the feature itself in (but you have to access it via the JSON).

As for how to implement the parsing, @zadjii-msft has a branch pulling out the commandline arg parsing into its own lib. He'll hop on here soon for more details on that. I think he said he'd make a PR to check that in?

I addition I was not sure if we want to limit the length or to add some scrolling.

The style you used makes the text wrap if the text doesn't fit. Let's keep that for now for consistency with other text settings. This is a common problem we have among all of those settings (see background image and icon path), so we're hoping to come up for a more elegant solution at some point.

@zadjii-msft
Copy link
Member

Yea so, I have a branch over in dev/migrie/f/commandline-lib that pulled the commandline parsing into it's own lib. It's a little tangled with other code I was doing for window management, so it probably needs a bit more time to polish and untangle. I thought I'd need it for window management, but abandoned that path because it made WindowsTerminal dependent on like, the whole world, and I didn't want to bring all those dependencies all the way up the tree. For TSE though, I think it's already dependent on all the things the commandline args are (TSM, TermControl).

I think the commits that are relevant are:

ca6a5fb
b3601ab
d4dbd53
4a536a3

I think we should be able to move the commandline arg parser into it's own lib. Then, we could have both TerminalAppLib and TerminalSettingsEditor reference that lib. That would let the TerminalSettingsEditor create an AppCommandlineArgs instance to do some parsing. It's not the best solution, since we'd have two copies of the code in our package (one compiled into TerminalApp.dll, and another compiled into Microsoft.Terminal.Settings.Editor.dll), but I really really doubt that it would be a significant hit.

Theoretically in the future, we could make it it's own DLL too (Microsoft.Terminal.Commandline.dll) or something, then we'd only have one copy. But touching the project structure is a huge pain and I don't want to deal with that dependency tree myself 😄

So I'll get to working on polishing that.

@Don-Vito
Copy link
Contributor Author

@zadjii-msft, @carlos-zamora - Will SUI be released (as stable) in 1.6 or only in 1.7? Because I think that we need to have startupActions by the time SUI released.

@zadjii-msft
Copy link
Member

It certainly will not be. We're trying to get back on the monthly release train (after taking a long time with 1.6). That does however mean that 1.7 is extra short. We're still missing a bunch of things before we want to call the SUI "stable", this included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants