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

Server Status Ping API #367

Merged
merged 1 commit into from
Jan 14, 2015
Merged

Server Status Ping API #367

merged 1 commit into from
Jan 14, 2015

Conversation

stephan-gh
Copy link
Contributor

Adds an API for #331 to the SpongeAPI. This implements most things mentioned there except direct access to protocol versions, instead there is a (currently) read-only wrapper around them as GameVersion.

I decided to use the newer names from the protocol rewrite in 1.7 instead of the older names from the legacy ping. That's why "server list ping" is "status ping" and "MotD" is "description" now. This matches these names to the ones used in the protocol and the Minecraft source, and should also prevent confusion about "MotD" in the server list, or "MotD" as the message if you join a server (like used by Essentials) in the future.

@stephan-gh stephan-gh added this to the 1.1-Release milestone Jan 4, 2015
@stephan-gh stephan-gh self-assigned this Jan 4, 2015
@dobrakmato
Copy link

Is Favicon the right name for the class? Wouldn't the ServerIcon or just Icon be better?

@stephan-gh
Copy link
Contributor Author

@dobrakmato Favicon is the named used in the protocol and in Minecraft internally. It's the name I have always used for this, but I can change it if others agree that it should be named differently.

* @throws IOException If the favicon couldn't be loaded
* @throws FileNotFoundException If the file doesn't exist
*/
Favicon loadFavicon(File file) throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

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

throws IOException, FileNotFoundException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already included in IOException (FileNotFoundException is a subclass of IOException) and IntelliJ IDEA doesn't stop with complaining that it is already defined.

@maxov
Copy link
Contributor

maxov commented Jan 7, 2015

This seems mostly okay, but I would also try to consider the 'streaming favicon' use case, a-la ServerListPlus 😉. How would one implement that in this API? Could they? Most likely that would require a ProtocolLib analog, but still an interesting thought.

@stephan-gh
Copy link
Contributor Author

@gratimax I have considered that, but as explained in #331 (comment), it's basically just a bug on the client that is making this possible, there is nothing in the protocol that would allow this. The client is just not validating the packets sent by the server, so by resending the status response packet again and again it will keep the connection open infinitely.

This is not hard to fix on the client and requires quite some modifications on the server (and has some other problems on the client caused by this), so this is not something that should be in the SpongeAPI. However, it should be possible for mods to extend this API and add new functionality.

@Aaron1011
Copy link
Contributor

The precise effect of cancelling this event should be documented, as a Vanilla client always expects a response.

@stephan-gh
Copy link
Contributor Author

@Aaron1011 Added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants