8000 Specify reason when on game leave; fix #2624 by ctrlaltca · Pull Request #2633 · Cockatrice/Cockatrice · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Specify reason when on game leave; fix #2624 #2633

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 24, 2017
Merged

Specify reason when on game leave; fix #2624 #2633

merged 2 commits into from
Apr 24, 2017

Conversation

ctrlaltca
Copy link
Contributor
@ctrlaltca ctrlaltca commented Apr 24, 2017 •
edited by tooomm
Loading

Related Ticket(s)

Short roundup of the initial problem

When an user leaves a game the reason is not stated, leading to misinterpretations:

  • the game host can quietly kick an user saying that he leaved by himself;
  • a user disconnected because of a temporary internet connection problem will seems to have leaved the game.

What will change with this Pull Request?

The reason why a user leaved the game is passed by the server to clients following @Daenyth's suggestion of keeping the protocol backwards compatible:
#2624 (comment)

Please check the reason strings i chose, they need to be short but correct english.

Explanation with different words:

OTHER = "Server doesn't support this feature"
USER_KICKED = "kicked"
USER_LEFT = "user left"
USER_DISCONNECTED = "server is shutting down, user is spectator or no db setup"

Daenyth
Daenyth requested changes Apr 24, 2017
View reviewed changes
common/pb/event_leave.proto Outdated
enum LeaveReason {
OTHER = 1;
USER_KICKED = 2;
USER_LEAVED = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Should be USER_LEFT ; leaved isn't correct English

cockatrice/src/tab_game.cpp Outdated
switch(reason)
{
case Event_Leave::USER_KICKED:
return tr("kicked");
Copy link
Member

Choose a reason for hiding this comment

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

8000

kicked by game host or moderator ?

cockatrice/src/tab_game.cpp Outdated
return tr("kicked");
break;
case Event_Leave::USER_LEAVED:
return tr("player leaved the game");
Copy link
Member

Choose a reason for hiding this comment

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

player left the game

cockatrice/src/tab_game.cpp Outdated
return tr("player leaved the game");
break;
case Event_Leave::USER_DISCONNECTED:
return tr("disconnected");
Copy link
Member

Choose a reason for hiding this comment

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

player disconnected from server

@ctrlaltca
Copy link
Contributor Author

Strings updated, thank you

tooomm
tooomm reviewed Apr 24, 2017
View reviewed changes
common/pb/event_leave.proto
OTHER = 1;
USER_KICKED = 2;
USER_LEFT = 3;
USER_DISCONNECTED = 4;
Copy link
Contributor
@tooomm tooomm Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

When a user closes the game it'll show "left"? (other than game --> leave game)
When a user closes the app it'll show "left" or "disconnect"?
When a user restarts his PC or his Internet connection drops, what will be shown?

@ctrlaltca
Copy link
Contributor Author

When a user closes the game it'll show "left"? (other than game --> leave game)

When a user closes the game, he gets asked "do you really want to leave the game?" and if he accepts, cockatrice informs the server that the user left the game. So: "left"

When a user closes the app it'll show "left" or "disconnect"?

If you "menu->disconnect" from the server, or just close the whole cockatrice app, it will show "disconnect".

When a user restarts his PC or his Internet connection drops, what will be shown?

"disconnect"

@tooomm
Copy link
Contributor
tooomm commented Apr 24, 2017

What will the message be if I kill the Cockatrice process?

Do we somehow want to (can?) distinguish between disconnecting intentionally and unintentionally or indirectly?
Intentionally beeing hitting disconnect.
Indirectly beeing app close.
Unintentionally being restart or internet drop.

@ctrlaltca
Copy link
Contributor Author

What will the message be if I kill the Cockatrice process?

"disconnect" once the server hits timeout.

Do we somehow want to (can?) distinguish between disconnecting intentionally and unintentionally or indirectly?

If the server loses connection with a player, it has no way to know if the user wanted to disconnect or it was a connection problem.

@tooomm
Copy link
Contributor
tooomm commented Apr 24, 2017

If the server loses connection with a player, it has no way to know if the user wanted to disconnect or it was a connection problem.

Sure sure, we can't know if they killed the process or restarted their pc.
But we could tell if the app was closed intentionally (hitting "x" or "Cockatrice --> Exit"), same as we know if a player hit "leave game".

@ctrlaltca
Copy link
Contributor Author

But we could tell if the app was closed intentionally (hitting "x" or "Cockatrice --> Exit"), same as we know if a player hit "leave game".

We could make cockatrice send a "command_leave" to the server for each open game before it disconnects or closes down.

@Daenyth
Copy link
Member
Daenyth commented Apr 24, 2017

But we could tell if the app was closed intentionally (hitting "x" or "Cockatrice --> Exit"),

Currently that's not true, I'm pretty sure closing is just closing and doesn't inform the server.

There's actually perhaps a good reason to keep it that way - you can close the app then reopen, and you will be reconnected to any in-progress games, whereas if you send an explicit disconnect I believe it would close the game

@ctrlaltca
Copy link
Contributor Author

There's actually perhaps a good reason to keep it that way - you can close the app then reopen, and you will be reconnected to any in-progress games, whereas if you send an explicit disconnect I believe it would close the game

Didn't think of that, it seems reasonable to keep it as it is.

@tooomm
Copy link
Contributor
tooomm commented Apr 24, 2017

Currently that's not true

Ok, I didn't know.

If @ctrlaltca's idea (We could make cockatrice send a "command_leave" to the server for each open game before it disconnects or closes down.) prevent's you from reconnecting that would be bad, yes.

Only "problem" left is that rage quitters might tend to just hit "x" and go on, while you might think there is some serious problem and you keep waiting for nothing.

A new command command_closeapp brings up new problems? Old clients wouldn't just be able to send it...

@ctrlaltca
Copy link
Contributor Author
ctrlaltca commented Apr 24, 2017 •
edited
Loading

Only "problem" left is that rage quitters might tend to just hit "x" and go on, while you might think there is some serious problem and you keep waiting for nothing.

If they "X" the game, you'll know they left it. If they "X" cockatrice and quit, they solved the problem for you and all the other players :)

@tooomm
Copy link
Contributor
tooomm commented Apr 24, 2017 •
edited
Loading

If they "X" cockatrice and quit, they solved the problem for you and all the other players :)

Just kind of. :)

For example a player could experience a forced daily disconnect... if you are in the middle of a game and you enjoy it so far you really want to wait for him. But you just see "disconnected".
If somebody hits "x" instead without telling you before, you could see "closed app" instad "disconnected" and have some additional information to evaluate on.
In this case for example, if he doesn't manage to rejoin within 30s because he closed the app on fault, he might never will. The problem with raging is that they don't tell you before, so you can't know. :)

Different for a forced internet 24h disconnect by your provider, he will come back within like 5min and you happily wait!

@ctrlaltca ctrlaltca merged commit 29904c4 into Cockatrice:master Apr 24, 2017
@ctrlaltca ctrlaltca deleted the fix_2624 branch April 24, 2017 20:20
@fagoliveira
Copy link

I don't know what's the current status of this, but I thought I should leave a comment.

So far, all the games I've done with the most recent release (2.3.17-beta004 (2017-04-27)) have shown a "reason unknown" when players leave. It still doesn't distinguish the reasons why players leave (in other words, getting kicked, leaving the game or leaving the server all show the same thing). Is this supposed to be happening? If not, could it be because players are running different versions of Cockatrice?

@ctrlaltca
Copy link
Contributor Author

I suppose the server has not been updated yet, so it's not passing the information back to clients.

@Daenyth
Copy link
Member
Daenyth commented May 2, 2017

cc @woogerboy21 next maintenance window, can you upgrade?

@ZeldaZach
Copy link
Member

I spoke with Woods a few days ago and this is correct; the server wasn't updated yet. I'll work on a full release at some point

@tooomm tooomm mentioned this pull request May 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tooomm tooomm tooomm left review comments

@Daenyth Daenyth Daenyth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ctrlaltca @tooomm @Daenyth @fagoliveira @ZeldaZach
0