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

Add a Monarch/Peasant sample app #8171

Merged
29 commits merged into from
Jan 19, 2021
Merged

Conversation

zadjii-msft
Copy link
Member

This PR adds a sample monarch/peasant application. This is a type of
application where a single "Monarch" can coordinate the actions of multiple
other "Peasant" processes, as described by the specs in #7240 and #8135.

This project is intended to be a standalone sample of how the architecture would
work, without involving the entirety of the Windows Terminal build. Eventually,
this architecture will be incorporated into wt.exe itself, to enable scenarios
like:

For an example of this sample running, see the below GIF:

monarch-peasant-sample-001

This sample operates largely by printing to the console, to help the reader
understand how it's working through its logic.

I'm doing this mostly so we can have a committed sample of this type of application, kinda like how VtPipeTerm is a sample ConPTY application. It's a lot easier to understand (& build on) when there aren't any window shenanigans, settings loading, Island instantiation, or anything else that the whole of WindowsTerminal.exe needs

@zadjii-msft zadjii-msft added Issue-Samples A request for an example on how we do something. Probably a good docs candidate. Product-Terminal The new Windows Terminal. labels Nov 5, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Gave it a read. Left some commentary.

consolegit2gitfilters.json Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/AppState.cpp Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/Monarch.cpp Outdated Show resolved Hide resolved
// {50dba6cd-2222-4b12-8363-5e06f5d0082c}
constexpr GUID Monarch_clsid{
0x50dba6cd,
0x2222,
Copy link
Member

Choose a reason for hiding this comment

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

Presuming you generated this.... 0x2222 is pretty lucky.

src/tools/MonarchPeasantSample/Peasant.h Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/main.cpp Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/main.cpp Show resolved Hide resolved
src/tools/MonarchPeasantSample/PeasantMain.cpp Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/AppState.cpp Outdated Show resolved Hide resolved
@zadjii-msft
Copy link
Member Author

Hey all, I'm bumping this PR now that we've discussed a bit more.

I'm planning on leaving this PR largely as-is. The goal is to be a sample of Monarch/Peasant, not necessarily the full window managing features from the spec. So things like naming "windows", or providing IDs on the commandline, or using new as -1 - those aren't in this sample.

If we really, REALLY want, I can add them, but I think we can review this without them.

I will update the GlomToLastWindow enum to better match the spec, however

@zadjii-msft zadjii-msft self-assigned this Dec 14, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Good enough for the sample. I think we worked out the big things about this in the spec and spec review.

src/tools/MonarchPeasantSample/AppState.cpp Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/Monarch.cpp Outdated Show resolved Hide resolved
src/tools/MonarchPeasantSample/main.cpp Show resolved Hide resolved
@zadjii-msft zadjii-msft removed their assignment Dec 15, 2020
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 17, 2020
ghost pushed a commit that referenced this pull request Jan 7, 2021
Adds a `Microsoft.Terminal.Remoting.dll` to our solution. This DLL will
be responsible for all the Monarch/Peasant work that's been described in
#7240 & #8135. 

This PR does _not_ implement the Monarch/Peasant architecture in any
significant way. The goal of this PR is to just to establish the project
layout, and the most basic connections. This should make reviewing the
actual meat of the implementation (in a later PR) easier. It will also
give us the opportunity to include some of the basic weird things we're
doing (with `CoRegisterClass`) in the Terminal _now_, and get them
selfhosted, before building on them too much.

This PR does have windows registering the `Monarch` class with COM. When
windows are created, they'll as the Monarch if they should create a new
window or not. In this PR, the Monarch will always reply "yes, please
make a new window".

Similar to other projects in our solution, we're adding 3 projects here:
* `Microsoft.Terminal.Remoting.lib`: the actual implementation, as a
  static lib.
* `Microsoft.Terminal.Remoting.dll`: The implementation linked as a DLL,
  for use in `WindowsTerminal.exe`.
* `Remoting.UnitTests.dll`: A unit test dll that links with the static
  lib. 

There are plenty of TODOs scattered about the code. Clearly, most of
this isn't implemented yet, but I do have more WIP branches. I'm using
[`projects/5`](https://github.com/microsoft/terminal/projects/5) as my
notation for TODOs that are too small for an issue, but are part of the
whole Process Model 2.0 work.

## References

* #5000 - this is the process model megathread
* #7240 - The process model 2.0 spec.
* #8135 - the window management spec. (please review me, I have 0/3
  signoffs even after the discussion we had 😢)
* #8171 - the Monarch/peasant sample. (please review me, I have 1/2)

## PR Checklist
* [x] Closes nothing, this is just infrastructure
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This is a nice proof-of-concept. Are you gonna continue to update it as you make progress? Also, should there be copyright headers on each file?

src/tools/MonarchPeasantPackage/Package.appxmanifest Outdated Show resolved Hide resolved
Comment on lines +64 to +66
This project represents an _incomplete_ example, but one that covers enough of
the edge cases for the reader to comprehend the architecture. Below is a list of
things that are missing from the sample:
Copy link
Member

Choose a reason for hiding this comment

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

So you'll be updating this sample as you make progress on the real thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Frankly no - I'm just using the sample as like a minimal proof-of-concept. The terminal will have the more complete implementation, this is just to understand the basics. Kinda like how VtPipeTerm is a "sample" terminal using conpty. VTPT isn't really a complete sample, and we've learned a lot since then, but it helps demonstrate some of the concepts.

src/tools/MonarchPeasantSample/SampleMonarch.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 19, 2021
@ghost
Copy link

ghost commented Jan 19, 2021

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.

@ghost ghost merged commit c33a979 into main Jan 19, 2021
@ghost ghost deleted the dev/migrie/oop-monarch-peasant-sample branch January 19, 2021 21:55
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
Adds a `Microsoft.Terminal.Remoting.dll` to our solution. This DLL will
be responsible for all the Monarch/Peasant work that's been described in
microsoft#7240 & microsoft#8135. 

This PR does _not_ implement the Monarch/Peasant architecture in any
significant way. The goal of this PR is to just to establish the project
layout, and the most basic connections. This should make reviewing the
actual meat of the implementation (in a later PR) easier. It will also
give us the opportunity to include some of the basic weird things we're
doing (with `CoRegisterClass`) in the Terminal _now_, and get them
selfhosted, before building on them too much.

This PR does have windows registering the `Monarch` class with COM. When
windows are created, they'll as the Monarch if they should create a new
window or not. In this PR, the Monarch will always reply "yes, please
make a new window".

Similar to other projects in our solution, we're adding 3 projects here:
* `Microsoft.Terminal.Remoting.lib`: the actual implementation, as a
  static lib.
* `Microsoft.Terminal.Remoting.dll`: The implementation linked as a DLL,
  for use in `WindowsTerminal.exe`.
* `Remoting.UnitTests.dll`: A unit test dll that links with the static
  lib. 

There are plenty of TODOs scattered about the code. Clearly, most of
this isn't implemented yet, but I do have more WIP branches. I'm using
[`projects/5`](https://github.com/microsoft/terminal/projects/5) as my
notation for TODOs that are too small for an issue, but are part of the
whole Process Model 2.0 work.

## References

* microsoft#5000 - this is the process model megathread
* microsoft#7240 - The process model 2.0 spec.
* microsoft#8135 - the window management spec. (please review me, I have 0/3
  signoffs even after the discussion we had 😢)
* microsoft#8171 - the Monarch/peasant sample. (please review me, I have 1/2)

## PR Checklist
* [x] Closes nothing, this is just infrastructure
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
This PR adds a sample monarch/peasant application. This is a type of
application where a single "Monarch" can coordinate the actions of multiple
other "Peasant" processes, as described by the specs in microsoft#7240 and microsoft#8135.

This project is intended to be a standalone sample of how the architecture would
work, without involving the entirety of the Windows Terminal build. Eventually,
this architecture will be incorporated into `wt.exe` itself, to enable scenarios
like:
* Run `wt` in the current window (microsoft#4472)
* Single Instance Mode (microsoft#2227)

For an example of this sample running, see the below GIF:

![monarch-peasant-sample-001](https://user-images.githubusercontent.com/18356694/98262202-f39b1500-1f4a-11eb-9220-4af4d922339f.gif)

This sample operates largely by printing to the console, to help the reader
understand how it's working through its logic.

I'm doing this mostly so we can have a _committed_ sample of this type of application, kinda like how VtPipeTerm is a sample ConPTY application. It's a lot easier to understand (& build on) when there aren't any window shenanigans, settings loading, Island instantiation, or anything else that the whole of `WindowsTerminal.exe` needs

* [x] I work here
* [x] This is sample code, so I'm not shipping tests for it.
* [x] Go see the doc over in microsoft#8135
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Samples A request for an example on how we do something. Probably a good docs candidate. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants