CnCNet / xna-cncnet-client

XNA / MonoGame based client for playing classic Command & Conquer games both online and offline with a CnCNet game spawner.
Other
224 stars 85 forks source link

Finalize localization system #409

Closed Metadorius closed 1 year ago

Metadorius commented 1 year ago

TODO

SadPencil commented 1 year ago

I have an idea to define \\, \@, \; and \n (both @ and \n can be used to represent a newline) so that the translation table can write any texts without constraints.


The follwing code is provided by chatgpt. Unfortunately chatgpt is in a high demand and I have not ask him for an encoding code.

Can you help me write a C# code to decode a backslash-escaped string

Certainly! Here's a simple function that takes a string as input and returns a decoded version of the string by replacing any backslash-escaped characters with their unescaped counterparts:

string DecodeString(string input)
{
    // Use a StringBuilder to build the output string
    StringBuilder output = new StringBuilder();

    // Keep track of the current position in the input string
    int pos = 0;

    // Loop through the input string
    while (pos < input.Length)
    {
        // If the current character is a backslash, process the escape sequence
        if (input[pos] == '\\')
        {
            // Get the next character in the string
            pos++;
            if (pos >= input.Length)
            {
                // If there are no more characters, the string is invalid
                throw new ArgumentException("Invalid escape sequence at end of string");
            }

            // Process the escaped character
            switch (input[pos])
            {
                case '\\':
                    output.Append('\\');
                    break;
                case '"':
                    output.Append('"');
                    break;
                case '\'':
                    output.Append('\'');
                    break;
                case 'n':
                    output.Append('\n');
                    break;
                case 'r':
                    output.Append('\r');
                    break;
                case 't':
                    output.Append('\t');
                    break;
                default:
                    // If the character is not a recognized escape sequence, just append it to the output
                    output.Append('\\');
                    output.Append(input[pos]);
                    break;
            }
        }
        else
        {
            // If the character is not a backslash, just append it to the output
            output.Append(input[pos]);
        }

        // Move on to the next character
        pos++;
    }

    // Return the decoded string
    return output.ToString();
}

To use this function, you can pass a string with backslash-escaped characters as the argument, and it will return the decoded version of the string. For example:

string input = "This is a\\tbackslash-escaped\\nstring";
string output = DecodeString(input);
Console.WriteLine(output);

This will output the following string:

This is a   backslash-escaped
string

I hope this helps! Let me know if you have any questions.
Metadorius commented 1 year ago

The \; won't work because INI syntax has no idea of that. Also why bother the ChatGPT if we can just use the built in string methods... (I also have CoPilot so eh 😛 )

SadPencil commented 1 year ago

The \; won't work because INI syntax has no idea of that. Also why bother the ChatGPT if we can just use the built in string methods... (I also have CoPilot so eh 😛 )

Oh right... something like \59 or \semicolon for ;?

Metadorius commented 1 year ago

Oh right... something like \59 or \semicolon for ;?

Maybe. Will have to think about it.

Metadorius commented 1 year ago

Implemented everything remaining. Also made the client read \@ as @ and \semicolon as ; from multiline INI strings.

Rans4ckeR commented 1 year ago

Don't forget to also check the build scripts by running Build-All.ps1 Debug & Build-All.ps1 Release. AfterPublish.targets will still try to copy Localization.dll & Localization.pdb.

Metadorius commented 1 year ago

Don't forget to also check the build scripts by running Build-All.ps1 Debug & Build-All.ps1 Release. AfterPublish.targets will still try to copy Localization.dll & Localization.pdb.

Done, thanks for reminding.

github-actions[bot] commented 1 year ago

Nightly build for this pull request:

SadPencil commented 1 year ago

@Rans4ckeR Could you please help me understand which version of Rampastring.Tools will be included after deleting Localization project? See #393

Rans4ckeR commented 1 year ago

@Rans4ckeR Could you please help me understand which version of Rampastring.Tools will be included after deleting Localization project? See #393

XNAUI pulls in Tools: image

SadPencil commented 1 year ago

@Rans4ckeR Take an example. Suppose XNAUI requires >=2.0.4 and the client requires >=2.1.0. How to explicitly specify this constraint if non of the dependencies has this requirement.

Rans4ckeR commented 1 year ago

@Rans4ckeR Take an example. Suppose XNAUI requires >=2.0.4 and the client requires >=2.1.0. How to explicitly specify this constraint if non of the dependencies has this requirement.

In that case you need to again add a Tools 2.1.0 PackageReference directly to the client (ClientCore for example).

Metadorius commented 1 year ago

Updated.

SadPencil commented 1 year ago

All Environment.NewLine should be replaced with \n as the former is a runtime thing and the compiler can not deal with it. Working on it.

SadPencil commented 1 year ago

Apart from the minor revision https://github.com/Metadorius/xna-cncnet-client/pull/3, there is one last thing to be discussed: should we load the CultureInfo in the client as specified in the translation file? (Add a property in the translation file referring to the CultureInfo code)

Metadorius commented 1 year ago

Apart from the minor revision https://github.com/Metadorius/xna-cncnet-client/pull/3, there is one last thing to be discussed: should we load the CultureInfo in the client as specified in the translation file? (Add a property in the translation file referring to the CultureInfo code)

No, we should not, this is handled by the folder name. Multiple reasons for that:

  1. Loading it from the INI means more file reading, while incremental it still adds to the pile, also easier to just enumerate folders.
  2. CultureInfo codes and translations should be in a dictionary like relation, so that for each unique CultureInfo there should be exactly 0 or 1 translations. Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?
SadPencil commented 1 year ago

Apart from the minor revision Metadorius#3, there is one last thing to be discussed: should we load the CultureInfo in the client as specified in the translation file? (Add a property in the translation file referring to the CultureInfo code)

No, we should not, this is handled by the folder name. Multiple reasons for that:

1. Loading it from the INI means more file reading, while incremental it still adds to the pile, also easier to just enumerate folders.

2. CultureInfo codes and translations should be in a dictionary like relation, so that for each unique CultureInfo there should be exactly 0 or 1 translations. Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?

We are talking about different things. I mean changing CultureInfo.CurrentCulture and CultureInfo.CurrentUICulture. The locale code is used to identify a translation file while the cultureinfo code can be used to set up these two fields.

SadPencil commented 1 year ago

Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?

Although this is another topic, sometimes there is more than one translations for a lanuage and sometimes there can be a fanny and funny translation alongside with a normal one.

In a special case, in China there is a need to replace every word of "China" with a faked country name so that one can broadcasting their game without being censored by gov (ridiculous but it is), so there are actually two zh-CN translations, one of which is the normal one and the other meets the censorship requirement.

Metadorius commented 1 year ago

Ah, I see. I don't think there's much point though? What benefits does that bring?

SadPencil commented 1 year ago

Ah, I see. I don't think there's much point though? What benefits does that bring?

The format of numbers, date & times, and (perhaps, not validated) exception message texts as one need to use English exception text to report the bug to modders.

SadPencil commented 1 year ago

Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?

Although this is another topic, sometimes there is more than one translations for a lanuage and sometimes there can be a fanny and funny translation alongside with a normal one.

In a special case, in China there is a need to replace every word of "China" with a faked country name so that one can broadcasting their game without being censored by gov (ridiculous but it is), so there are actually two zh-CN translations, one of which is the normal one and the other meets the censorship requirement.

I realized this topic is also important and it is better to key a translation file using an ID text to support:

Metadorius commented 1 year ago

Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?

Although this is another topic, sometimes there is more than one translations for a lanuage and sometimes there can be a fanny and funny translation alongside with a normal one.

In a special case, in China there is a need to replace every word of "China" with a faked country name so that one can broadcasting their game without being censored by gov (ridiculous but it is), so there are actually two zh-CN translations, one of which is the normal one and the other meets the censorship requirement.

That all can be done without the need for specifying CultureInfo to match to. Maybe for overriding the CultureInfo of the thread, but not for matching, as neither should not be picked automatically.

SadPencil commented 1 year ago

Making it an INI key breaks that behaviour and opens the door to issues of having multiple translations with the same CultureInfo. Why should one deal with such?

Although this is another topic, sometimes there is more than one translations for a lanuage and sometimes there can be a fanny and funny translation alongside with a normal one. In a special case, in China there is a need to replace every word of "China" with a faked country name so that one can broadcasting their game without being censored by gov (ridiculous but it is), so there are actually two zh-CN translations, one of which is the normal one and the other meets the censorship requirement.

That all can be done without the need for specifying CultureInfo to match to. Maybe for overriding the CultureInfo of the thread, but not for matching, as neither should not be picked automatically.

Yes. These are two topics.

Metadorius commented 1 year ago

I realized this topic is also important and it is better to key a translation file using an ID text to support:

  • zh-CN
  • zh-CN/censorship # otherwise users have to manually replace the files
  • zh-CN/fancy

You can just have different translations for that + BasedOn (assuming it takes relative paths). And the censored translation should be the default one IMO.

Metadorius commented 1 year ago

I mean for safety reasons.

SadPencil commented 1 year ago

Ah, I see. I don't think there's much point though? What benefits does that bring?

The format of numbers, date & times, and (perhaps, not validated) exception message texts as one need to use English exception text to report the bug to modders.

Now there is only one topic remaining. Since the locale code does not have to be a recognizable string, to support loading the CultureInfo an additional field named CultureInfo should be added (to specify the CultureInfo)

Metadorius commented 1 year ago

After giving it some more thought I don't see any benefit in touching CultureInfo. We pick the language, not the culture for dates etc. CurrentCulture is already set by the system, and CurrentUICulture affects only the UI language of the current thread, which we already manage by ourselves, so no point in changing it.

SadPencil commented 1 year ago

After giving it some more thought I don't see any benefit in touching CultureInfo. We pick the language, not the culture for dates etc. CurrentCulture is already set by the system, and CurrentUICulture affects only the UI language of the current thread, which we already manage by ourselves, so no point in changing it.

Then I don't have further comments.

SadPencil commented 1 year ago

It could be better to provide an option to dismiss empty value when generating translation stub. e.g.:

; INI:Maps:MapsMO\Standard\actionreaction:Briefing=
; INI:Maps:MapsMO\Standard\advancedwarfare:Briefing=
; INI:Maps:MapsMO\Standard\advancement:Briefing=
; INI:Maps:MapsMO\Standard\alaskanoutback:Briefing=

These lines are annoying

Metadorius commented 1 year ago

@SadPencil should we ignore them at all (don't translate) or just don't notify the translation about them? I think an empty value should always be empty, so we could just bail early in L10N function.

SadPencil commented 1 year ago

@SadPencil should we ignore them at all (don't translate) or just don't notify the translation about them? I think an empty value should always be empty, so we could just bail early in L10N function.

well I prefer just don't notify the translation about them

Metadorius commented 1 year ago
Metadorius commented 1 year ago

TODO for myself: make MultiColumnListBox adjustable, review the source generator/analyzer warnings.

Metadorius commented 1 year ago

Seems like it needs a rebase. Finally, it looks good otherwise, assuming you've found no issues in testing.

Done. Few people have done tests and we didn't find any more outstanding issues.