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.

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

Conversation

zadjii-msft
Copy link
Member

Don't go.
Tracked in #14957

Comment on lines +2079 to +2081
// if the windowId is the same as our name, do nothing
if (windowId == WindowProperties().WindowName() ||
windowId == winrt::to_hstring(WindowProperties().WindowId()))
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)

@@ -1351,7 +1353,7 @@ void AppHost::_handleMoveContent(const winrt::Windows::Foundation::IInspectable&
windowBoundsReference = inDips.to_winrt_rect();
}

_windowManager.RequestMoveContent(args.Window(), args.Content(), args.TabIndex(), windowBoundsReference);
_windowManager.RequestMoveContent(targetWindow, args.Content(), args.TabIndex(), windowBoundsReference);
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated? The previous code had the benefit that the 3 args arguments map more clearly to the function call.

Copy link
Member Author

Choose a reason for hiding this comment

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

op yea that was a side effect of the first attempt at a fix

@DHowett DHowett changed the title Don't even attempt to MoveContent to the same window you started in Don't even try to MoveContent to the same window you started in May 12, 2023
@DHowett DHowett merged commit 1324a01 into main May 12, 2023
@DHowett DHowett deleted the dev/migrie/b/just-dont-go branch May 12, 2023 22:24
DHowett pushed a commit that referenced this pull request May 12, 2023
Don't go.
Tracked in #14957

(cherry picked from commit 1324a01)
Service-Card-Id: 89180217
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants