Refactor Pane to be able to host non-terminal content #16170

zadjii-msft · 2023-10-13T19:19:17Z

Instead of Pane hosting a TermControl directly, it now hosts an IPaneContent. This is an abstraction between the TermControl and the pane itself, to allow for arbitrary implementations of IPaneContent, with things that might not be terminals.

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

This PR by itself doesn't do much. It's just a refactoring.

It doesn't actually add any other types of pane content.
It overall just tries to move code whenever possible, with as little refactoring as possible. There are some patterns from before that don't super scale well to other types of pane content (think: the xyzChanged events on IPaneContent).
There's a few remaining places where Pane is explicitly checking if its content is a terminal. We probably shouldn't, but meh

There are two follow-up PRs to this PR:

In addition, there's more work to be done after these merge:

TODO! issue number for "Replace IPaneContent::xyzChanged with PropertyChanged events"
TODO! issue number for "Re-write state restoration so panes don't produce NewTerminalArgs"

Validation Steps Performed

It still launches
It still works
Broadcasting still works
The weird restart connection thing from Fix enter to restart the first pane in a split #16001 still works

PR Checklist

Closes A pane doesn't necessarily need to host a terminal. #997

other PRs

…rminal-panes-2023

…ng on termcontrol

…rminal-panes-2023

(cherry picked from commit 63ba8e1)

(cherry picked from commit 262d95a)

…rminal-panes-2023

lhecker · 2023-10-16T15:05:05Z

src/cascadia/TerminalApp/IPaneContent.idl

+        event Windows.Foundation.TypedEventHandler<Object, Object> TitleChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Object> TabColorChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Object> TaskbarProgressChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Object> ConnectionStateChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Object> ReadOnlyChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Object> FocusRequested;


Another option would be to have a "something changed" event and just recalculate all of the values on the next UI event loop tick (via Dispatcher.RunAsync).

I'm still concerned about the rising number of events. I currently don't quite see the benefit of this fine granularity. 🤔

src/cascadia/TerminalApp/IPaneContent.idl

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/TerminalTab.cpp

src/cascadia/TerminalApp/TerminalPaneContent.h

src/cascadia/TerminalApp/TerminalPaneContent.cpp

carlos-zamora · 2023-10-18T21:54:59Z

src/cascadia/TerminalApp/TerminalPaneContent.cpp

+    void TerminalPaneContent::_controlTitleChanged(const IInspectable&, const IInspectable&)
+    {
+        TitleChanged.raise(*this, nullptr);
+    }
+    void TerminalPaneContent::_controlTabColorChanged(const IInspectable&, const IInspectable&)
+    {
+        TabColorChanged.raise(*this, nullptr);
+    }
+    void TerminalPaneContent::_controlSetTaskbarProgress(const IInspectable&, const IInspectable&)
+    {
+        TaskbarProgressChanged.raise(*this, nullptr);
+    }
+    void TerminalPaneContent::_controlReadOnlyChanged(const IInspectable&, const IInspectable&)
+    {
+        ReadOnlyChanged.raise(*this, nullptr);
+    }
+    void TerminalPaneContent::_controlFocusFollowMouseRequested(const IInspectable&, const IInspectable&)
+    {
+        FocusRequested.raise(*this, nullptr);
+    }


nit: it feels like we could use a macro for these

src/cascadia/TerminalApp/Pane.cpp

carlos-zamora · 2023-10-19T21:06:18Z

src/cascadia/TerminalApp/TerminalPage.cpp

-    void TerminalPage::_restartPaneConnection(const std::shared_ptr<Pane>& pane)
+    void TerminalPage::_restartPaneConnection(
+        const TerminalApp::TerminalPaneContent& paneContent,
+        const winrt::Windows::Foundation::IInspectable&)


Suggested change

const winrt::Windows::Foundation::IInspectable&)

)

What do we need this for?

It's a typed event that we're just plumbing through. RestartTerminalRequested doesn't currently actually raise the event with args but it could? I dunno, that's just how these are generally done.

carlos-zamora · 2023-10-19T21:10:34Z

src/cascadia/TerminalApp/Pane.h

@@ -73,7 +73,7 @@ class Pane : public std::enable_shared_from_this<Pane>

    std::shared_ptr<Pane> GetActivePane();
    winrt::Microsoft::Terminal::Control::TermControl GetLastFocusedTerminalControl();
-    winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl();
+    winrt::Microsoft::Terminal::Control::TermControl GetTerminalControl() const;


There's a few of these like GetTerminalControl() and GetProfile() that kinda don't make sense here anymore. Why not do this:

move them to TerminalPaneContent

force callers to GetContent().GetTerminalControl()
...or something like that? That way it feels like a cleaner abstraction/separation between the Pane and underlying content.

You know what happened here - the remaining callers got merged into main mostly after I had written most of this. The broadcast input stuff? All merged after I did most of this initially 🤦

I wanna say that this is probably okay, mainly for the reason that when I went to go solve the last few cases here (mainly broadcasting related, and also one for detaching termcontrol's from a window), I ended up with a bunch of

TermControl& _termControlFromPane(const auto& pane) { if (const auto content{ pane->GetContent() }) { if (const auto termContent{ content.try_as<winrt::TerminalApp::TerminalPaneContent>() }) { return termContent.GetTerminal(); } } return nullptr; }

helpers scattered through the code. Almost seemed more sensible to just leave it in Pane. Maybe rename it to MaybeGetTerminalControl? idk

…rminal-panes-2023

DHowett · 2024-03-08T19:26:16Z

src/cascadia/TerminalApp/IPaneContent.idl

+
+    runtimeclass BellEventArgs
+    {
+        Boolean FlashTaskbar { get; };


(why is this decision per-event?)

I suppose this is a bit of a change, yea.

Before, in Pane::_ControlWarningBellHandler, the Pane would figure out what to do based on the Profile for the control that raised the event.

Now, that logic moved into TerminalPaneContent::_controlWarningBellHandler. But we've moved the param from just a parame on the pane->page event, to a property raised from any content->page...

(okay but also as you mentioned, the bell plumbing clearly got lost in a merge)

Theoretically, some other kind of pane could also ask for a taskbar flash, not just a terminal

But that's not based on what is inside the tab, that's based on the user's settings...

src/cascadia/TerminalApp/Pane.cpp

DHowett · 2024-03-08T19:32:51Z

src/cascadia/TerminalApp/Pane.cpp

@@ -1332,9 +1113,16 @@ TermControl Pane::GetLastFocusedTerminalControl()
 // - <none>
 // Return Value:
 // - nullptr if this Pane is a parent, otherwise the TermControl of this Pane.
-TermControl Pane::GetTerminalControl()
+TermControl Pane::GetTerminalControl() const


FYI the contract of this API has changed w.r.t. how it works for leaves. Is that intended?

Yea? It's basically an added "if this leaf is a terminal pane" check, so now callers need to be prepared to handle null as "No, this pane was not a terminal"

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/TerminalPaneContent.h

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/TerminalTab.cpp

…rminal-panes-2023

…ld close it

DHowett · 2024-03-19T21:46:24Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -333,7 +333,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
            // (The window has a min. size that ensures that there's always a scrollbar thumb.)
            if (drawableRange < 0)
            {
-                assert(false);


this keeps showing up in your pull requests.. what is it

context here: #16809 (comment)

since I've got marks in my prompt, they hit that assert literally every time I debug the Terminal. We concurred it's not really needed

This PR automagically finds and replaces all[^1] usages of our TYPED_EVENT macro with `til::event`. Benefits include: * less macro magic * editors are more easily able to figure out the relationship between `til::event<> Foo;`, `Foo.raise(...)`, and `bar.Foo({this, &Bar::FooHandler})` (whereas before the relationship between `_FooHandlers(...)` and `bar.Foo({...})`) couldn't be figured out by vscode & sublime. Other find & replace work that had to be done: * I added the `til::typed_event<>` == `<IInspectable, IInspectable>` thing from #16170, since that is goodness * I actually fixed `til::property_changed_event`, so you can use that for your property changed events. They're all the same anyways. * events had to come before `WINRT_PROPERTY`s, since the latter macro leaves us in a `private:` block * `Pane::SetupChildCloseHandlers` I had to swap _back_, because the script thought that was an event 🤦 * `ProfileViewModel::DeleteProfile` had to be renamed `DeleteProfileRequested`, since there was already a `DeleteProfile` method on it. * WindowManager.cpp was directly wiring up it's `winrt::event`s to the monarch & peasant. That doesn't work with `til::event`s and I'm kinda surprised it ever did <details> <summary>The script in question</summary> ```py import os import re def replace_in_file(file_path, file_name): with open(file_path, 'r', encoding="utf8") as file: content = file.read() found_matches = False # Define the pattern for matching pattern = r' WINRT_CALLBACK\((\w+),\s*(.*?)\);' event_matches = re.findall(pattern, content) if event_matches: found_matches = True print(f'found events in {file_path}:') for match in event_matches: name = match[0] args = match[1] if name == "newConnection" and file_name == "ConptyConnection.cpp": # This one is special continue old_declaration = 'WINRT_CALLBACK(' + name + ', ' + args + ');' new_declaration = 'til::event<' + args + '> ' + name + ';' if name != "PropertyChanged" else 'til::property_changed_event PropertyChanged;' print(f' {old_declaration} -> {new_declaration}') content = content.replace(old_declaration, new_declaration) typed_event_pattern = r' TYPED_EVENT\((\w+),\s*(.*?)\);' typed_matches = re.findall(typed_event_pattern, content) if typed_matches: found_matches = True print(f'found typed_events in {file_path}:') for match in typed_matches: name = match[0] args = match[1] if name == "newConnection" and file_name == "ConptyConnection.cpp": # This one is special continue old_declaration = f'TYPED_EVENT({name}, {args});' was_inspectable = (args == "winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable" ) or (args == "IInspectable, IInspectable" ) new_declaration = f'til::typed_event<{args}> {name};' if not was_inspectable else f"til::typed_event<> {name};" print(f' {old_declaration} -> {new_declaration}') content = content.replace(old_declaration, new_declaration) handlers_pattern = r'_(\w+)Handlers\(' handler_matches = re.findall(handlers_pattern, content) if handler_matches: found_matches = True print(f'found handlers in {file_path}:') for match in handler_matches: name = match if name == "newConnection" and file_name == "ConptyConnection.cpp": # This one is special continue old_declaration = f'_{name}Handlers(' new_declaration = f'{name}.raise(' print(f' {old_declaration} -> {new_declaration}') content = content.replace(old_declaration, new_declaration) if found_matches: with open(file_path, 'w', encoding="utf8") as file: file.write(content) def find_and_replace(directory): for root, dirs, files in os.walk(directory): if 'Generated Files' in dirs: dirs.remove('Generated Files') # Exclude the "Generated Files" directory for file in files: if file.endswith('.cpp') or file.endswith('.h') or file.endswith('.hpp'): file_path = os.path.join(root, file) try: replace_in_file(file_path, file) except Exception as e: print(f"error reading {file_path}") if file == "TermControl.cpp": print(e) # raise e # Replace in files within a specific directory directory_path = 'D:\\dev\\public\\terminal\\src' find_and_replace(directory_path) ``` </details> [^1]: there are other macros we use that were also using this macro, those weren't replaced. --------- Co-authored-by: Dustin Howett <[email protected]> Co-authored-by: Leonard Hecker <[email protected]>

…rminal-panes-2023

## 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

lhecker · 2024-03-28T22:12:59Z

src/cascadia/TerminalApp/TerminalTab.cpp

+        events.CloseRequested = content.CloseRequested(
+            winrt::auto_revoke,
+            [dispatcher, weakThis](auto sender, auto&&) -> winrt::fire_and_forget {
+                // Don't forget! this ^^^^^^^^ sender can't be a reference, this is a async callback.
+
+                // The lambda lives in the `std::function`-style container owned by `control`. That is, when the
+                // `control` gets destroyed the lambda struct also gets destroyed. In other words, we need to
+                // copy `weakThis` onto the stack, because that's the only thing that gets captured in coroutines.
+                // See: https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870
+                const auto weakThisCopy = weakThis;
+                co_await wil::resume_foreground(dispatcher);
+                // Check if Tab's lifetime has expired
+                if (auto tab{ weakThisCopy.get() })
+                {
+                    if (const auto content{ sender.try_as<TerminalApp::IPaneContent>() })
+                    {
+                        tab->_rootPane->WalkTree([content](std::shared_ptr<Pane> pane) {
+                            if (pane->GetContent() == content)
+                            {
+                                pane->Close();
+                            }
+                        });
+                    }
+                }
+            });


FYI this addition breaks #16598 because it closes all tabs when it receives a AppHost::_CloseRequested event and then serializes them to disk. This code however asynchronously emits another AppHost::_CloseRequested event which causes the entire process to restart but now there are no tabs left and so it ends up with an empty persistence state.

I think this issue also exists without my persistence PR, however there it's not apparent since nothing of importance happens in AppHost::_CloseRequested.

I've fixed the issue by simply not shutting down tabs on exit.

However, I believe that this is a flaw in this changeset regardless. This is because this event hook creates a loop in my opinion: When the tab closes, it closes the pane which closes the content which closes the pane which closes the tab. This recursion may end up emitting events twice, but definitely at least does things redundantly (even if the 2nd time around may not have any obvious effect).

For instance, TerminalPage::_RemoveTab is called when you explicitly close a tab. This will Shutdown() the root pane which will close the content. This will asynchronously emit a Closed to the pane which, if it still exists for whatever reason, bubbles it up to the TerminalPage which got the Closes event hooked up to _RemoveTab.

I believe this can be fixed by bringing some clarity into the flow. For instance, _RemoveTab could be split into _CloseTab and _RemoveTab. The former gets called by everyone and the latter is exclusively hooked to the Pane's Closed event. The former calls Pane::Shutdown which closes the content which emits the closed event and we can let it bubble up which then removes the tab. If we did that, there would be no ambiguity and there's only 1 way a tab can be closed and only 1 way the tab can be removed.

... 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 * #16170 * #16171 * #16172 <-- you are here * #16895 Refs #997 Closes #8452

…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 17 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

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

[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 remote-tracking branch 'origin/main' into dev/migrie/fhl/non-te…

c869b47

…rminal-panes-2023

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

2086e0f

…rminal-panes-2023

derp

81889a6

microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Oct 13, 2023

annoying build break

fd06409

lhecker reviewed Oct 16, 2023

View reviewed changes

carlos-zamora reviewed Oct 19, 2023

View reviewed changes

zadjii-msft added 2 commits October 25, 2023 09:04

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

8df9523

…rminal-panes-2023

mostly nits

58e8f3c

lhecker approved these changes Nov 1, 2023

View reviewed changes

zadjii-msft added 4 commits November 2, 2023 06:13

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

cf920e7

…rminal-panes-2023

try to remove a few of these but ultimately, eh

4cec7e9

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

f622d80

…rminal-panes-2023

maybe I'm not that good at coding

6bc711d

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

de5f7af

…rminal-panes-2023

zadjii-msft mentioned this pull request Mar 7, 2024

Replace usages of TYPED_EVENT with til::event #16837

Merged

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

a3fbc64

…rminal-panes-2023

DHowett requested changes Mar 8, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 8, 2024

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Mar 15, 2024

zadjii-msft added 2 commits March 18, 2024 13:19

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

ef775a8

…rminal-panes-2023

GREAT-GREAT-GRANDPARENT: Hey when a pane wants to get closed, we shou…

524d658

…ld close it

microsoft-github-policy-service bot removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Mar 18, 2024

zadjii-msft mentioned this pull request Mar 18, 2024

Add support for resizing panes with mouse #16895

Open

2 tasks

zadjii-msft added 6 commits March 19, 2024 13:34

some of the easier nits

6789ec0

get rid of this file

fd8b083

more nits

052dc78

eh these events are from pane content anyways!

a7533fa

hey there buddy, did you get lost?

826fc08

RegisterBigTimeEncapsulationViolatingTerminalPaneContentEvents

52970ef

zadjii-msft assigned DHowett Mar 19, 2024

DHowett reviewed Mar 19, 2024

View reviewed changes

zadjii-msft mentioned this pull request Mar 21, 2024

Add support for restoring non-terminal panes, and opening them with splitPane, newTab #16914

Merged

zadjii-msft added 2 commits March 21, 2024 13:14

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

2083b2f

…rminal-panes-2023

derp

df73d75

DHowett approved these changes Mar 26, 2024

View reviewed changes

zadjii-msft merged commit 08dc346 into main Mar 26, 2024
20 checks passed

zadjii-msft deleted the dev/migrie/fhl/non-terminal-panes-2023 branch March 26, 2024 16:03

lhecker reviewed Mar 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Pane to be able to host non-terminal content #16170

Refactor Pane to be able to host non-terminal content #16170

zadjii-msft commented Oct 13, 2023 •

edited by DHowett

Loading

lhecker Oct 16, 2023

lhecker Nov 1, 2023

carlos-zamora Oct 18, 2023

carlos-zamora Oct 19, 2023

zadjii-msft Oct 25, 2023

carlos-zamora Oct 19, 2023

zadjii-msft Nov 2, 2023

zadjii-msft Nov 8, 2023

DHowett Mar 8, 2024

zadjii-msft Mar 19, 2024

zadjii-msft Mar 19, 2024

DHowett Mar 19, 2024

DHowett Mar 8, 2024

zadjii-msft Mar 19, 2024

DHowett Mar 19, 2024

zadjii-msft Mar 20, 2024

lhecker Mar 28, 2024

lhecker Mar 28, 2024 •

edited

Loading

Refactor Pane to be able to host non-terminal content #16170

Refactor Pane to be able to host non-terminal content #16170

Conversation

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

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

PR Checklist

other PRs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

zadjii-msft commented Oct 13, 2023 •

edited by DHowett

Loading

lhecker Mar 28, 2024 •

edited

Loading