realXtend / tundra

realXtend Tundra SDK, a 3D virtual world application platform.
www.realxtend.org
Apache License 2.0
84 stars 70 forks source link

Userconnection #749

Closed cadaver closed 10 years ago

cadaver commented 10 years ago

UserConnection refactoring.

jonnenauha commented 10 years ago

This is very nice. I read all of the code and left few comments inline. I especially like that you made the login properties to QVarianMap now, even though its not yet used for kNet connections as JSON we can do that in the future (as the code comment says).

I think we can in the future maybe improve this still a bit by making a IUserConnection that would be fully clean of kNet internals and defines and make both classes inherit it. Then both can implement Send<T>(messageStruct) and Send(const kNet::DataSerializer &ds) (I think DS is fine from knet to be in the interface). Seems like currently in the websocket user connection has a null kNet::MessageConnection* in there which looks weird.

Also the type enum basically hard codes the known types to native and websocket, which should probably be removed. Now 3rd party connection implementations need to go and add a enum to core :) Once common interface is there just dynamic_cast to the wanted type for inspection (well, this is probably not the fastest way so there could be something like QString ConnectionType() { return "websocket" or "knet"; }

Thoughts? This is great initial work and the above things are not mandatory but I recon you might agree that they would make this still a bit cleaner.

(I do recognize once the JSON login reply is used for native we can probably remove the enum!)

cadaver commented 10 years ago

Yes, the ConnectionType enum is not ideal and it's not even consistently used. Will likely change it to string. I'll also look if making a subclass for kNet connections is easy, because I noticed it's not nice to have to watch out for null pointers. It'll break some code, like ProjectRoom, though.

Stinkfist0 commented 10 years ago

Re: implicit QVariant->bool conversion: Actually I think it ends up to QVariant::cmp() and should work just fine, but I think explicit toBool() wouldn't hurt there.

jonnenauha commented 10 years ago

@cadaver I think this will break our whole Meshmoon servers too once merged so I'll have to be a bit careful about it. But this is not a big problem, just couple of days to port things to the new way. I suspect we can update project room quite quickly as well.

Well just have to accept that old things might break if we wanna go forward with the codebase :) Next big thing is the UTF8 string stuff for which you already added the initial protocol version stuff.

cadaver commented 10 years ago

Stinkfist: didn't remember there was already eg. KNetAssetProvider and not kNetAssetProvider. I'll rename that.

jonnenauha commented 10 years ago

Looks like you got everything we noted together here in order, thanks for that. Feel free to merge when you feel its ready for it.