Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enable the Terminal to tell ConPTY who the owner is #12526
Enable the Terminal to tell ConPTY who the owner is #12526
Changes from 15 commits
30863d2
555042e
31e9fef
cb2ad2b
b257581
2009147
669b1e2
5d96691
e46e9a3
23b6fee
cfded0f
cf65085
659d752
4edeec3
000cb83
0ba955d
5e54423
a536bb8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind filing a followup to make this and the following ones about focus part of a new interface?
ITerminalConnectionWindowHosting
or somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't love this coupling, thus the interface :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: vaguely prefer the
HostingWindow
parlance; we can take that as a followup (look at TSE'sIHostedInWindow
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's this a
struct
? Will there be more members in the future?Why is
handle
not aHWND
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, they're all structs? I thought it was a nice abstraction to indicate "this is the packet of data that's sent". I don't expect to add more members in the future, but who knows.
As far as the
uint64_t
vsHWND
- IIRCHWND
is technically just avoid*
, so I wanted to be explicit with the size of data that's getting passed on the wire here. It's the same idea as like, every time we use auint64_t
for aHWND
in any WinRT things.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think transmitting it as a
unintptr_t
would've been more consistent with our other Win32 APIs (which this API is closest to IMO).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, this is a wire format. 32-bit VSCode might be using 64-bit conhost (in fact: it is using 64-bit conhost).
uintptr_t
changes size.uint64_t
does not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting -- why did we keep it a
uint64_t
this deep in the stack? We should decap it as early as we canThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
psuedo
->pseudo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure . . . so this sets the OWNER, not the PARENT, for the same reason as below (no
CHILDWINDOW
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly. There's no
::SetOwner
,SetParent
sets the owner/parent based on ifWS_CHILD
is set.