Better relogin code #13
Conversation
| if len(c.replaceCandidates) == 0 { | ||
| // Noone connected with our nonce | ||
| // TODO(Notabilis): Maybe do the ContainsName() check here case-insensitive? | ||
| // Having "Peter" and "peter" is quite strange. Same for HasClient(). |
There was a problem hiding this comment.
We'd need to make sure then that Widelands itself is also case-insensitive. Too much effort?
There was a problem hiding this comment.
Should be no problem implementing this, though I don't think we have to change anything ingame for this. Except maybe when we also want to be case-insensitive when using whisper messages.
There was a problem hiding this comment.
Just wondering: Are you waiting for me to implement this? I wouldn't have done it as part of this branch but of course I can do so.
I looked into the code and for internet gaming it should be possible to change this as a pure metaserver-side change. When we also want to have this in LAN games we have to change Widelands, too.
Two questions regarding the SQL user database:
- Is the username column of the auth_user table an index? In that case the performance would most likely drop when we perform case-insensitive queries. (Could probably be circumvented by adding an always lower-case column which could be indexed.)
- Are there any usernames registered which will break when we implement this?
There was a problem hiding this comment.
Yes, we can always fix this later. I was sort of hoping that SirVer would pitch in, but looks like he doesn't have time at the moment.
Maybe wait with merging until Wednesday, when the current tournament games have been played - just in case?
There was a problem hiding this comment.
I am not that familiar with the things you are working on but will try to answer some questions:
The username of auth_user is a field of type vchar. So this is not an index.
A username "Kaputtnik" is a different user as "kaputtnik" (usernames are always case sensitive)
Are there any usernames registered which will break when we implement this?
If you mean a case insensitive search: I didn't searched for usernames, but if have found none it would mean to implement a thing to forbid a username 'Kaputtnik' when a username 'kaputtnik' already exists. This has to be done before you implement a case insensitive search.
There was a problem hiding this comment.
Thanks for the review! We could merge this, but as far as I know SirVer has to deploy it manually anyway. So lets just wait until he has time for it and merge the related Widelands branch when he deploys the metaserver.
Also thanks for the information regarding the database server, I totally forgot about the registration process. So we have to update the website, the metaserver and Widelands itself when we want to add this. I will open a bug report / feature request on launchpad for implementing this in the future.
|
I am right now in my last week at Google which is a bit draining, so I do not have the energy for reviews right now. I have two weeks of vacation starting next week, I will do it then. |
|
No problem, take your time. This isn't urgent and it makes no sense doing a review when you don't want to do so. |
|
I really want to do the review though. This feature is the most anticipated on my wish list since zoom is implemented. |
|
Code lgtm! |
|
@gunchleoc @frankystone you should have all the rights to do the deploy of this on the server as well, right? @Notabilis if you want to get an ssh account on the server to do the deploying, just send me an email directly. |
|
I should have the rights... but i am feeling not able to deploy it. I don't know anything about this Github stuff... |
|
Maybe we should have a session together where you can teach us? |
|
I am happy to have a session. We haven't talked in a long while anyways, so its time. I do not think I can teach much more than what is in the README of this repository already though. |
|
Will merge now and redeploy once https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264 is submitted. Probably tomorrow. |
|
The widelands branch has been merged at r8503. I just now redeployed the current master of this repo (including this PR) onto the server. Expect people to start asking questions why the have trouble with dev versions before 8503. |
|
Seems to be working fine, thanks! |
Improved the relogin code by adding semi-constant user ids.
Reconnecting to the same name on the metaserver should now work for most cases, even when the complete client computer crashes. The id is used to recognize the user and assign his old name to him instead of giving him the requested name. When the user does a clean logout the reservation is removed.
If logging in with a registered account a second time, a new name will be assigned to the new client automatically instead of displaying an error message.
This branch should be deployed at the same time as merging the related branch of Widelands, since compatibility with the old trunk version breaks. Compatibility with build19 is still maintained.