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

Hiding a command by setting its name to null doesn't work properly #8402

Closed
DHowett opened this issue Nov 25, 2020 · 10 comments
Closed

Hiding a command by setting its name to null doesn't work properly #8402

DHowett opened this issue Nov 25, 2020 · 10 comments
Labels
Area-CmdPal Command Palette issues and features 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-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@DHowett
Copy link
Member

DHowett commented Nov 25, 2020

The documentation here states that assigning the value null to an action's "name" field will remove it from the command palette. As of version 1.4.3141.0 (the latest stable release at this writing), following these instructions has no discernible effect.
from @atimholt

Hmm. @carlos-zamora did we break some nullable settings in 1.4? This is before the JsonUtils changes in 1.5...
We should make sure we have tests covering this.
from @DHowett

From MicrosoftDocs/terminal#189

@DHowett DHowett added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Priority-2 A description (P2) labels Nov 25, 2020
@DHowett DHowett added this to the Terminal v1.6 milestone Nov 25, 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 Nov 25, 2020
@zadjii-msft zadjii-msft added the Area-CmdPal Command Palette issues and features label Dec 1, 2020
@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 3, 2020

@zadjii-msft - where was this filtering initially? During deserialization, or somewhere later on (in TerminalPage or Palette itself)?

@carlos-zamora
Copy link
Member

carlos-zamora commented Dec 3, 2020

Oh man. I completely missed responding to this in the docs repo. My bad!

@Don-Vito If you want to take a closer look at this, I'd guess the following places might be a good start:

  • GlobalAppSettings::LayerJson() - we take the json and update our KeyMapping (link)
  • KeyMapping::LayerJson() - we update our keymapping with each command/action (link)

@DHowett
Copy link
Member Author

DHowett commented Dec 3, 2020

It is less likely that this is related to key bindings and more likely that it is related to not-key-bindings.

@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 3, 2020

Oh man. I completely missed responding to this in the docs repo. My bad!

@Don-Vito If you want to take a closer look at this, I'd guess the following places might be a good start:

* `GlobalAppSettings::LayerJson()` - we take the json and update our KeyMapping ([link](https://github.com/microsoft/terminal/blob/8d26f452784cead9fdc353211d3d66d7c27f1fc9/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp#L305))

* `KeyMapping::LayerJson()` - we update our keymapping with each command/action ([link](https://github.com/microsoft/terminal/blob/8d26f452784cead9fdc353211d3d66d7c27f1fc9/src/cascadia/TerminalSettingsModel/KeyMappingSerialization.cpp#L95))

@carlos-zamora - yep.. I looked there earlier today.. but didn't find any special treatment for name equals null.. neither in the latest version nor in the previous version.. so I was wondering if I am looking at the correct place or the filtering out done somewhere later on.

@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 9, 2020

@zadjii-msft , @carlos-zamora - on the de-serialization level I see a special handling for null only for action name, and not command name as appears in documentation.
I looked in the history as well.. but didn't find any reference to command name even in the first commit that the palette was introduced. Any chance that the filtering was somewhere else?

@DHowett
Copy link
Member Author

DHowett commented Dec 9, 2020

So, the hiding probably happens when we request the name and it comes back as null, rather than during deserialization.

@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 9, 2020

So, the hiding probably happens when we request the name and it comes back as null, rather than during deserialization.

When we request the name it might be too late to hide, because at this stage if the "name" didn't exist we will use the one generated from the args. I consider to take an older release and debug it. I just wondered if @zadjii-msft could possibly remember where this filtering was.

@Don-Vito
Copy link
Contributor

@DHowett - apparently it is a duplicate of #7441 from August. I suggest to track that task and close this one.

@zadjii-msft
Copy link
Member

Hey good call! Thanks for helping clean up the repo.

/dup #7441

@ghost
Copy link

ghost commented Dec 11, 2020

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Dec 11, 2020
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 11, 2020
@zadjii-msft zadjii-msft removed this from the Terminal v1.6 milestone Dec 11, 2020
This issue was closed.
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-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-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

4 participants