SceptreOfficial / Simplex-Support-Services

Support system for Arma 3
Other
26 stars 12 forks source link

Localization - Interaction & Support modules #67

Closed Mysteryjuju closed 3 years ago

SceptreOfficial commented 3 years ago

I see a lot of functionality changes beyond regular translation stuff, can you elaborate so I know what's going on? The big one being ccb71faff92a16f7105b512e8adcae069986d548

Mysteryjuju commented 3 years ago

Indeed, it's a big PR. During tests for localization, I encountered a problem with some notifications which are not in the good language: the server is configured in english and my game in french. A lot of SSS notifications are generated by the owner of the support (generally, the server), and the localization was executed on the server before sending the notification string to clients: the string was displayed with the wrong language on clients which have not their games configured in english. The solution here, instead of sending a localized string, is to send a block of code which will be executed on the client to used its own localization settings. I modified the function common/fnc_notify.sqf to support code with arguments instead of simple string in its parameters. This function is used in both modules interaction and support, that's why there is the two modules localized in the same PR.

SceptreOfficial commented 3 years ago

I understand. It seems kind of awkward sending code for this IMO. What if it just sends the LSTRING and the final output is localized on the client? I'll have a deeper look at it when I'm back at my PC though.

Mysteryjuju commented 3 years ago

That was my first approach, but some notifications used more complex scripts to be executed (each time you have a call to fnc_properTime.sqf for example). To avoid multiple functions or functionnalities, I updated fnc_notify to manage all cases with a block of code as argument. But we can change this to support LSTRING as parameter of fnc_notify too and perform localization on it. That was what I do first before I have problems with more complex strings which need more scripts to be built.

SceptreOfficial commented 3 years ago

Ah, yeah that makes sense. It will probably take a lot of restructuring of the mod in general to achieve a cleaner approach, so we'll run with the work you've done for now. I appreciate it. I'll merge in a day or so in case you have any other changes to do.

SceptreOfficial commented 3 years ago

Also feel free to throw your name(s) into the authors.txt

Mysteryjuju commented 3 years ago

As you want for the merge with this solution. With the actual code structure, in order to avoid big restructuring, I see 3 options: 1) fnc_notify take a block of code as parameter to execute it directly on clients (solution proposed in the patch) 2) fnc_notify can support block of code and LSTRING as parameter and manage them internally (execute code for block of code or perform a localize for LSTRING) 3) we create split fnc_notify in two distinct functions (to avoid a complex function as in proposition 2): -> fnc_notify: take LSTRING as parameter and localize it (to keep code simple when there is a simple string) -> fnc_notify_code: take a block of code as parameter

I have no preference. Don't hesitate if you want I change it (or with another solution if you have one). Clean code is important for the future. ;)

SceptreOfficial commented 3 years ago

I'd say leave it as is, if it all works. We can come up with something later on. If it's ready I'll merge on your command.

Mysteryjuju commented 3 years ago

You can merge !

SceptreOfficial commented 3 years ago

Then it shall be done