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

Bulk i18n of epicmenu widget options #5237

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

moreginger
Copy link
Contributor

No description provided.

@moreginger
Copy link
Contributor Author

moreginger commented Apr 4, 2024

This doesn't cover everything, but it's a good start.
It seems to start and I went through all the settings pages - they seem OK?

using this script and some fixup:

import argparse
import json
import re

options_block_regex = re.compile(r'^options_(path|order)?\s*=')
options_start_regex = re.compile(r'^options\s*=\s*{')
widget_name_regex = re.compile(r'name\s*=\s*[\'"]([^\'"]+)[\'"],')
option_start_regex = re.compile(r'^\s+([a-zA-Z0-9_]+)\s*=\s*{')
option_string_regex = re.compile(r'^(\s+)(name|desc)\s*=\s*[\'"]([^\'"]+)[\'"],\s*$')

parser = argparse.ArgumentParser(prog='i18n_options')
parser.add_argument('filename')
parser.add_argument('--i18nfile', help='Path to translations file')

args = parser.parse_args()

filename = args.filename
i18nfile = args.i18nfile

def cleanup_string(s):
    return re.sub(r'\W+', '', s).replace('_', '').lower()

widget_prefix = None

new_content = []

epicmenu_json = None
with open(i18nfile) as json_file:
    epicmenu_json = json.load(json_file)

option_name = None

print('processing ' + filename + '...')

with open(filename) as lua_file:
    wrote_prefix = False
    in_options = False
    for line in lua_file.readlines():
        if 'i18nPrefix' in line:
            print('i18nPrefix already exists in ' + filename + '. Exiting...')
            exit(0)

        if not wrote_prefix and options_block_regex.search(line):
            new_content.append("i18nPrefix = '" + widget_prefix + "'\n")
            wrote_prefix = True

        if not widget_prefix and widget_name_regex.search(line):
            widget_prefix = cleanup_string(widget_name_regex.search(line).group(1)) + '_'
        elif options_start_regex.search(line):
            in_options = True
        elif line.startswith('}'):
            in_options = False
        elif line.endswith('},'):
            option_name = False
        elif option_start_regex.search(line):
            match = option_start_regex.search(line)
            option_name = match.group(1).lower() + '_option'
        elif in_options and option_string_regex.search(line):
            match = option_string_regex.search(line)
            content = match.group(3)
            if match.group(2) == 'name':
                epicmenu_json[widget_prefix + option_name] = content
                line = None
            elif match.group(2) == 'desc':
                epicmenu_json[widget_prefix + option_name + '_desc'] = content
                line = None

        if line:
            new_content.append(line)

if not option_name:
    print('No options found in ' + filename + '. Exiting...')
    exit(0)

with open(filename, 'w') as lua_file:
    lua_file.write(''.join(new_content))

with open(i18nfile, 'w') as json_file:
    json.dump(epicmenu_json, json_file, indent='\t', sort_keys=True, separators=(',', ' : '))

@GoogleFrog
Copy link
Contributor

I wish translations didn't make the rest of development so inconvenient.

@moreginger
Copy link
Contributor Author

They are a pain, I've gotten used to them being annoying. One option is to have the English translation with the code, and not in a resources file. But you'd have to extract and store it for switching back to English.

enableBloom = {type = 'bool', name = 'Apply Bloom Effect (HDR Only)', value = true,},
enableHDR = {
type = 'bool',
alue = true,
Copy link
Member

Choose a reason for hiding this comment

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

alue. Extra scary because there could be more errors like this lurking around the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting this.

Maybe the answer is: run script on another copy of ZK, review differences?
For ~100 files there should be no change.

Or maybe review only added/changed lines and assume deleted are OK, if there's a way to do that.

@@ -1228,6 +1260,7 @@
"unitmarkerzerok_disableall_desc" : "Mark nothing.",
"unitmarkerzerok_enableall" : "Enable All",
"unitmarkerzerok_enableall_desc" : "Marks all listed units.",
"unitmarkerzerok_unitslabel" : "unitslabel",
Copy link
Member

Choose a reason for hiding this comment

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

Label name is probably technical and shouldn't be put into i18n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm interested in what the labels are. I'll have a look later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this one is weird, and good spot. It looks like the "value" is the text that we'd want here. I searched for "unitslabel" and found no references, it could be constructed of course. But as a label I find it hard to imagine the string has a technical purpose.

	unitslabel = {name="unitslabel", type = 'label', value = "Individual Toggles", path = options_path},

Generally, labels are UI elements as you no doubt know, and the i18n of them is working in other places.

"combooverheadfreecameraexperimental_option_followoutscrollspeed" : "Off Screen Tracking Speed",
"combooverheadfreecameraexperimental_option_followoutscrollspeed_desc" : "Tracking speed while cursor is off-screen. \\n\\nRecommend: Highest (prevent missed action)",
"combooverheadfreecameraexperimental_option_followzoominspeed" : "Zoom-in Speed",
"combooverheadfreecameraexperimental_option_followzoominspeed_desc" : "Zoom-in speed when cursor is on-screen. Default: 50%",
Copy link
Member

@sprunk sprunk Apr 5, 2024

Choose a reason for hiding this comment

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

% is a format specifier in i18n, it should become %%

@sprunk
Copy link
Member

sprunk commented Apr 5, 2024

Monoliths are hard to review and there's a bunch of issues as above. I think ideally there would be a bunch of commits with, say, 10 widgets per commit and then they would be easy to review and mergeable individually.

@moreginger
Copy link
Contributor Author

moreginger commented Apr 5, 2024

You can safely replace all the % with %% in that file.

Monoliths are hard to review

Hmm I thought you'd prefer to get it all done in one.

I'm not sure that I have the staying power to create a series of 14 PRs. Especially after having done a conversion twice already. I appreciate it's a massive pain to review the large change.

I think maybe if I make a couple of changes to the script it might be best if you use it in batches you are happy with. Perhaps you can feel happier that it is acting in a sensible way? I can let you know the 2 widgets that it breaks. That will leave 10 or 20 that either partially or don't i18n without changes, but we can chase those later.

@moreginger
Copy link
Contributor Author

You might also want to review the technique. I wonder if it's better not to move the English translation to the resources file, @GoogleFrog didn't seem happy with it. I can imagine how that might work - providing the current value as a default when requesting translations, and store it as English if you don't have an English translation stored, but I think you'd want to consider the API carefully together. If I start trying to guess what you want then we'll be here a long time!

I guess that makes it harder to translate them though 🤷

@@ -35,7 +36,6 @@ options_order = {
-- toggle grabinput easily.
options = {
grabinput = {
name = "Lock Cursor to Window",
tooltip = "Prevents the cursor from leaving the Window/Screen",
Copy link
Member

Choose a reason for hiding this comment

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

tooltip seems unhandled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, good spot. I'm not sure if options work at all, certainly my script isn't smart enough to do them properly.
I guess we really need to cut this into smaller pieces.

@GoogleFrog
Copy link
Contributor

@sprunk I hope you're not waiting on me for anything.

@sprunk
Copy link
Member

sprunk commented May 4, 2024

No, I'm waiting for chore mana from myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants