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

Add 'sendchat' command for sending private chats #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lieuwex
Copy link
Member

@lieuwex lieuwex commented Jul 18, 2016

@tomsmeding How's this? The client doesn't necessarily have to implement this right now. But it's maybe a nice feature to have.

@tomsmeding
Copy link
Member

Great idea!
But then players should never have a name that can be/become a roomid; else the client cannot distonguish between them.

@lieuwex
Copy link
Member Author

lieuwex commented Jul 20, 2016

Good point, should I add a room id collision check in nicknameExists?

@lieuwex
Copy link
Member Author

lieuwex commented Jul 20, 2016

I also added an ID field to players (491ac3a).
We can also move from nicknames to IDs only. But the client has to be updated for that.

@tomsmeding
Copy link
Member

Good point, should I add a room id collision check in nicknameExists?

You also need to prevent collisions with any future room id's. I think it's better to either make private messages a different message typ, or add a boolean argument to it indicating whether the message is private.

We can also move from nicknames to IDs only.

Meh

@lieuwex
Copy link
Member Author

lieuwex commented Jul 20, 2016

You also need to prevent collisions with any future room id's. I think it's better to either make private messages a different message typ, or add a boolean argument to it indicating whether the message is private.

Good point.

We can also move from nicknames to IDs only.

Meh

I mean behind the scenes, not user facing. Collisions have to be fixed by appending an (n) after the name, like player(2).

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.

2 participants