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 shortcuts for starting new sessions is not reflected in new-session dropdown #13943

Closed
vermiculus opened this issue Sep 8, 2022 · 4 comments · Fixed by #17215
Closed
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@vermiculus
Copy link

Windows Terminal version

1.14.2281.0

Windows build number

10.0.19044.0

Other Software

No response

Steps to reproduce

  1. Have at least four (or some number of) profiles configured.
  2. Remove default keybindings like ctrl+shift+4 and make sure customizations are applied.
  3. Click dropdown to launch a new shell.

Expected Behavior

I would expect to not see ctrl+shift+4 to be advertised as bound.

Actual Behavior

You will see ctrl+shift+4 still advertised as bound.

image

Keybindings were removed from the right-hand side and are still visible in the drop-down. The keybindings don't work, as expected, but they are still being advertised.

@vermiculus vermiculus added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 8, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 8, 2022
@DHowett
Copy link
Member

DHowett commented Sep 8, 2022

Thanks for the report!

@carlos-zamora, this is probably a layering/masking issue. The shortcuts exist on the lower layer (defaults.json) but have been unbound on the upper layer (settings.json).

@DHowett DHowett added Area-Settings Issues related to settings and customizability, for console or terminal Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Sep 8, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 8, 2022
@DHowett DHowett added this to the Terminal v1.17 milestone Sep 8, 2022
@zadjii-msft
Copy link
Member

Shockingly, this isn't a test for this case.

    void KeyBindingsTests::TestUnbindReverseLookup()
    {
        Log::Comment(L"TODO!");

        // Wrap the first one in `R"!(...)!"` because it has `()` internally.
        const std::string bindings0String{ R"!([ { "command": { "action": "newTab", "index": 0 }, "keys":"ctrl+a" } ])!" };
        const std::string bindings1String{ R"([ { "keys": "ctrl+a", "command": null } ])" };

        const auto bindings0Json = VerifyParseSucceeded(bindings0String);
        const auto bindings1Json = VerifyParseSucceeded(bindings1String);

        auto actionMap = winrt::make_self<implementation::ActionMap>();
        VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size());

        actionMap->LayerJson(bindings0Json);
        VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size());

        NewTerminalArgs newTerminalArgs{ 0 };
        NewTabArgs newTabArgs{ newTerminalArgs };
        {
            auto keyChord{ actionMap->GetKeyBindingForAction(ShortcutAction::NewTab, newTabArgs) };
            VERIFY_IS_NOT_NULL(keyChord);
        }

        actionMap->LayerJson(bindings1Json);

        {
            auto keyChord{ actionMap->GetKeyBindingForAction(ShortcutAction::NewTab, newTabArgs) };
            VERIFY_IS_NULL(keyChord);
        }
    }

This actually does pass today on main, so there's gotta be some parenting trickiness that causes this to fail

zadjii-msft added a commit that referenced this issue Sep 8, 2022
@zadjii-msft
Copy link
Member

This is a test however: dev/migrie/b/13943-a-test-for-this

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 8, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented May 24, 2023

ignore me, just adding some keywords: new tab key keys shortcut shortcuts menu bindings

@zadjii-msft zadjii-msft modified the milestones: Terminal v1.19, Backlog Oct 4, 2023
github-merge-queue bot pushed a commit that referenced this issue Jun 6, 2024
With the move to Action IDs, it doesn't quite make sense anymore for a
`Command` to know which keys map to it. This PR removes all `Keys` from
`Command`, and any callers to that now instead query the `ActionMap` for
that Command's keys.


Closes #17160 
Closes #13943
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 6, 2024
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-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants