General review

[liach]

Requesting an overall review from @i509VCB before formal submission of this draft to the fabric api.

This draft is production-ready.

[i509VCB]

Gave it a good look through. Many of the concepts are actually quite interesting. Mainly a few questions is what this whole review consists of.

networking-api-v1-draft/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/client/ClientLoginContext.java

Lines 85 to 96 in 37b32cc

	void respond(PacketByteBuf buf);
	/**
	* Sends a response to the server.
	*
	* <p>The {@code response} may be {@code null} to indicate a "not understood"
	* query response.</p>
	*
	* @param buf the content of the response, may be {@code null}
	* @param callback a callback when the response is sent, may be {@code null}
	*/
	void respond(PacketByteBuf buf, GenericFutureListener<? extends Future<? super Void>> callback);

Would it make sense to overload the void respond(PacketByteBuf buf); method into something like

default void respond(PacketByteBuf buf) {
    this.respond(buf, null);
}

Similarly below with the CompletableFuture methods as well?

networking-api-v1-draft/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/client/ClientNetworking.java

Lines 117 to 125 in 37b32cc

	public static PlayPacketSender getPlaySender() throws IllegalStateException {
	ClientPlayerEntity player = MinecraftClient.getInstance().player;
	if (player == null) {
	throw new IllegalStateException("Cannot get packet sender when not in game!");
	}
	return getPlaySender(player.networkHandler);
	}

Maybe have a way to check if the client's own play sender is available?

networking-api-v1-draft/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/server/ServerLoginContext.java

Lines 71 to 74 in 37b32cc

	/**
	* Returns the integer ID of the query response.
	*/
	int getQueryId();

This kinda seems like internals to me. Is there any reason this is exposed?
If this is needed, maybe explain what the query id corresponds to more precisely.

networking-api-v1-draft/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/server/ServerNetworking.java

Lines 56 to 58 in 37b32cc

	*
	* @see io.github.fablabsmc.fablabs.api.networking.v1.client.ClientNetworking
	*/

Maybe say @see io.github.fablabsmc.fablabs.api.networking.v1.client.ClientNetworking for networking on the client. Or something similar if possible

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketChannelCallback.java

Maybe move this and PacketListenerCallback to a callbacks package to clear up a bit of the root package?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketReceiver.java#L59-L61

Throw a /* @Nullable */ on the method so when we got annotations in fab api we can just search and replace those comments with the annotations?

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/api/networking/v1/PacketSender.java#L40-L46

Like the case above in ClientLoginConext, would it make sense to overload the method with no future listener to call the method with a future listener but pass null for the future.

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/client/ClientLoginNetworkAddon.java#L121-L124

I think there is a getClient call or would work with just an accessor to get the MinecraftClient from the ClientLoginNetworkHandler

Similar situation like above here is possible:
https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/client/ClientPlayNetworkAddon.java#L119-L122

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/impl/networking/AbstractChanneledNetworkAddon.java#L148-L153

Maybe use Identifier#tryCreate here so you can just null check

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/java/io/github/fablabsmc/fablabs/mixin/networking/ClientConnectionMixin.java#L58

Mark this @Unique
Repeast that with any fields you add to any mixins

https://github.com/FabLabsMC/networking-api-v1-draft/blob/master/src/main/resources/assets/networking-api-v1-draft/icon.png

Icon will obviously be changed when PRed to Fabric API

[liach]

1. For providing default impl for overloading, I'd argue that I will leave the implementation in the impl package than leaving default impl in the api package.
2. Client's own play sender is initialized the same time as client player's network handler, so that check you suggest would be extraneous.
3. The query id is like the "message id" in https://wiki.vg/Protocol#Login_Plugin_Request which can be totally internal and be left unexposed if desired
4. @see tag cannot include a description
5. Don't think a callback package with like 2 callbacks is very necessary. The root package is just a set of interfaces defined that expand from the Client/Server networking; they aren't entry to the API themselves, but part of the API.
6. Adding /*Nullable*/ sounds good
7. Again overloading goes to impl
8. Since we can call MinecraftClient.getInstance() don't think it worthes to add an extra accessor, and in a short while don't think minecraft can have two concurrent clients at once
9. Identifier.tryParse literally just throws that exception but eats that exception message, and I want that exception message
10. @Unique sure, dont' think it has a potency to clash though
11. Icon definitely subject to change

So a few changes:

• Add /*Nullable*/ to packet receiver
• Add @Unique to client connection field
• Stop exposing query id for login queries, not much point
• Remember to change icon when it's part of a fabric pr

[i509VCB]

I'd say the @Unique isn't just for clashes but also acts as a sort of decorator telling maintainers that you added that field.