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

Tweak send stream figure #1054

Merged
merged 3 commits into from Feb 14, 2018
Merged

Tweak send stream figure #1054

merged 3 commits into from Feb 14, 2018

Conversation

martinthomson
Copy link
Member

The use of "Application Open" was confusing. The text is clearer on this point, so simplify the diagram.

Closes #1008.

The use of "Application Open" was confusing.  The text is clearer on this point, so simplify the diagram.

Closes #1008.
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Jan 16, 2018
@@ -2960,7 +2960,7 @@ data to a peer.

~~~
o
| Application Open
| Open Stream
| Open Paired Stream (bidirectional)
v
Copy link
Contributor

Choose a reason for hiding this comment

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

The term "Paired Stream" only appears in the figures. Shouldn't this be Open Unidirectional Stream, Open Bidirectional Stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Does the latest change work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah - sort of - Receive in Send diagram is a bit confusing - but I suppose it means also get ready to receive data.

| Application Open
| Open Paired Stream (bidirectional)
| Open Stream
| Open Bidirectional Receive Stream
Copy link
Contributor

Choose a reason for hiding this comment

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

You have the directions flipped (should be Send here and Receive below). Admittedly pedantic, but how about "Open Bidirectional Stream (Sending part)"? Calling it a "Bidirectional Send Stream" sounds silly.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is entirely intentional. Opening the receive half of a bidirectional stream causes the send side to open.

Copy link
Member Author

Choose a reason for hiding this comment

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

@janaiyengar do you have any suggestions for clarifying this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed the current text is confusing. Open Bidirectional Stream might be clearer.

@martinthomson
Copy link
Member Author

Is this better?

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Thanks, this is clearer to me.

@martinthomson martinthomson merged commit 626f919 into master Feb 14, 2018
@martinthomson martinthomson deleted the fix-send-states branch February 14, 2018 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants