Allow the user to use the tab switcher with in-order tab switching #8076

zadjii-msft · 2020-10-28T16:50:07Z

Summary of the Pull Request

Changes the way the useTabSwitcher setting works. It now accepts either a boolean or a string:

true, "mru": Use the tab switcher with MRU tab switching
"inOrder": Use the tab switcher, with in-order tab switching
false, "disabled": Don't use the tab switcher. Tabs will switch in-order.

This is following the discussion chronicled in #8025, as well as the follow-up investigation in that thread.

References

Display ATS tabs in MRU order #7952 introduced MRU tab switching

PR Checklist

Closes Add a setting for in-order tab traversal with the tab switcher #8025 - there's also discussion of using a parameter in an action to override this setting, but that should get punted to a follow-up task
I work here
Tests added/passed - YOU BET THEY WERE
Requires documentation to be updated

Validation Steps Performed

I've been switching tabs all day and all night, with different settings values, and hot-reloading the setting.

I also ran the test I added.

…tter than two, so I've got to get rid of a bunch of this.

…order-best-order # Conflicts: # src/cascadia/TerminalSettingsModel/GlobalAppSettings.h # src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl

All our JSON files are _actually_ JSONC files - json with comments. A well-behaved application that accepts JSON should accept and ignore comments. However, `jsonlint` is not a well behaved application in this regard. So, to prevent the linter from complaining about our JSON comments, we need to disable it entirely. THAT'S RIGHT, there's not a setting to allow JSONC. See #8076 as an example of this working. This will also unblock #7462.

miniksa · 2020-10-28T21:10:50Z

doc/cascadia/profiles.schema.json

@@ -684,8 +684,20 @@
        },
        "useTabSwitcher": {
          "default": true,
-          "description": "When set to \"true\", the \"nextTab\" and \"prevTab\" commands will use the tab switcher UI.",
-          "type": "boolean"
+          "description": "When set to \"true\" or \"mru\", the \"nextTab\" and \"prevTab\" commands will use the tab switcher UI, with most-recently-used ordering. When set to \"inOrder\", the tab switcher will switch tabs in their current ordering. Set to \"false\" to disable the tab switcher.",


Took me a minute here. So the UI is used for either true, MRU, or inOrder. And false is no UI. The way it's described its sort of like "true"/"mru" = UI and MRU, "inorder" = ???? and in order, "false" = No UI. Like it's weirdly implicit that the UI is used for in order and explicit for the other two options.

Or maybe I'm just crazy.

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

src/cascadia/TerminalApp/CommandPalette.cpp

…order-best-order

src/cascadia/TerminalApp/TerminalPage.cpp

leonMSFT · 2020-10-29T21:28:25Z

doc/cascadia/profiles.schema.json

@@ -684,8 +684,20 @@
        },
        "useTabSwitcher": {
          "default": true,
-          "description": "When set to \"true\", the \"nextTab\" and \"prevTab\" commands will use the tab switcher UI.",
-          "type": "boolean"
+          "description": "When set to \"true\" or \"mru\", the \"nextTab\" and \"prevTab\" commands will use the tab switcher UI, with most-recently-used ordering. When set to \"inOrder\", the tab switcher will switch tabs in their current ordering. Set to \"false\" to disable the tab switcher.",


hmm is there a way we can only accept the three strings "mru", "inOrder", and "disabled"? imo that's clearer than giving the option of true/false as well

It's too late, we already started accepting true and false, and telling people to use those settings ;___;

So now they've gotta mean something sensible in the enum world as well

…order-best-order

…e the menu is open

leonMSFT

nice nice

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett

Dizzying complexity. There's a lot of moving parts here.

ghost · 2020-11-05T14:05:43Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2020-11-05T18:32:42Z

@zadjii-msft did you doc this? ;P

ghost · 2020-11-11T18:14:41Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

brunosantosrhsenso · 2020-11-17T00:43:09Z

Tab switch behavior changed on me out of nowhere. I've been using terminal for almost an year and this whole time tab switch was always in order, and that's how I like it. All of a sudden it changed to mru. May I suggest, next time a change like that is made, that the previous default behavior remains the default for old installations?

zadjii-msft added 6 commits October 27, 2020 10:33

This seems to work, but hot-reloading is _weird_

aa73770

This feels better, but now we've decided that one setting would be be…

60ae88d

…tter than two, so I've got to get rid of a bunch of this.

This is all sorts of wrong, but it's what I've got so far todaye

0d0df37

That's right, THERE ARE TESTS

3fcd5c5

cleanup for review

ff8e34c

adios comment

3823f7b

ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 28, 2020

zadjii-msft assigned leonMSFT Oct 28, 2020

zadjii-msft added 3 commits October 28, 2020 12:18

Merge remote-tracking branch 'origin/main' into dev/migrie/f/8025-in-…

e9922e9

…order-best-order # Conflicts: # src/cascadia/TerminalSettingsModel/GlobalAppSettings.h # src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl

update defaults.json to match

eaec797

Disable the json linter, who can't be bothered to handle json correctly

9edf792

zadjii-msft mentioned this pull request Oct 28, 2020

Disable the json linter #8077

Merged

miniksa reviewed Oct 28, 2020

View reviewed changes

src/cascadia/LocalTests_TerminalApp/TabTests.cpp Show resolved Hide resolved

src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into dev/migrie/f/8025-in-…

0ba2b22

…order-best-order

leonMSFT reviewed Oct 29, 2020

View reviewed changes

leonMSFT assigned zadjii-msft and unassigned leonMSFT Oct 29, 2020

zadjii-msft added 3 commits November 3, 2020 06:51

Merge remote-tracking branch 'origin/main' into dev/migrie/f/8025-in-…

72ef722

…order-best-order

This only uses one list, but doesn't update when a tab is closed whil…

7abf67c

…e the menu is open

Only one list of tabs, whoop

2180774

zadjii-msft assigned leonMSFT and unassigned zadjii-msft Nov 4, 2020

leonMSFT approved these changes Nov 4, 2020

View reviewed changes

zadjii-msft unassigned leonMSFT Nov 4, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Nov 4, 2020

ghost requested a review from carlos-zamora November 4, 2020 21:06

ghost requested review from DHowett and PankajBhojwani November 4, 2020 21:06

carlos-zamora reviewed Nov 4, 2020

View reviewed changes

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

DHowett approved these changes Nov 5, 2020

View reviewed changes

zadjii-msft added 2 commits November 5, 2020 07:41

PR nits

59e957f

We weren't using this at all anymore

4c73bdd

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 5, 2020

ghost merged commit 6639df9 into main Nov 5, 2020

ghost deleted the dev/migrie/f/8025-in-order-best-order branch November 5, 2020 14:28

ghost mentioned this pull request Nov 11, 2020

Add a setting for in-order tab traversal with the tab switcher #8025

Closed

DevWouter mentioned this pull request Nov 12, 2020

Release notes uses useTabSwitcher while it should be tabSwitcherMode #8235

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the user to use the tab switcher with in-order tab switching #8076

Allow the user to use the tab switcher with in-order tab switching #8076

zadjii-msft commented Oct 28, 2020

miniksa Oct 28, 2020

leonMSFT Oct 29, 2020

zadjii-msft Nov 2, 2020

leonMSFT left a comment

DHowett left a comment

ghost commented Nov 5, 2020

DHowett commented Nov 5, 2020

ghost commented Nov 11, 2020

brunosantosrhsenso commented Nov 17, 2020

Allow the user to use the tab switcher with in-order tab switching #8076

Allow the user to use the tab switcher with in-order tab switching #8076

Conversation

zadjii-msft commented Oct 28, 2020

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

miniksa Oct 28, 2020

Choose a reason for hiding this comment

leonMSFT Oct 29, 2020

Choose a reason for hiding this comment

zadjii-msft Nov 2, 2020

Choose a reason for hiding this comment

leonMSFT left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Nov 5, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett commented Nov 5, 2020

ghost commented Nov 11, 2020

brunosantosrhsenso commented Nov 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.