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

Allow newTabMenu's matchProfile to work with regex #18553

Open
carlos-zamora opened this issue Feb 10, 2025 · 11 comments
Open

Allow newTabMenu's matchProfile to work with regex #18553

carlos-zamora opened this issue Feb 10, 2025 · 11 comments
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.

Comments

@carlos-zamora
Copy link
Member

Description of the new feature

Currently, matchProfile has 3 ways to get a match:

  • profile name
  • profile source
  • commandline

Today, a profile automatically gets picked up by the matcher if it matches one of the 3 fields above. But it has to be an exact match!

It'd be useful if we could allow for regexes to make it easier to get a match.

(Bonus, somewhat related idea) I think it's worth replacing the text box with a dropdown in the UI too. Just let the user directly select a source from the ones already registered.

Proposed technical implementation details

No response

@carlos-zamora carlos-zamora added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Feb 10, 2025
@carlos-zamora carlos-zamora added this to the Terminal v1.24 milestone Feb 10, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 10, 2025
@carlos-zamora carlos-zamora added Help Wanted We encourage anyone to jump in on these. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 10, 2025
@carlos-zamora
Copy link
Member Author

Bonus: we should move ICU from TerminalCore into til and use that for the regexes

@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 12, 2025
@VishnuSrivatsava
Copy link

Hi @carlos-zamora
I’m interested in working on this feature. I have experience with C++ and performance optimization. Could you provide some guidance on where in the codebase the matching logic is currently implemented? Also, do you have any specific preferences for integrating ICU regex into til?
Looking forward to your input!

@carlos-zamora
Copy link
Member Author

Hi @VishnuSrivatsava! Thanks for expressing your interest. 😊 Here's some notes that might be helpful for you:

The idea here is basically to move the ICU code into TIL so that we can use it throughout the codebase. Once that's done, it should be pretty easy to update MatchProfilesEntry::MatchesProfile with regex matching.

(Bonus, somewhat related idea) I think it's worth replacing the text box with a dropdown in the UI too. Just let the user directly select a source from the ones already registered.

If this is of interest to you, here's a quick walkthrough of what needs to be done:

  • NewTabmenu.xaml:
    • Currently, we use 3 text boxes in the match profiles UI:
      <TextBox x:Uid="NewTabMenu_AddMatchProfiles_Name"
      Text="{x:Bind ViewModel.ProfileMatcherName, Mode=TwoWay}" />
      <TextBox x:Uid="NewTabMenu_AddMatchProfiles_Source"
      Text="{x:Bind ViewModel.ProfileMatcherSource, Mode=TwoWay}" />
      <TextBox x:Uid="NewTabMenu_AddMatchProfiles_Commandline"
      Text="{x:Bind ViewModel.ProfileMatcherCommandline, Mode=TwoWay}" />
    • The idea is to replace the second one (the one for "Source") with a dropdown. The options there would be auto-generated from all the profile sources we have available.
      • Thinking through this, we need a way to query the settings model for all the known sources. I'm working on that change right now as I'm currently working on adding in an Extensions page, which already required a lot of similar changes to the settings model. So, this part with the dropdown might not make sense to do just yet.

Hope this helps!

@VishnuSrivatsava
Copy link

That was helpful, thanks! Would you like me to work on integrating regex matching into MatchesProfile using ICU now, or should I wait until the ICU refactor in til is complete? Also, is there anything specific you’d like me to focus on? I’d love to contribute, and I recently completed my Microsoft OA—hoping this could be a great way to get started!

@carlos-zamora
Copy link
Member Author

No need to wait. 😊 I say go ahead and add the regex matching into MatchesProfiles now. If you can handle moving over the ICU to til, great. If not, it's fine. That'll just be a task to do in the future.

As for other ways to contribute, the Help-Wanted tag and good first issue tag are probably a good place to start. Pick whatever looks interesting to you. I've mainly been working on settings UI/model things and accessibility lately so that's personally what I'm focusing on, but I bounce around to other parts of the repo too.

@VishnuSrivatsava
Copy link

Thanks! I’ll focus on adding regex matching for now. If I get time later, I’ll look into moving ICU to til as well.

@VishnuSrivatsava
Copy link

Hey @carlos-zamora
Since TAEF is Windows-specific, how can I run the tests on macOS? Is there an alternative setup or a way to containerize the test environment? Let me know how I should proceed.

@zadjii-msft
Copy link
Member

.... if you don't have access to a Windows machine to run the tests, then how are you running the Terminal to manually validate 🤔

@zadjii-msft
Copy link
Member

also:

I could have swore that regex matching for profiles was in one of the #12584 drafts, but then we cut it for some reason? I can't remember why now.

@VishnuSrivatsava
Copy link

.... if you don't have access to a Windows machine to run the tests, then how are you running the Terminal to manually validate 🤔

I own a Mac as my main machine, so I was just checking if there was a way to run the tests without switching over. I haven’t really implemented much yet—was just exploring the codebase and getting familiar with things. But yeah, I’ll grab a Windows machine when needed. 😅

@carlos-zamora
Copy link
Member Author

also:

I could have swore that regex matching for profiles was in one of the #12584 drafts, but then we cut it for some reason? I can't remember why now.

Final spec: https://github.com/microsoft/terminal/blob/main/doc/specs/%231571%20-%20New%20Tab%20Menu%20Customization/%231571%20-%20New%20Tab%20Menu%20Customization.md

The regex section is at the bottom Future Considerations section.

Relevant discussions:

Thinking through it again, matchProfiles currently direct string matching for name, commandline, and source. Of the 3, source is pretty helpful because I can put like all my WSL profiles in one folder. commandline is still pretty useful because I can have like multiple pwsh.exe profiles that have a different startingDirectorty. name is the one that I'm struggling to find useful unless we had regex matching.

People aren't really using matchProfiles rn, so I think silently making them regex would be fine. Again, going through the options:

  • name --> upgrading from direct string matching to regex would likely return the same profile(s)
  • source --> "Microsoft.Terminal.Wsl" would return the same profiles
  • commandline --> pwsh.exe would return the same profiles

(now for a more spicy thought)
Following up on this thread, I'd really like it if name, commandline, and source were applied as an intersection, not a union. Kinda reiterating what you said at the end of that thread, this would allow users to get a union via multiple matchProfile entries and get an intersection via a single matchProfile entry.

Silently changing matchProfiles behavior also means that we can update the UI's localized strings, whereas adding functionality further complicates the UI, as it would have to expose regex and non-regex scenarios (doable, but I don't know if making it more complicated is worth it tbh).

@zadjii-msft Thoughts? Probably worth discussing at sync on Monday?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

3 participants