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

Added custom notify user event #2398

Merged
merged 3 commits into from
Feb 8, 2017

Conversation

woogerboy21
Copy link
Contributor

This PR adds a custom notify user event. I am closing an existing PR and implementing this as a first step towards completing #1863 . Adding a custom notification event such as this allows for a larger scope of use inside the server/client.

Here is a sceen shot of the custom message. There is one translated line in the dialog (that can be reworded upon suggestion) but with the layout as it is now it should enable the user to copy/paste the custom message out into a translation type software such as google translate in the event they do need the message translated.

image

@woogerboy21 woogerboy21 added App - Cockatrice Tickets relating to the cockatrice application App - Protocol / API Tickets that will require protocol level support in addition to client and server changes labels Jan 31, 2017
@woogerboy21
Copy link
Contributor Author

If anyone would like me to setup a test box for this change I can as well. Its pretty simple though so wasnt sure if it mattered.

@woogerboy21
Copy link
Contributor Author

Looks like travis is acting derpy again....

@skwerlman
Copy link
Contributor

If the server operator wants to provide a translated version of the custom message, is that possible with this pr? I imagine it would require the client to send its current language, but it'd be a nice feature.

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Feb 1, 2017

If the server operator wants to provide a translated version of the custom message, is that possible with this pr? I imagine it would require the client to send its current language, but it'd be a nice feature.

This PR simply adds the ability for non-predefined messages to be shown to the user. It's intent was to allow future development for server side operators to add in customization to the code base yet not require client side manipulation to present the notifications. For example, I you were running a server and wanted to add a custom message that is sent during the registration process to a user, their current client would simply show a popup saying they received a message their client didnt understand and to upgrade the client and try again, were as now you can add code to the server sending a custom event and the client would get the popup you see in the example and the message be displayed in the details section.

As for translating the message, the expectation was that the user of the client would need to translate it using whatever means they would personally use. The current translation method used is pre-processing that is done during compile time and "on-the-fly" type translation currently is not possible. We could consider doing some type of "on the fly" translation by having the server query some external source for translation and send the message that the external source returns. But that idea is questionably safe IMO. I could explore that if its of interest by the group as a whole.

What kind of idea did you have in mind? Or were you just thinking if the server could translate something in general it would be cool to have. On most accounts we do have the nationality flag and could query that to find a supporting language to perform "on-the-fly" type translations with the idea mentioned above.

@tooomm
Copy link
Member

tooomm commented Feb 1, 2017

I guess he is thinking about something like this:

Client somehow sends his language setting to the server.
The Server then sends not the general message, but the one fitting the language setting of the client.
For example a server operator could save english, spanish, german and a chinese version of the text to support whatever languages he wants to support and send the requested version if available.

@woogerboy21
Copy link
Contributor Author

Client somehow sends his language setting to the server.
The Server then sends not the general message, but the one fitting the language setting of the client.
For example a server operator could save english, spanish, german and a chinese version of the text to support whatever languages he wants to support and send the requested version if available.

This would be particularly hard to do (but not impossible). The original design thought for this is that there is little change to the client side code which would allow for an easy method for operators to send messages to basically down level clients. In fact I could take this PR one step further and create a public function on the server called "sendCustomNotifyEvent" that could be called to send these types of messages making life even easier for server operators (i like this idea and may still do so if @Daenyth thinks its a good idea). How ever what your suggesting is that now we have a entire event / command / response process that would require additional level of complexity on the server operators additional code logic portion they are adding to the server side code. It's not out of the question to implement. In mind though it would be something code logic wise done prior to sending the event. How simple or complex should something like this be? My $0.02 are as follows:

  • Its always been an effort to reduce protocol sprawl, the more complex it gets the larger the protocol will grow (potentially)
  • Is translation better done by an operator or user? My thought is the user, as an operator I know I don't speak multiple languages so even translating something I want may not be perfect. The user probably understands the structure of whatever language they are using better than I.
  • As mentioned earlier, code logic for translation purposes would be done prior to whats currently in this PR so we can either a) expand this PR further to include it (if that is the direction) or b) create a new PR to add the new functionality when/if we deem it to be advantageous of an idea to implement.

As far as will this PR in its current state facilitate the idea of translation the answer is no. It simply adds the event type and client side code logic to allow event message information to the end user via their client. Any logic on how the message is to be formatted prior to sending the data to the client is still the responsibility of the person adding the code logic to send the message.

@skwerlman
Copy link
Contributor

My idea was along the lines of what tooomm detailed, where the server op could define a message for a given language, and the client would specify what language it preferred when saying hello to the client. Clients which ask for a language that isn't available or don't specify a language would be given whatever language is set as the default for the server.

AFAICT, the only protocol change that isn't already there would be specifying the preferred language, which could piggyback on any command sent very early on (i assume there's some sort of handshake before further commands can be sent?).

To my mind, translation should be done in the following order:

  • Client (when the message has a known meaning, like we already do)
  • Server (when the message has custom content, like in this pr)
  • User (as a last resort ONLY; I can't imagine being told to copy-paste a message into google translate just to read the rules as being a positive user experience)

I don't think this has to have server-side translations right now, but I do think it would be a nice feature to have in the future.

@woogerboy21
Copy link
Contributor Author

I could create an event / command / response group that would get the client too tell the server what language setting the client they are communicating with has set. But tracking which message needs to go to what user with what language would be the tricky part. Conceptually I can already work through it in my mind but would suggest having a separate PR for it and work through the process in layers. Of course if the supreme ruler @Daenyth says otherwise I can add things to this PR :)

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Feb 1, 2017

@skwerlman

(i assume there's some sort of handshake before further commands can be sent?).

There is not. The server simply formats the command into a container and sends the command to the socket listener pushing it back to the client that has the live connection. There is no hello type message sent prior to the server sending a message to the client. When the client first connects it hears some version information from the server at which point authentication begins. Once authenticated the session between the server and client are established and all communications between the two are assumed healthy given the client hasn't timed out from the ping command it sends every so many seconds. If the ping command from a client isn't received X number of times over the course of Y time span the server marks the clients connection as dead and the session is destroyed.

P.S. Welcome to me spoiling the mystical magical way the client<->server communication works as well as showing you the very fringe of the servers 🍝 logic.

@tooomm
Copy link
Member

tooomm commented Feb 1, 2017

Can you be more specific about what kind of useful messages this could be used for?
Which scenarios do you have in mind @woogerboy21 ?

@woogerboy21
Copy link
Contributor Author

woogerboy21 commented Feb 2, 2017

It is originally designed to accommodate #1863. But I can see it used else were. Say for example a server operator wants to notify users at login of something. The operator can simply add a small block of code and when users log in they would get the message with out worry users would have to download a new client to see the message. This of course assumes a time period has passed and the current user base updates to a version of the client that has this notification build it.

I really haven't sat down and thought of other scenario's to much.

Edit:
Maybe something like allowing moderators to notify users of a custom warning..... maybe

Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

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

Looks good to me

@woogerboy21 woogerboy21 merged commit b64eab2 into Cockatrice:master Feb 8, 2017
@woogerboy21 woogerboy21 deleted the custom_user_notifyevent branch July 24, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App - Cockatrice Tickets relating to the cockatrice application App - Protocol / API Tickets that will require protocol level support in addition to client and server changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants