Implement language settings from Thirsty CoilSnake - #324
Conversation
Not everything has been fully translated yet like certain console strings. - English - Japanese (日本語) Code by cooprocks123e & Gabbi
|
Is it too much trouble for the language file to be a csv instead of a json? csv would be much easier for translators |
PhoenixBound
left a comment
PhoenixBound
left a comment
There was a problem hiding this comment.
It looks like I never submitted this review last year(!). Sorry about that. By now, neither of us probably fully remembers how this code works... That being said, most of the language-relevant changes are just to use a lot less + with strings and a lot more .format(...).
I already mentioned on Discord how translations require maintenance as text in a program is added and changed. If we do end up merging a localization system, it would be nice to have a translator for any added languages to ping before a release and get things fixed up, to avoid sending the user an exe where many options are in one language and there's a few dialogue boxes in another. (But first we probably need to get an actual regular release schedule, maybe... Jank Jam 4 is getting dangerously close.)
| self.guistrings.get("ask_enable_debug_prompt") | ||
| + self.guistrings.get("coilsnake_is_running") | ||
| + self.guistrings.get("advanced_users"), |
There was a problem hiding this comment.
In all of these places where multiple strings are fetched from the file and concatenated together, there should just be one string. The only reason they were concatenated together in the first place was to avoid making long lines that require a lot of sideways scrolling to read. (Note that some cases relied on Python concatenating adjacent string literals automatically.)
(Other instances: lines 153-155; 169-171; 208-209; 301-302; 339-344; 380-382; 413-415)
| self.guistrings.get("java_following_loc") | ||
| + system_java_exe + "\n\n" | ||
| + "To use this installation of Java, select \"Yes\".\n\n" | ||
| + "To override and instead use a different version of Java, select \"No\".", | ||
| + self.guistrings.get("select_yes") | ||
| + self.guistrings.get("select_yes"), |
There was a problem hiding this comment.
You replaced these two unique lines with the same line repeated twice. Combine into one string and use string formatting:
self.guistrings.get("java_detected_at_following_location").format(system_java_exe)| def create_gui(self): | ||
| self.root = Tk() | ||
| self.root.wm_title("CoilSnake " + information.VERSION) | ||
| self.guistrings.change_language(language_name="en") #replace this with [whatever is in Preferences when we put default language in the preferences stuff] |
There was a problem hiding this comment.
Nit: maybe adjust this comment to begin with TODO: so that it's easier to search for/easier to see in certain text editors
| last_used_temporary_menu_index = 1 | ||
| def add_menu_item_and_get_index(self, menu: Menu, command=None, submenu=None) -> int: | ||
| templabel = f"MyTemp{self.last_used_temporary_menu_index}" | ||
| self.last_used_temporary_menu_index += 1 | ||
| if command: | ||
| menu.add_command(label=templabel, command=command) | ||
| elif submenu: | ||
| menu.add_cascade(label=templabel, menu=submenu) | ||
| item_index = menu.index(templabel) | ||
| return item_index | ||
|
|
||
| def _add_translated_menu_item(self, menu: Menu, label_string_name: str, command=None, submenu=None): | ||
| index = self.add_menu_item_and_get_index(menu, command=command, submenu=submenu) | ||
| def translation_callback(): | ||
| menu.entryconfigure(index, label=self.guistrings.get(label_string_name)) | ||
| self.guistrings.register_callback(translation_callback) |
There was a problem hiding this comment.
Why is this adding a unique temporary label to a menu item...?
Are there problems if you don't put any label on a menu item? If so, does it need to be 100% unique? Would label_string_name itself be unique enough of a label?
| # TODO do some check so that unallocated ranges don't overlap | ||
| cur_begin, cur_end = range | ||
| for prev_begin, prev_end in self.unallocated_ranges: | ||
| if prev_begin <= cur_end and cur_begin <= prev_end: | ||
| raise InvalidArgumentError(f"Couldn't mark range ({cur_begin:#x}, {cur_end:#x}) as free " | ||
| f"because it overlaps with ({prev_begin:#x}, {prev_end:#x})") |
There was a problem hiding this comment.
This is an unrelated change that I already made a PR for (#318). Probably shouldn't be included in this PR. Same goes for the Mother 2 ROM type stuff since Mother 2 support isn't added in this PR
| @@ -0,0 +1,156 @@ | |||
| { | |||
| "coilsnake_name": "まきへびドライ ", | |||
There was a problem hiding this comment.
This is normal CoilSnake, but the translation still refers to it as Thirsty CoilSnake/Makihebi Dry and mentions "non-Mother 2" ROMs instead of "non-EarthBound ROMs." So submitting the translation as-is might not work, especially before CoilSnake even gets Mother 2 support...
| @staticmethod | ||
| def _json_to_translations(json_data): | ||
| missing = "Missing localization string" | ||
| return defaultdict(lambda: missing, json_data) | ||
|
|
||
| @classmethod | ||
| def _load_language(cls, language: Language): | ||
| try: | ||
| with open(language.get_json_path(), "r", encoding="utf-8") as file: | ||
| json_data = json.load(file) | ||
| return cls._json_to_translations(json_data) | ||
| except: | ||
| return None | ||
|
|
There was a problem hiding this comment.
If I understand this correctly, this will make all missing strings in a JSON file say Missing localization string? This could work, but it seems like a lot of existing software tries to display the original language's string if a translation can't be found. There's no perfect solution here obviously, just wanted to start a discussion.
3 participants
Not everything has been fully translated yet like certain console strings.
Code by cooprocks123e & Gabbi