Skip to content

Update websocket.rst,remove WebSocketClient #7837

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 12 commits into from
Closed

Update websocket.rst,remove WebSocketClient #7837

wants to merge 12 commits into from

Conversation

Re-volution
Copy link

WebSocketServer and WebSocketServer no longer exist in godot4

@Re-volution
Update websocket.rst
d028e31 
WebSocketClient and WebSocketServer  no longer exist in godot4
@Re-volution
Update websocket.rst
636d043
@AThousandShips AThousandShips mentioned this pull request Aug 24, 2023
Closed
@AThousandShips AThousandShips added area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.1 labels Aug 24, 2023
@AThousandShips AThousandShips requested a review from Faless August 24, 2023 12:56
AThousandShips
AThousandShips reviewed Aug 24, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The first part is now mostly redundant as it is almost exactly a copy of the code in the documentation for WebSocketPeer, the minor changes in the code copied also made the readability worse

tutorials/networking/websocket.rst
@@ -38,59 +38,32 @@ This example will show you how to create a WebSocket connection to a remote serv

extends Node

# The URL we will connect to
export var websocket_url = "wss://libwebsockets.org"
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

Because I tried and couldn't connect to this address

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that this doesn't provide an example that can be tried to ensure you've done the right thing, your address below isn't a real address

Copy link
Author

Choose a reason for hiding this comment

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

You're right, maybe this should be changed to var websocket_url = "wss://localhost:8999", which would look better. It's best to add a comment # You need to start websocket server that listens to port 8999.

Copy link
Member

Choose a reason for hiding this comment

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

I think a proper working server is required, as is provided for the other PR changing this

Copy link
Member

Choose a reason for hiding this comment

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

#7645 is IMHO more clean, and predates this one, so I think this should be closed in favor of it

Copy link
Author

Choose a reason for hiding this comment

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

I hope to see the content he submitted as soon as possible.Thank you, my friend.

AThousandShips
AThousandShips reviewed Aug 24, 2023
View reviewed changes
AThousandShips
AThousandShips reviewed Aug 24, 2023
View reviewed changes
AThousandShips
AThousandShips reviewed Aug 24, 2023
View reviewed changes
AThousandShips
AThousandShips reviewed Aug 24, 2023
View reviewed changes
@Piralein
Copy link
Member

Please take #7645 into account.
We would like to prevent duplicate work.

@AThousandShips
Copy link
Member

That's true had completely forgotten that one existed as well

Re-volution and others added 10 commits August 24, 2023 22:13
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
5e6e2a9 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
ef98ab6 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
0dbe9a1 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
8bd3534 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
5737f98 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
c377438 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
539b974 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
3da604d 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
54586d2 
Co-authored-by: A Thousand Ships <[email protected]>
@Re-volution @AThousandShips
Update tutorials/networking/websocket.rst
5573d36 
Co-authored-by: A Thousand Ships <[email protected]>
@AThousandShips
Copy link
Member

Thank you for your contribution, sorry it didn't work out, hope to see other contributions from you in the future friend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@Faless Faless Awaiting requested review from Faless

Assignees
No one assigned
Labels
archived area:manual Issues and PRs related to the Manual/Tutorials section of the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Re-volution @Piralein @AThousandShips