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.

Sign up for GitHub

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

Jump to bottom

newTab with out-of-range profile index uses the default profile #11114

Closed
KalleOlaviNiemitalo opened this issue Sep 1, 2021 · 6 comments · Fixed by #11621
Closed

newTab with out-of-range profile index uses the default profile #11114

KalleOlaviNiemitalo opened this issue Sep 1, 2021 · 6 comments · Fixed by #11621
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
22H2

Comments

@KalleOlaviNiemitalo
Copy link

Windows Terminal version (or Windows build number)

1.9.1942.0

Other Software

No response

Steps to reproduce

  1. Uninstall and reinstall Windows Terminal, to delete any custom settings.
  2. Start Windows Terminal. It opens a tab with the PowerShell profile.
  3. Open the new-tab dropdown list and note it lists five profiles
    Windows PowerShell Ctrl+Shift+1
    Command Prompt Ctrl+Shift+2
    PowerShell Ctrl+Shift+3
    Debian Ctrl+Shift+4
    Azure Cloud Shell Ctrl+Shift+5
  4. Close the new-tab dropdown list.
  5. Press Ctrl+Shift+9.

Expected Behavior

No tab opens, because there are fewer than nine active profiles. Perhaps it beeps instead.

Actual Behavior

A new tab opens with the PowerShell profile.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 1, 2021
@DHowett
Copy link
Member

DHowett commented Sep 1, 2021

Hmm. I likely regressed this in the change to profile resolution...

@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Sep 2, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 2, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.12 milestone Sep 2, 2021
@zadjii-msft zadjii-msft added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Sep 2, 2021
@zadjii-msft zadjii-msft mentioned this issue Sep 8, 2021
Closed
@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Sep 18, 2021
@zadjii-msft
Copy link
Member

zadjii-msft commented Sep 29, 2021 •
edited
Loading

Oh wait, this isn't a regression. Even in #10982 this worked this way. OP is reporting this in 1.9, and the profile resolution changes only started in v1.11.2421.0.

I'm un-blocking this one, and tossing into 2.0, discussion.

  • How breaking do we think it is if 
    { "command": { "action": "newTab", "index": 999 } },
    does nothing instead?
  • Is there someone out there relying on ctrl+shift+9 to launch their default profile (despite only having two profiles)
  • This is kinda like "pass through move-focus keys when there aren't any panes"

It's easy enough to change this, but we'll need to think for a second about this.

pushed proposal in https://github.com/microsoft/terminal/pull/new/dev/migrie/b/11114-proposal

@zadjii-msft zadjii-msft added Needs-Discussion Something that requires a team discussion before we can proceed and removed Severity-Blocking We won't ship a release like this! No-siree. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Sep 29, 2021
@zadjii-msft zadjii-msft added the Priority-3 A description (P3) label Sep 29, 2021
zadjii-msft added a commit that referenced this issue Sep 29, 2021
@zadjii-msft
Proposal for how to fix #11114
74c717a
@KalleOlaviNiemitalo
Copy link
Author

I'd prefer discarding the keyboard event rather than passing it through to the conpty.

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 15, 2021
@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 26, 2021
@zadjii-msft
Copy link
Member

Theoretically, the best thing to do here is warn the user "hey bud you had an action for 'profile 900' but you have like 5, that doesn't make sense". Unfortunately, we ship with the profiles 1-9 actions by default. So we can't warn for those easily - we'd need to conditionally warn depending on where the action camefrom and that's ridiculous.

We also concur that this is different than switchToTab(999). For this one, we can determine at parse time that doesn't make sense. switchToTab is different, because we won't know at parse time if the value is bigger than the number of tabs.

We also agree that it is okay that these two act differently. It makes more sense for newTab(999) to not open something, and switchToTab(999) to switch to the last tab, rather than them both doing the "same thing".

@zadjii-msft zadjii-msft mentioned this issue Oct 26, 2021
Merged
4 tasks
@ghost ghost added the In-PR This issue has a related PR label Oct 26, 2021
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@ghost ghost closed this as completed in #11621 May 5, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 5, 2022
ghost pushed a commit that referenced this issue May 5, 2022
@zadjii-msft
Ignore newTab actions with a profile index greater than the number …
ce850ca 
…of profiles (#11621)

## Summary of the Pull Request

As discussed in team sync. Ignore `newTab` actions with a profile index greater than the number of profiles.

## PR Checklist
* [x] Closes #11114
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated - maybe❓
carlos-zamora pushed a commit that referenced this issue May 12, 2022
@zadjii-msft @carlos-zamora
Ignore newTab actions with a profile index greater than the number …
55efdbc 
…of profiles (#11621)

## Summary of the Pull Request

As discussed in team sync. Ignore `newTab` actions with a profile index greater than the number of profiles.

## PR Checklist
* [x] Closes #11114
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated - maybe❓

(cherry picked from commit ce850ca)
Service-Card-Id: 81744839
Service-Version: 1.13
@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #11621, which has now been successfully released as Windows Terminal v1.13.1143.:tada:

Handy links:

@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #11621, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
22H2
Development

Successfully merging a pull request may close this issue.

Ignore newTab actions with a profile index greater than the number of profiles microsoft/terminal
3 participants
@DHowett @zadjii-msft @KalleOlaviNiemitalo