Exclude pinned tabs when closing items

[axelcarl]

Closing multiple items will no longer closed pinned tabs.

Closes #19560

Release Notes:

• Fixed close actions closing pinned tabs.

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Axel Carlsson.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[CharlesChen0823]

might we can add one more action? like CloseAllItemsExcludePinned?

[axelcarl]

@CharlesChen0823 that could be an option, or add an action called CloseAllIncludingPinned, I guess it depends on what is the most sensible default?

[HarshNarayanJha]

Well, Close All Tabs shouldn't really close the Pinned tabs because they are special, that's why they were pinned. Adding an option that says Close All Except Pinned will only introduce inconvenience!

[axelcarl]

Yes, that was my feeling as well. Do you think I should add a CloseAllIncludingPinned that can be bound to qa (vim-mode) for those that want that as their default behavior?

[HarshNarayanJha]

Do you think I should add a CloseAllIncludingPinned that can be bound to qa (vim-mode) for those that want that as their default behavior?

Could be useful for is non vim users too. I agree with CloseAllIncludingPinned Action. But don't make it default, and for vim users, they know it better.

[axelcarl]

@CharlesChen0823 @HarshNarayanJha Added the new action "CloseAllItemsIncludingPinned" and tests, I guess the question now is how close left/right should be affected, it would maybe make sense to alter them in a separate pull request though.

[SomeoneToIgnore]

1. I think we need to adjust all "Close*` tab-related commands
2. We do not need any extra actions (not sure why that was suggested initially, but it's not a maintainer's suggestion so feel free to ignore).
   Something like

#[derive(Clone, PartialEq, Debug, Deserialize, Default)]
#[serde(rename_all = "camelCase")]
pub struct CloseAllItems {
    pub save_intent: Option<SaveIntent>,
    pub close_pinned: bool,
}

where close_pinned will be false by default and anyone may rebind it to something else if they need.

[axelcarl]

@SomeoneToIgnore Good idea, will refactor the PR.

[SomeoneToIgnore]

Nice, and even the new test is there.
Seems to be close to merging.

Let's also update both default-macos.json and default-linux.json to explicitly show the "close_pinned": false in each affected action: this will serve as a documentation.

[SomeoneToIgnore]

Suggested change

	pub close_pinned: Option<bool>,
	pub close_pinned: bool,

Why cannot it be just this? What does None mean?

Same for everything below.

[axelcarl]

I opted for an Option to keep support for old keymaps, however if that is not reason enough I can switch it to bool!

(Setting it to a bool would require that every CloseAction would have to be followed by brackets and an explicit boolean value, whereas leaving it empty now would mean close_pinned would be "false" due to the unwrap_or statements).

[SomeoneToIgnore]

Setting it to a bool would require that every CloseAction would have to be followed by brackets and an explicit boolean value

Can you elaborate?
I think it's not true at all, as when we deserialize, we fill in the defaults for the missing declarations.
Example:
we have

zed/assets/keymaps/default-macos.json

Line 117 in b0b29d9

"cmd-shift-left": ["editor::SelectToBeginningOfLine", { "stop_at_soft_wraps": true }],

and when I declare

{
    "bindings": {
      "shift-d": "editor::SelectToBeginningOfLine"
    }
  }

it works for me just fine.

[axelcarl]

Alright, then I was mistaken, will alter it to bool and update the json files. Thanks!

[SomeoneToIgnore]

Thank you.