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.

Sign up for GitHub

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

Don't even try to MoveContent to the same window you started in #15325

Merged
merged 3 commits into from
May 12, 2023
Merged

Don't even try to MoveContent to the same window you started in #15325

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e714c59
Don't even attempt to MoveContent to the same window you started in
zadjii-msft May 10, 2023
1193af3
Merge remote-tracking branch 'origin/main' into dev/migrie/b/just-don…
zadjii-msft May 12, 2023
f714949
revert src/cascadia/WindowsTerminal/AppHost.cpp
zadjii-msft May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2081,6 +2081,13 @@ namespace winrt::TerminalApp::implementation
const auto windowId{ args.Window() };
if (!windowId.empty())
{
// if the windowId is the same as our name, do nothing
if (windowId == WindowProperties().WindowName() ||
windowId == winrt::to_hstring(WindowProperties().WindowId()))
Comment on lines +2084 to +2086
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmmm okay, but it does feel a bit weird

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why it needs to compare against both properties? I don't quite get that...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhecker so, the deal is that we generally treat "window" as a stealth union. Users can either specify a window name or a window ID in that property. So actions like

        { "keys": "f1", "command": { "action": "moveTab", "window": "1" }, },
        { "keys": "f2", "command": { "action": "moveTab", "window": "foo" }, },

both work.

We don't know whether the string was the ID, or the name, so we have to check both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 why can't we just be like, if(this == otherwindae)

{
return true;
}

if (const auto terminalTab{ _GetFocusedTabImpl() })
{
auto startupActions = terminalTab->BuildStartupActions(true);
Expand Down