Switch Connections to use `ValueSet`s to initialize them by zadjii-msft · Pull Request #10184 · microsoft/terminal

zadjii-msft · 2021-05-25T21:35:31Z

⚠️ targets #10051

Summary of the Pull Request

This PR does one big, primary thing. It removes all the constructors from any TerminalConnections, and changes them to use an Initialize method that accepts a ValueSet of properties.

Why?

For the upcoming window/content process work, we'll need the content process to be able to initialize the connection in the content process. However, the window process will be the one that knows what type of connection to make. Enter ConnectionInformation. This class will let us specify the class name of the type we want to create, and a set of settings to use when initializing that connection.

IMPORTANT: As a part of this, the constructor for a connection must have 0 arguments. RoActivateInstance lets you just conjure a WinRT type just by class name, but that class must have a 0 arg ctor. Hence the need for Initialize, to actually pass the settings.

We're using a ValueSet here because it's basically a json blob, with more steps. In the future, when extension authors want to have custom connections, we can always deserialize the json into a ValueSet, pass it to their connection's Initialize, and let then get what they need out of it.

References

PR Checklist

Closes https://github.com/microsoft/terminal/projects/5#card-50760298
I work here
[n/a] Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

ConnectionInformation was included as a part of this PR, to demonstrate how this will eventually be used. ConnectionInformation is not currently used.

Validation Steps Performed

It still builds and runs.

Now we have an InteractivityAutomationPeer, which acts like the implementation for TermControlAutomationPeer. TermControlAutomationPeer is still needed though, because it implements FrameworkElementAutomationPeer, which is needed to hook up the AP to the actual UIA tree. So we split up the work into two halves, the bit that should be done by the core (the ITextProvider, the IControlAccessibilityInfo), vs the stuff that needs to be in the control (FrameworkElementAutomationPeer). IUiaEventDispatcher should probably be in the control as well, but that's a problem for future us.

… remain.

This reverts commit c113b65. That commit reverted 30b8335

…ractivity-projection-000

…ity-projection-000 # Conflicts: # src/cascadia/TerminalControl/ControlInteractivity.cpp # src/cascadia/TerminalControl/ControlInteractivity.h # src/cascadia/TerminalControl/TermControl.cpp # src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

…s are still wrong

…dcomp-handle

…ractivity-projection-000

…e/oop/sample-project

…ity-projection-000

github-actions · 2021-06-10T17:55:30Z

@check-spelling-bot Report

Unrecognized words, please review:

libe

Previously acknowledged words that are now absent

AAAAABBBBBBBCCC AAAAABCCCCCCCCC AAAAADCCCCCCCCC cang hpcon memcopy rgch Scrolldown Scrolldownpage Scrollup Scrolluppage serializer serializers Tlg

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the dev/migrie/oop/connection-factory-rebase branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/d4c4492f5f94884a8f3cfbac33af2835fbc13d3f.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/858842035" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

…ity-projection-000

commit 04b751f Author: Mike Griese <[email protected]> Date: Tue May 25 16:30:51 2021 -0500 I really shouldn't be doing the throttledfunc thing in this PR commit d615ece Author: Mike Griese <[email protected]> Date: Tue May 25 16:12:52 2021 -0500 this doesn't belong here commit 59dc2ab Author: Mike Griese <[email protected]> Date: Tue May 25 15:41:50 2021 -0500 doc comments commit 3950869 Author: Mike Griese <[email protected]> Date: Tue May 25 15:16:58 2021 -0500 fix both slns to work commit 1f51a26 Author: Mike Griese <[email protected]> Date: Tue May 25 11:37:46 2021 -0500 Remove all IConnectionSettings from WT. Use ValueSet instead commit 7e4ec26 Merge: 36042a3 d4c4492 Author: Mike Griese <[email protected]> Date: Tue May 25 07:38:20 2021 -0500 Merge branch 'dev/migrie/oop/sample-project' into dev/migrie/oop/connection-factory-rebase commit 36042a3 Merge: 5411d8a 76a2dd5 Author: Mike Griese <[email protected]> Date: Mon May 24 12:47:25 2021 -0500 Merge branch 'dev/migrie/oop/sample-project' into dev/migrie/oop/connection-factory-rebase commit 5411d8a Author: Mike Griese <[email protected]> Date: Thu May 20 11:36:00 2021 -0500 Fix the azure connection, and build breaks in general This totally works, so that's cool commit 6104728 Author: Mike Griese <[email protected]> Date: Fri May 14 11:51:48 2021 -0500 cleanup commit d65f897 Author: Mike Griese <[email protected]> Date: Fri May 14 11:22:24 2021 -0500 looks like scrolling is back on the menu boys. 16.243s vs 16.035s in-proc vs out, so I'm gonna say there's not a substantial regression here, whoop whoop commit c850dcf Author: Mike Griese <[email protected]> Date: Fri May 14 10:14:33 2021 -0500 This gets rid of the output one, which I was the MOST worried about. Still worried about the scrollbar one though. In release, we're getting 16.849s vs 16.378s for in-proc vs oop commit 0b53563 Author: Mike Griese <[email protected]> Date: Fri May 14 09:10:27 2021 -0500 found a way to have dispatchers inside ControlCore, in the content process commit 72ccb5d Author: Mike Griese <[email protected]> Date: Tue May 11 16:48:12 2021 -0500 Comment out these events, becasue they're hella slow. We'll have to replace them commit aeb4317 Author: Mike Griese <[email protected]> Date: Tue May 11 14:38:55 2021 -0500 Well, the connection is out of proc, but the callbacks to the UI process are **P A I N** commit 89e0ad5 Author: Mike Griese <[email protected]> Date: Tue May 11 12:38:31 2021 -0500 I really hate this whole 'you can't have two methods with the same number of args' crap. Makes this way harder. Clearly, what I did here is still in-proc commit 206aee3 Author: Mike Griese <[email protected]> Date: Tue May 11 11:59:39 2021 -0500 Change TermControl to beable to do the in-proc connection creation, probably? commit 1d6b1d1 Author: Mike Griese <[email protected]> Date: Tue May 11 09:30:43 2021 -0500 Add IConnectionSettings, so connections can get their settings from another object, not necessarily the ctor commit 7fa5ccd Author: Mike Griese <[email protected]> Date: Mon May 10 16:45:54 2021 -0500 proof of concept using RoActivateClass to create a connection by string (cherry picked from commit 248f91c)

…ity-projection-000

…hub.com/microsoft/terminal into dev/migrie/interactivity-projection-000

…thub.com/microsoft/terminal into dev/migrie/oop/connection-factory-rebase

…ity-projection-000

…ion-000' into dev/migrie/oop/connection-factory-rebase

carlos-zamora

Just the nit about adding a bunch of const there.

src/cascadia/TerminalConnection/ConnectionInformation.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2021-07-19T16:42:07Z

src/cascadia/TerminalConnection/ConnectionInformation.cpp

+        auto pointer = winrt::put_abi(inspectable);
+        ::IInspectable** raw = reinterpret_cast<::IInspectable**>(pointer);


this seems scary. is there not a way to just get an abi-compatible IInspectable** directly?

mfw you suggested that I do this in the first place

Right right, but I meant something closer to, "in the intervening time, did we find something that is less ~ ~ weird ~ ~ than that?" 😄

(reinterpret_casting a pointer is scary, and it didn't seem necessary from the original example 😄)

DHowett · 2021-07-19T16:44:47Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+            // auto bad = unbox_value_or<hstring>(settings.TryLookup(L"foo").try_as<IPropertyValue>(), nullptr);
+            // It'll just return null
+
+            _commandline = winrt::unbox_value_or<winrt::hstring>(settings.TryLookup(L"commandline").try_as<Windows::Foundation::IPropertyValue>(), _commandline);


This could use some helpers in the future... and could use type deduction like JsonUtils

template <typename T> TryGetSetting(ValueSet v, std::wstring_view key, T& storage) { storage = winrt::unbox_value_or<typename std::decay<T>::type>(s.TryLookup(key).try_as...., storage); }

…tion-factory-rebase

ghost · 2021-07-19T21:31:42Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…tion-factory-rebase

zadjii-msft added 30 commits April 29, 2021 12:03

Only access ControlInteractivity through the projection

e4478ee

Only 24 more errors to go

400b35f

some comments that should have been in the previous commit. 16 errors…

4578c13

… remain.

GetHoveredCell/UpdateHoveredCell. 12 remain.

d8cc047

Revert "Revert "Use DComp surface handle for Swap Chain management.""

21a97b6

This reverts commit c113b65. That commit reverted 30b8335

some minor cleanup

2e861d8

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e20caae

…ractivity-projection-000

Use the HANDLE for the swapchainin the core projection

d667f3b

it builds and runs so I guess it's done now

62cbf30

After much work, a .sln that can be used to just test XI, with MUX

015c8a5

rename this thing

92478bb

Embed a Terminal Control in the app, whoop whoop

761b692

Some polish for the sample app

ad71df3

Do the delayload dance

445bdf6

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

1376721

…ractivity-projection-000

What a ridiculous hack

a1ce7cb

fix accessibility

03fb764

I want to get the padding out of interactivity, but the control bound…

6e031e5

…s are still wrong

now this works!

47410c9

Some cleanup

0869456

a bit more cleanup

dace0c4

Merge remote-tracking branch 'origin/main' into dev/migrie/f/oop/use-…

12b78f5

…dcomp-handle

Merge branch 'dev/migrie/f/oop/use-dcomp-handle' into dev/migrie/inte…

e340fe9

…ractivity-projection-000

god I knew I left these behind

b7b23a8

Merge branch 'dev/migrie/interactivity-projection-000' into dev/migri…

07c731d

…e/oop/sample-project

Add updates to the project from dev/migrie/oop/the-whole-thing

71d445b

Some cleanup for review

6e3fadd

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

086710a

…ity-projection-000

ghost requested review from PankajBhojwani and lhecker June 10, 2021 17:53

DHowett changed the base branch from dev/migrie/oop/sample-project to dev/migrie/interactivity-projection-000 June 10, 2021 17:57

DHowett force-pushed the dev/migrie/oop/connection-factory-rebase branch from 04b751f to 248f91c Compare June 10, 2021 18:00

DHowett added 2 commits June 10, 2021 13:00

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

f5665c5

…ity-projection-000

DHowett force-pushed the dev/migrie/oop/connection-factory-rebase branch from 248f91c to c15a93a Compare June 10, 2021 18:01

This comment has been minimized.

Sign in to view

zadjii-msft added 7 commits July 8, 2021 09:07

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

056c354

…ity-projection-000

minor nits from carlos

e643885

Merge branch 'dev/migrie/interactivity-projection-000' of https://git…

33653e0

…hub.com/microsoft/terminal into dev/migrie/interactivity-projection-000

Merge branch 'dev/migrie/oop/connection-factory-rebase' of https://gi…

cde5f7c

…thub.com/microsoft/terminal into dev/migrie/oop/connection-factory-rebase

type

ddb3f6d

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

3d59a67

…ity-projection-000

Merge remote-tracking branch 'origin/dev/migrie/interactivity-project…

02a5de3

…ion-000' into dev/migrie/oop/connection-factory-rebase

carlos-zamora approved these changes Jul 13, 2021

View reviewed changes

src/cascadia/TerminalConnection/ConnectionInformation.cpp Show resolved Hide resolved

DHowett approved these changes Jul 19, 2021

View reviewed changes

Base automatically changed from dev/migrie/interactivity-projection-000 to main July 19, 2021 16:59

zadjii-msft added 3 commits July 19, 2021 12:01

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/connec…

e4cb799

…tion-factory-rebase

nits from review

f554846

audit mode strikes again

810260f

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 19, 2021

zadjii-msft added 2 commits July 20, 2021 09:19

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/connec…

b595215

…tion-factory-rebase

fix build

0516be3

ghost merged commit 6e70c4a into main Jul 20, 2021

ghost deleted the dev/migrie/oop/connection-factory-rebase branch July 20, 2021 15:02

zadjii-msft mentioned this pull request Nov 22, 2021

Design a way for TerminalConnections to receive arbitrary (JSON?) arguments #4003

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch Connections to use `ValueSet`s to initialize them#10184

Switch Connections to use `ValueSet`s to initialize them#10184
89 commits merged intomainfrom
dev/migrie/oop/connection-factory-rebase

zadjii-msft commented May 25, 2021 •

edited by DHowett

Loading

Uh oh!

github-actions bot commented Jun 10, 2021 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

Uh oh!

Uh oh!

Uh oh!

DHowett Jul 19, 2021

Uh oh!

zadjii-msft Jul 19, 2021

Uh oh!

DHowett Jul 19, 2021

Uh oh!

DHowett Jul 19, 2021

Uh oh!

ghost commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		auto pointer = winrt::put_abi(inspectable);
		::IInspectable raw = reinterpret_cast<::IInspectable>(pointer);

Conversation

zadjii-msft commented May 25, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ targets #10051

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

github-actions bot commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

@check-spelling-bot Report

Unrecognized words, please review:

Uh oh!

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DHowett Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 19, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zadjii-msft commented May 25, 2021 •

edited by DHowett

Loading

github-actions bot commented Jun 10, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.