Refactor the SettingsTab to be a pane #16172

zadjii-msft · 2023-10-13T19:57:46Z

... technically. We still won't let it actually be a pane, but now it acts like one. It's hosted in a SettingsPaneContent. There's no more SettingsTab. It totally can be in a pane (but don't?)

Validation Steps Performed

Still opens the settings
Only opens a single settings tab, or re-activates the existing one
Session restores!
Updates the title of the tab appropriately
I previously did use the scratchpad action to open the settings in a pane, and that worked.

Related PRs

Refactor Pane to be able to host non-terminal content #16170
- Add an experimental "scratchpad pane", for testing #16171
  - Refactor the SettingsTab to be a pane #16172 <-- you are here
    - Add support for resizing panes with mouse #16895

Refs #997
Closes #8452

…rminal-panes-2023

…ng on termcontrol

…rminal-panes-2023

…/fhl/scratchpad-pane

(cherry picked from commit 63ba8e1)

(cherry picked from commit 262d95a)

…/fhl/scratchpad-pane

…rminal-panes-2023

…/fhl/scratchpad-pane

…anes

## Summary of the Pull Request Builds upon #16170. This PR simply adds a singly type of non-terminal pane - a "scratchpad pane". This is literally just a single text box, in a pane. It's on the `{ "command": "experimental.openScratchpad" }` action. ## References and Relevant Issues See: #997 ## Detailed Description of the Pull Request / Additional comments I also put it behind velocity so it won't even go into preview while this bakes. This is really just here to demonstrate that this works, and is viable. The next PR is much more interesting. ## Validation Steps Performed Screenshot below. ## other PRs * #16170 * #16171 <-- you are here * #16172

…hpad-pane

…anes

zadjii-msft · 2024-03-26T19:36:01Z

but yes, you know what? I'll do that this week and stack it on here

This is in dev/migrie/f/sui-panes...dev/migrie/b/remove-terminaltab

That's ready to go here, but there's also still another two PRs stacked on top of here. Let's merge those in too, before the noisy rename & move

DHowett · 2024-04-03T15:05:36Z

src/cascadia/TerminalApp/TerminalPaneContent.cpp

+        // Do nothing. We'll later be updated manually by
+        // UpdateTerminalSettings, which we need for profile and
+        // focused/unfocused settings.
+        assert(false); // If you hit this, you done goofed.


nit: i'd prefer a more specific and actionable message than "you done goofed"

DHowett · 2024-04-03T15:09:26Z

src/cascadia/TerminalApp/TerminalTab.cpp

@@ -383,7 +390,7 @@ namespace winrt::TerminalApp::implementation
            return RS_(L"MultiplePanes");
        }
        const auto activeContent = GetActiveContent();
-        return activeContent ? activeContent.Title() : L"";
+        return activeContent ? activeContent.Title() : winrt::hstring{ L"" };


nit

Suggested change

return activeContent ? activeContent.Title() : winrt::hstring{ L"" };

return activeContent ? activeContent.Title() : winrt::hstring{};

These are equivalent, but now that you have to specify the type explicitly we may as well switch to the nullary constructor

DHowett · 2024-04-03T15:09:43Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        // HORRIBLE
+        //
+        // Workaround till we know how we actually want to handle state
+        // restoring other kinda of panes. If this is a settings tab, just


Suggested change

// restoring other kinda of panes. If this is a settings tab, just

// restoring other kinds of panes. If this is a settings tab, just

lhecker · 2024-04-03T15:17:48Z

src/cascadia/TerminalApp/TerminalPaneContent.idl


 namespace TerminalApp
 {
    [default_interface] runtimeclass TerminalPaneContent : IPaneContent, ISnappable
    {
        Microsoft.Terminal.Control.TermControl GetTermControl();

-        void UpdateSettings(const Microsoft.Terminal.Settings.Model.TerminalSettingsCreateResult settings,
-                            const Microsoft.Terminal.Settings.Model.Profile profile);
+        void UpdateTerminalSettings(TerminalSettingsCache cache);


From an "offline" (lol) discussion:
It may make sense here to construct TerminalPaneContent with a shared reference to the TerminalSettingsCache and then let UpdateSettings() create the cached objects lazily. That way we avoid the extra API function and try_as tests.

We can do this in a follow-up PR.

@lhecker

As @lhecker noted in the #16172 review, `UpdateTerminalSettings` is wacky. We can just pass the cache in at the start, then reset it and reuse it in `UpdateSettings`. One fewer `try_as`!

…splitPane`, `newTab` (#16914) This changes `NewTabArgs`, `SplitPaneArgs`, and `NewWindowArgs` to accept a `INewContentArgs`, rather than just a `NewTerminalArgs`. This allows a couple things: * Users can open arbitrary types of panes with the existing `splitPane`, `newWindow` actions, just by passing `"type": "scartchpad"` (for example). This is a lot more flexible than re-defining different `"openScratchpad"`, `"openTasksPane"`, etc, etc actions for every kind of pane. * This allows us to use the existing machinery of session restore to also restore non-terminal panes. The `type` property was added to `newTab`, `splitPane`, `newWindow`. When omitted, we still just treat the json as a blob of NewTerminalArgs. There's not actually any other kinds of `INewContentArgs` in this PR (other than the placeholder `GenericContentArgs`). In [`dev/migrie/fhl/md-pane`](dev/migrie/f/tasks-pane...dev/migrie/fhl/md-pane), I have a type of pane that would LOVE to add some args here. So that's forward-thinking. There's really just two stealth types of pane for now: `settings`, and `scratchpad`. Those I DON'T have as constants or anything in this PR. They probably should be? Though, I suspect around the time of the tasks & MD panes, I'll come up with whatever structure I actually want them to take. ### future considerations here * In the future, this should allow extensions to say "I know how to host `foo` content", for 3p content. * The `wt` CLI args were not yet updated to also accept `--type` yet. There's no reason we couldn't easily do that. * I considered adding `ICanHasCommandline` to allow arbitrary content to generate a `wt` commandline-serializable string. Punted on that for now. ## other PRs * #16170 * #16171 * #16172 * #16895 * #16914 <-- you are here Closes #17014

zadjii-msft added 30 commits May 12, 2023 13:32

I wanted to do this in one shot but _zelda_

f353323

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/non-te…

4e14442

…rminal-panes-2023

hey look, it builds now

ef6bb8a

move GetNewTerminalArgs into IPaneContent

5b3aa54

close event

84df819

snapping now uses an interface, so that it's not TermControl-specific

7c9ffb0

huge shuffling so that pane content can raise events instead of relyi…

a1da6c1

…ng on termcontrol

some last cleanups

049c043

[TO PARENT] dead code

2dd8f40

Single commit that adds the whole scratchpad and action

1b39db7

[PARENT] You know what, I just went for it.

63ba8e1

[PARENT] try to use GetActiveTerminalControl less in TerminalTab

262d95a

Let's just make it experimental

2d40306

Merge commit 'b4042ea' into dev/migrie/fhl/non-terminal-panes-2023

e6dc314

Merge commit '6a10ea5' into dev/migrie/fhl/non-terminal-panes-2023

e31202b

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/non-te…

11126f9

…rminal-panes-2023

Merge branch 'dev/migrie/fhl/non-terminal-panes-2023' into dev/migrie…

5f9add4

…/fhl/scratchpad-pane

keybindings too

a23c1a2

[PARENT] You know what, I just went for it.

5582e1b

(cherry picked from commit 63ba8e1)

[PARENT] try to use GetActiveTerminalControl less in TerminalTab

f89368c

(cherry picked from commit 262d95a)

Merge branch 'main' into dev/migrie/fhl/non-terminal-panes-2023

e0b003a

Merge branch 'dev/migrie/fhl/non-terminal-panes-2023' into dev/migrie…

86914bd

…/fhl/scratchpad-pane

feature flags too

1cc9835

POC: yea, this works

cbd61b0

this works better than it has any right to

29d0d57

derp

fb7c809

icons for non-terminal pane content

842326d

update settings should work now

521e301

background brush, done

9531069

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/non-te…

c869b47

…rminal-panes-2023

zadjii-msft added 5 commits March 26, 2024 11:22

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/non-te…

61e952c

…rminal-panes-2023

Merge branch 'dev/migrie/fhl/non-terminal-panes-2023' into dev/migrie…

0979cd6

…/fhl/scratchpad-pane

Doesn't really need to be projected

b6e4b62

un fix this file

1d20599

Merge branch 'dev/migrie/fhl/scratchpad-pane' into dev/migrie/f/sui-p…

ddc88c8

…anes

Base automatically changed from dev/migrie/fhl/scratchpad-pane to main March 26, 2024 18:34

zadjii-msft added 3 commits March 26, 2024 13:55

ALSO doesn't really need to be projected

10e1e46

Merge remote-tracking branch 'origin/main' into dev/migrie/fhl/scratc…

7243d22

…hpad-pane

Merge branch 'dev/migrie/fhl/scratchpad-pane' into dev/migrie/f/sui-p…

ef560bf

…anes

Merge branch 'main' into dev/migrie/f/sui-panes

e05b2bb

carlos-zamora approved these changes Apr 2, 2024

View reviewed changes

DHowett approved these changes Apr 3, 2024

View reviewed changes

lhecker approved these changes Apr 3, 2024

View reviewed changes

zadjii-msft enabled auto-merge April 3, 2024 15:16

lhecker reviewed Apr 3, 2024

View reviewed changes

zadjii-msft added this pull request to the merge queue Apr 3, 2024

Merged via the queue into main with commit 67ae9f6 Apr 3, 2024
20 checks passed

zadjii-msft deleted the dev/migrie/f/sui-panes branch April 3, 2024 16:02

zadjii-msft mentioned this pull request Apr 3, 2024

Get rid of UpdateTerminalSettings #17009

Merged

krzysdz mentioned this pull request Apr 4, 2024

Terminal does not remember session, opens with settings #17019

Closed

zadjii-msft mentioned this pull request May 1, 2024

[1.21] Clicking on the right side of a combobox doesn't cause it to open / dropdown #17062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the SettingsTab to be a pane #16172

Refactor the SettingsTab to be a pane #16172

zadjii-msft commented Oct 13, 2023 •

edited by DHowett

Loading

zadjii-msft commented Mar 26, 2024

DHowett Apr 3, 2024

DHowett Apr 3, 2024

DHowett Apr 3, 2024

lhecker Apr 3, 2024

	return activeContent ? activeContent.Title() : winrt::hstring{ L"" };
	return activeContent ? activeContent.Title() : winrt::hstring{};

	// restoring other kinda of panes. If this is a settings tab, just
	// restoring other kinds of panes. If this is a settings tab, just

Refactor the SettingsTab to be a pane #16172

Refactor the SettingsTab to be a pane #16172

Conversation

zadjii-msft commented Oct 13, 2023 • edited by DHowett Loading

Validation Steps Performed

Related PRs

zadjii-msft commented Mar 26, 2024

DHowett Apr 3, 2024

Choose a reason for hiding this comment

DHowett Apr 3, 2024

Choose a reason for hiding this comment

DHowett Apr 3, 2024

Choose a reason for hiding this comment

lhecker Apr 3, 2024

Choose a reason for hiding this comment

zadjii-msft commented Oct 13, 2023 •

edited by DHowett

Loading