Don't add android localization strings into ios files and vice versa #1793

pastk · 2022-01-07T11:23:53Z

pastk commented

2022-01-07 11:23:53 +00:00

Issue #587

pastk reviewed 2022-01-07 11:26:11 +00:00

tools/unix/generate_localizations.sh

					
				@ -63,3 +62,1 @@

				"$TWINE" generate-all-localization-files --include translated --format android "$MERGED_FILE" "$OMIM_PATH/android/res/"

				"$TWINE" generate-all-localization-files --format apple "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --format apple-plural "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --include translated --format android --untagged --tags android "$MERGED_FILE" "$OMIM_PATH/android/res/"

pastk commented

2022-01-07 11:26:10 +00:00

can't use twine validation still as it doesn't allow dots . in definition names (all our types use dots e.g. like [type.highway.footway]

can't use twine validation still as it doesn't allow dots `.` in definition names (all our types use dots e.g. like `[type.highway.footway]`

biodranik (Migrated from github.com) reviewed 2022-01-07 11:39:45 +00:00

tools/unix/generate_localizations.sh

					
				@ -63,3 +62,1 @@

				"$TWINE" generate-all-localization-files --include translated --format android "$MERGED_FILE" "$OMIM_PATH/android/res/"

				"$TWINE" generate-all-localization-files --format apple "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --format apple-plural "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --include translated --format android --untagged --tags android "$MERGED_FILE" "$OMIM_PATH/android/res/"

biodranik (Migrated from github.com) commented

2022-01-07 11:39:45 +00:00

Let's patch our twine fork :)

pastk reviewed 2022-01-07 11:44:25 +00:00

tools/unix/generate_localizations.sh

					
				@ -63,3 +62,1 @@

				"$TWINE" generate-all-localization-files --include translated --format android "$MERGED_FILE" "$OMIM_PATH/android/res/"

				"$TWINE" generate-all-localization-files --format apple "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --format apple-plural "$MERGED_FILE" "$OMIM_PATH/iphone/Maps/LocalizedStrings/"

				"$TWINE" generate-all-localization-files --include translated --format android --untagged --tags android "$MERGED_FILE" "$OMIM_PATH/android/res/"

pastk commented

2022-01-07 11:44:25 +00:00

Ok, maybe I'll do it sometime. Its not really important as our strings_utils.py does a good validation job already.

Ok, maybe I'll do it sometime. Its not really important as our `strings_utils.py` does a good validation job already.

pastk commented

2022-01-07 12:00:11 +00:00

I've regenerated localizations, rebuilt and ran the android app, it crashed when I tapped search icon:

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: app.organicmaps.debug, PID: 2805
    java.lang.IllegalStateException: Can't get string resource id for category:hotel

So some needed strings were removed..
I'll look into it.

I've regenerated localizations, rebuilt and ran the android app, it crashed when I tapped search icon: ``` E/AndroidRuntime: FATAL EXCEPTION: main Process: app.organicmaps.debug, PID: 2805 java.lang.IllegalStateException: Can't get string resource id for category:hotel ``` So some needed strings were removed.. I'll look into it.

pastk reviewed 2022-01-07 13:35:12 +00:00

tools/python/clean_strings_txt.py

pastk commented

2022-01-07 13:35:11 +00:00

What is not cool is that data/hardcoded_categories.txt is actually not used by the code, but the contents are duplicated in search/displayed_categories.cpp - it should be loaded from the file instead.

What is not cool is that `data/hardcoded_categories.txt` is actually not used by the code, but the contents are duplicated in `search/displayed_categories.cpp` - it should be loaded from the file instead.

pastk commented

2022-01-07 13:48:07 +00:00

The search works now. I haven't been able to crash the app so far :)
But given that android will crash if a string is not found (I guess iOS will not crash but display the key?) - its a risky update. I suggest we issue a beta with this change first.

The search works now. I haven't been able to crash the app so far :) But given that android will crash if a string is not found (I guess iOS will not crash but display the key?) - its a risky update. I suggest we issue a beta with this change first.

biodranik (Migrated from github.com) reviewed 2022-01-07 13:59:07 +00:00

data/strings/strings.txt

biodranik (Migrated from github.com) commented

2022-01-07 13:59:07 +00:00

Is it really necessary to remove a space?

biodranik (Migrated from github.com) reviewed 2022-01-07 14:14:24 +00:00

tools/python/clean_strings_txt.py

					
				@ -20,3 +20,3 @@

				OMIM_ROOT = ""

				CORE_RE = re.compile(r'GetLocalizedString\(\"(.*?)\"\)')

				CORE_RE = re.compile(r'GetLocalizedString\("(.*?)"\)')

biodranik (Migrated from github.com) commented

2022-01-07 14:14:24 +00:00

Use single quotes here and below for simlicity.

pastk reviewed 2022-01-07 14:15:17 +00:00

data/strings/strings.txt

pastk commented

2022-01-07 14:15:17 +00:00

yes, twine fails to detect the ios tag in the tags = android, ios string

yes, twine fails to detect the `ios` tag in the `tags = android, ios` string

biodranik (Migrated from github.com) reviewed 2022-01-07 14:23:51 +00:00

tools/python/clean_strings_txt.py

					
				@ -20,3 +20,3 @@

				OMIM_ROOT = ""

				CORE_RE = re.compile(r'GetLocalizedString\(\"(.*?)\"\)')

				CORE_RE = re.compile(r'GetLocalizedString\("(.*?)"\)')

biodranik (Migrated from github.com) commented

2022-01-07 14:19:25 +00:00

We tried to avoid NSLocalizedString in favor of our own macro L

tools/python/clean_strings_txt.py

biodranik (Migrated from github.com) commented

2022-01-07 14:23:37 +00:00

That problem was introduced by the author of the python script. The proper approach should be to delete data/hardcoded_categories.txt and to parse values from the cpp file.

biodranik (Migrated from github.com) reviewed 2022-01-07 14:25:37 +00:00

data/strings/strings.txt

biodranik (Migrated from github.com) commented

2022-01-07 14:25:36 +00:00

So it was our scripts' fault that introduced this extra space?

pastk reviewed 2022-01-07 14:29:34 +00:00

data/strings/strings.txt

pastk commented

2022-01-07 14:29:34 +00:00

yeap, without the space it works as expected

pastk reviewed 2022-01-07 14:34:02 +00:00

tools/python/clean_strings_txt.py

pastk commented

2022-01-07 14:34:02 +00:00

Hmm, I see.
Though having the categories not hardcoded, but in an editable resource file is overall a better approach, isn't it?

Hmm, I see. Though having the categories not hardcoded, but in an editable resource file is overall a better approach, isn't it?

biodranik (Migrated from github.com) reviewed 2022-01-07 14:54:38 +00:00

tools/python/clean_strings_txt.py

biodranik (Migrated from github.com) commented

2022-01-07 14:54:38 +00:00

The ideal solution would be to load them from categories.txt directly (but some custom starting UI order should be also supported, which can change dynamically later when a user uses something more often).

But using a separate file, unsynced with the code, just for scripts, is definitely the wrong way.

The ideal solution would be to load them from categories.txt directly (but some custom starting UI order should be also supported, which can change dynamically later when a user uses something more often). But using a separate file, unsynced with the code, just for scripts, is definitely the wrong way.

pastk reviewed 2022-01-07 15:09:20 +00:00

tools/python/clean_strings_txt.py

pastk commented

2022-01-07 15:09:20 +00:00

Ok, we'll get to it sometime
#1795

Ok, we'll get to it sometime #1795

pastk commented

2022-01-07 21:45:46 +00:00

The search works now. I haven't been able to crash the app so far :) But given that android will crash if a string is not found (I guess iOS will not crash but display the key?) - its a risky update. I suggest we issue a beta with this change first.

So what do you think we should do with it?
If you'd rather not merge it now then I can remove the risky use of android tags in generate_localizations.sh from this PR for now so that we can merge the rest.

> The search works now. I haven't been able to crash the app so far :) But given that android will crash if a string is not found (I guess iOS will not crash but display the key?) - its a risky update. I suggest we issue a beta with this change first. So what do you think we should do with it? If you'd rather not merge it now then I can remove the risky use of android tags in `generate_localizations.sh` from this PR for now so that we can merge the rest.

biodranik (Migrated from github.com) reviewed 2022-01-08 09:45:33 +00:00

tools/python/clean_strings_txt.py

biodranik (Migrated from github.com) commented

2022-01-08 09:45:33 +00:00

Please insert todo here about reading from cpp file.

biodranik (Migrated from github.com) approved these changes 2022-01-08 09:46:19 +00:00

pastk reviewed 2022-01-08 09:53:29 +00:00

tools/python/clean_strings_txt.py

pastk commented

2022-01-08 09:53:29 +00:00

Done!

biodranik commented

2022-01-08 10:55:27 +00:00

(Migrated from github.com)

So what do you think we should do with it?
If you'd rather not merge it now then I can remove the risky use of android tags in generate_localizations.sh from this PR for now so that we can merge the rest.

Merge. Test. Fix.

> So what do you think we should do with it? > If you'd rather not merge it now then I can remove the risky use of android tags in `generate_localizations.sh` from this PR for now so that we can merge the rest. Merge. Test. Fix.

pastk commented

2022-01-08 10:57:36 +00:00

Ok, its ready to merge then.

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik