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

NewTab won't leave the Command Palette #7441

Closed
carlos-zamora opened this issue Aug 28, 2020 · 6 comments · Fixed by #9621
Closed

NewTab won't leave the Command Palette #7441

carlos-zamora opened this issue Aug 28, 2020 · 6 comments · Fixed by #9621
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@carlos-zamora
Copy link
Member

Environment

Windows build number: 10.0.19042.0
Windows Terminal version : 1.3.2382.0

Steps to reproduce

Add the following to your settings.json:

{ "name": null, "command": { "action": "newTab", "commandline": "wsl.exe" }, "keys": "f2" }

Expected behavior

This entry does not appear in the Command Palette.

Actual behavior

It appears in the Command Palette after explicitly setting "name" to null.

@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Aug 28, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 28, 2020
@DHowett
Copy link
Member

DHowett commented Aug 28, 2020

oh no, this is another instance where we need to know the difference between null and ""!

Except not in JSON. In Command.cpp:33-66

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 28, 2020
@ghost
Copy link

ghost commented Oct 20, 2020

Hi. I've tried to resolve this, yet I think it can't be since code is acting the way it is.
{ "name": null, "command": { "action": "newTab", "commandline": "wsl.exe" }, "keys": "f2" } and
{"command": {"action": "splitPane", "commandline": "wsl.exe" },"keys": "f2" } are pretty much treated as exactly same(meaning they are both null) so no matter I use something like name.isNull(), name == Json::Value::nullSingleton, and name == NULL "name": null would pass if (const auto name{ json[JsonKey(NameKey)] }) in Command.cpp line 63 and _nameFromJson() returns L"". However, I was able to "name": null to not apeear in the CommandPalette, but at same time all the commands which dose not have name key in it e.g({ "command": "scrollDown", "keys": "ctrl+shift+down" }) also disappeared from it I only got Select color scheme..., New tab..., Split pane... in the CommandPalette. All I am saying is that if there are way to differentiate
{ "name": null, "command": { "action": "newTab", "commandline": "wsl.exe" }, "keys": "f2" } and
{"command": {"action": "splitPane", "commandline": "wsl.exe" },"keys": "f2" }
this issue can be resolved. Sorry if I'm wrong.

@zadjii-msft zadjii-msft added the Area-CmdPal Command Palette issues and features label Dec 1, 2020
@Don-Vito
Copy link
Contributor

@zadjii-msft, @DHowett - so this one is broken for 4 months.. and actually I am not sure when it really worked last time for commands that have action args 😊 Right now I don't see any evidence it ever worked 🤔

So.. I am not sure if it is acceptable.. but cannot we simply replace this mechanism with something more explicit and parse-able. Like adding a dedicated field (aka "hideFromPalette")? Since I am not sure we can call it breaking compatibility

@zadjii-msft
Copy link
Member

FYI future readers: #8402 has more investigation in it too.

Yea, I think the "name": null idea for hiding came about long before auto-generated names did, so now hiding with a null name seems weird.

I'm almost wondering how important hiding default commands is in the first place. It doesn't really seem like we've gotten requests for it. It's not like having default keybindings, where their presence might impact other apps.

That would certainly make the design easier. If we didn't need to worry about hiding defaults, we could focus on just hiding commands defined by the user.

@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Dec 11, 2020
@atimholt
Copy link

For what it's worth, I'm always for as much customizability as possible. The presence of commands that one has long decided they're never going to use creates a measure of noise and, worse, can often hinder "fluent" use by necessitating more keystrokes for the fuzzy-find search.

@ghost ghost added the In-PR This issue has a related PR label Apr 16, 2021
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 5, 2021
carlos-zamora added a commit that referenced this issue May 5, 2021
This entirely removes `KeyMapping` from the settings model, and builds on the work done in #9543 to consolidate all actions (key bindings and commands) into a unified data structure (`ActionMap`).

## References
#9428 - Spec
#6900 - Actions page

Closes #7441

## Detailed Description of the Pull Request / Additional comments
The important thing here is to remember that we're shifting our philosophy of how to interact/represent actions. Prior to this, the actions arrays in the JSON would be deserialized twice: once for key bindings, and again for commands. By thinking of every entry in the relevant JSON as a `Command`, we can remove a lot of the context switching between working with a key binding vs a command palette item.

#9543 allows us to make that shift. Given the work in that PR, we can now deserialize all of the relevant information from each JSON action item. This allows us to simplify `ActionMap::FromJson` to simply iterate over each JSON action item, deserialize it, and add it to our `ActionMap`.

Internally, our `ActionMap` operates as discussed in #9428 by maintaining a `_KeyMap` that points to an action ID, and using that action ID to retrieve the `Command` from the `_ActionMap`. Adding actions to the `ActionMap` automatically accounts for name/key-chord collisions. A `NameMap` can be constructed when requested; this is for the Command Palette.

Querying the `ActionMap` is fairly straightforward. Helper functions were needed to be able to distinguish an explicit unbinding vs the command not being found in the current layer. Internally, we store explicitly unbound names/key-chords as `ShortcutAction::Invalid` commands. However, we return `nullptr` when a query points to an unbound command. This is done to hide this complexity away from any caller.

The command palette still needs special handling for nested and iterable commands. Thankfully, the expansion of iterable commands is performed on an `IMapView`, so we can just expose `NameMap` as a consolidation of `ActionMap`'s `NameMap` with its parents. The same can be said for exposing key chords in nested commands.

## Validation Steps Performed

All local tests pass.
@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, 2021
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements 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
Development

Successfully merging a pull request may close this issue.

5 participants