[android] Reduce size of apk #3619

rtsisyk · 2022-10-13T16:43:05Z

Jean-BaptisteC commented

2022-10-13 16:43:05 +00:00

(Migrated from github.com)

This PR add resConfig in build.gradle to remove unused strings in librairies.
organicmaps/organicmaps#3584 (comment)

APK have lost 600Ko ~

This PR works with per-app language.

This PR add resConfig in build.gradle to remove unused strings in librairies. https://git.omaps.dev/organicmaps/organicmaps/pulls/3584#issuecomment-1276473507 APK have lost 600Ko ~ This PR works with per-app language.

rtsisyk reviewed 2022-10-14 08:32:46 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

rtsisyk commented

2022-10-14 08:32:46 +00:00

zh_TW or zh-rTW?

Jean-BaptisteC (Migrated from github.com) reviewed 2022-10-14 15:39:38 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

Jean-BaptisteC (Migrated from github.com) commented

2022-10-14 15:39:38 +00:00

zh_TW take in zh-rTW during compilation.
I have use per-app language and strings in values_fr-rCA.xml are available in app.

zh_TW take in zh-rTW during compilation. I have use per-app language and strings in values_fr-rCA.xml are available in app.

rtsisyk reviewed 2022-10-18 06:49:35 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

rtsisyk commented

2022-10-18 06:49:35 +00:00

OK, I tested that en-rGB works

OK, I tested that `en-rGB` works

rtsisyk approved these changes 2022-10-18 06:50:09 +00:00

biodranik (Migrated from github.com) requested changes 2022-10-18 21:57:47 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-10-18 21:57:36 +00:00

Please use single quotes for constants without vars inside them.
Is it possible to load them from a config file?
Is there a way to automatically calculate and set these languages based on our real translations?
Please update the documentation so that when a new language is added, this line should be updated too.

1. Please use single quotes for constants without vars inside them. 2. Is it possible to load them from a config file? 3. Is there a way to automatically calculate and set these languages based on our real translations? 4. Please update the documentation so that when a new language is added, this line should be updated too.

Jean-BaptisteC (Migrated from github.com) reviewed 2022-10-19 16:04:35 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

Jean-BaptisteC (Migrated from github.com) commented

2022-10-19 16:04:35 +00:00

Please use single quotes for constants without vars inside them.

Yes :)

Is it possible to load them from a config file?

Yes, we can add resConfig in gradle.properties Check this

Is there a way to automatically calculate and set these languages based on our real translations?

Not, you need to specify which language do you want integrate in Android build

Please update the documentation so that when a new language is added, this line should be updated too.

Do you want i add new section in TRANSLATIONS.md ? Or just specify a new line in Translation files section with link to build.gradle with description List of languages integrate in Android build

> 1. Please use single quotes for constants without vars inside them. Yes :) > 2. Is it possible to load them from a config file? Yes, we can add resConfig in [gradle.properties](https://github.com/organicmaps/organicmaps/blob/master/android/gradle.properties) Check [this](https://medium.com/livefront/improve-android-build-times-by-using-resconfigs-99177a3c6d90) > 3. Is there a way to automatically calculate and set these languages based on our real translations? Not, you need to specify which language do you want integrate in Android build > 4. Please update the documentation so that when a new language is added, this line should be updated too. Do you want i add new section in [TRANSLATIONS.md](https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md) ? Or just specify a new line in Translation files section with link to build.gradle with description `List of languages integrate in Android build`

rtsisyk reviewed 2022-10-20 08:11:33 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

rtsisyk commented

2022-10-20 08:11:33 +00:00

Please use single quotes for constants without vars inside them.

Yes :)

Thanks! This is not documented, but here is a strong belief that single quotes are easier to read than double quota, which is a case in Bash. I filed a follow-up ticket to research organicmaps/organicmaps#3684.

Is it possible to load them from a config file?

Yes, we can add resConfig in gradle.properties Check this

I don't see any difference between having this list in gradle.properties vs build.gradle. It doesn't change things significantly. Both options look like the same for me.

@biodranik do you have any better ideas?

Is there a way to automatically calculate and set these languages based on our real translations?

Not, you need to specify which language do you want integrate in Android build

Generating this list dynamically based on values-* directory sounds complicated to me. It is just not worth it.

Please update the documentation so that when a new language is added, this line should be updated too.

Do you want i add new section in TRANSLATIONS.md ? Or just specify a new line in Translation files section with link to build.gradle with description List of languages integrate in Android build

I don't see a good place where you can add a comment in translation files (string.txt). TRANSLATIONS.md should be fine.

> > 1. Please use single quotes for constants without vars inside them. > > Yes :) Thanks! This is not documented, but here is a strong belief that single quotes are easier to read than double quota, which is a case in Bash. I filed a follow-up ticket to research https://git.omaps.dev/organicmaps/organicmaps/issues/3684. > > > 2. Is it possible to load them from a config file? > > Yes, we can add resConfig in [gradle.properties](https://github.com/organicmaps/organicmaps/blob/master/android/gradle.properties) Check [this](https://medium.com/livefront/improve-android-build-times-by-using-resconfigs-99177a3c6d90) I don't see any difference between having this list in `gradle.properties` vs `build.gradle`. It doesn't change things significantly. Both options look like the same for me. @biodranik do you have any better ideas? > > > 3. Is there a way to automatically calculate and set these languages based on our real translations? > > Not, you need to specify which language do you want integrate in Android build > Generating this list dynamically based on `values-*` directory sounds complicated to me. It is just not worth it. > > 4. Please update the documentation so that when a new language is added, this line should be updated too. > > Do you want i add new section in [TRANSLATIONS.md](https://github.com/organicmaps/organicmaps/blob/master/docs/TRANSLATIONS.md) ? Or just specify a new line in Translation files section with link to build.gradle with description `List of languages integrate in Android build` I don't see a good place where you can add a comment in translation files (string.txt). TRANSLATIONS.md should be fine.

biodranik (Migrated from github.com) reviewed 2022-10-20 08:46:45 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-10-20 08:46:44 +00:00

I have a better idea. If languages can be loaded from file, let's update tools/unix/generate_localizations.sh to autogenerate this file with the list of all supported languages, for example, in strings.txt.

rtsisyk reviewed 2022-10-30 10:34:41 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

rtsisyk commented

2022-10-30 10:34:41 +00:00

We don't add new languages every day. Can we keep it simple?

biodranik (Migrated from github.com) reviewed 2022-10-30 12:35:35 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-10-30 12:35:35 +00:00

It is very easy to forget it. With the help of the community, I expect many languages to be added. Let's do it properly from the start. Once and for all.

biodranik (Migrated from github.com) reviewed 2022-11-11 21:13:09 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-11-11 21:13:09 +00:00

This one-liner can be inserted into generate_localizations.sh to autogenerate the include config for android:

sed -nE "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/'\1',/p" data/strings/strings.txt | sort -u | tr '\n' ' ' | sed -E 's/, $//'
'ar', 'be', 'bg', 'ca', 'cs', 'da', 'de', 'el', 'en', 'en-GB', 'es', 'es-MX', 'et', 'eu', 'fa', 'fi', 'fr', 'fr-CA', 'he', 'hi', 'hu', 'id', 'in', 'it', 'ja', 'ko', 'mr', 'nb', 'ne', 'nl', 'pl', 'pt', 'pt-BR', 'ro', 'ru', 'sk', 'sv', 'sw', 'th', 'tr', 'uk', 'vi', 'zh-Hans', 'zh-Hant'

This one-liner can be inserted into generate_localizations.sh to autogenerate the include config for android: ```bash sed -nE "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/'\1',/p" data/strings/strings.txt | sort -u | tr '\n' ' ' | sed -E 's/, $//' 'ar', 'be', 'bg', 'ca', 'cs', 'da', 'de', 'el', 'en', 'en-GB', 'es', 'es-MX', 'et', 'eu', 'fa', 'fi', 'fr', 'fr-CA', 'he', 'hi', 'hu', 'id', 'in', 'it', 'ja', 'ko', 'mr', 'nb', 'ne', 'nl', 'pl', 'pt', 'pt-BR', 'ro', 'ru', 'sk', 'sv', 'sw', 'th', 'tr', 'uk', 'vi', 'zh-Hans', 'zh-Hant' ```

Jean-BaptisteC (Migrated from github.com) reviewed 2022-11-11 22:06:01 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

Jean-BaptisteC (Migrated from github.com) commented

2022-11-11 22:06:01 +00:00

Can you give more information about what doing these commands, to better understand what i need to add in build.gradle.
Is it ok, if i add your commands just in end file before rm in .generate_localizations.sh ?

Can you give more information about what doing these commands, to better understand what i need to add in build.gradle. Is it ok, if i add your commands just in end file before rm in .generate_localizations.sh ?

biodranik (Migrated from github.com) reviewed 2022-11-11 22:19:25 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-11-11 22:19:24 +00:00

The first line is the command.
The second line is its output.

You need to include a file with these langs into build.gradle instead of hard-coding languages there, and generate this file automatically from the mentioned script.

The first line is the command. The second line is its output. You need to include a file with these langs into build.gradle instead of hard-coding languages there, and generate this file automatically from the mentioned script.

Jean-BaptisteC (Migrated from github.com) reviewed 2022-11-12 09:45:53 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

Jean-BaptisteC (Migrated from github.com) commented

2022-11-12 09:45:53 +00:00

List of languages is include in gradle.properties
resConfig call gradle.properties (It's more simple solution, i have try with generate txt file, but i didn't have find how import content of file and use in resConfig)
List of languages is more longer and when i try to build, compilation fail, can you check ?
More logs here :https://github.com/Jean-BaptisteC/organicmaps/actions/runs/3450378278/jobs/5758934308

- List of languages is include in gradle.properties - resConfig call gradle.properties (It's more simple solution, i have try with generate txt file, but i didn't have find how import content of file and use in resConfig) List of languages is more longer and when i try to build, compilation fail, can you check ? More logs here :https://github.com/Jean-BaptisteC/organicmaps/actions/runs/3450378278/jobs/5758934308

biodranik (Migrated from github.com) requested changes 2022-11-12 09:49:53 +00:00

android/build.gradle

					
				@ -160,6 +160,8 @@ android {

				    buildConfigField 'String', 'SUPPORT_MAIL', '"android@organicmaps.app"'

biodranik (Migrated from github.com) commented

2022-11-12 09:46:38 +00:00

I would name it supportedLocalizations

tools/unix/generate_localizations.sh

biodranik (Migrated from github.com) commented

2022-11-12 09:47:59 +00:00

It will add the line each time the script is run. The robust implementation should add or replace existing line if it's already there.

It will add the line _each time_ the script is run. The robust implementation should _add or replace existing_ line if it's already there.

biodranik (Migrated from github.com) commented

2022-11-12 09:48:47 +00:00

SUPPORTED_LOCALIZATIONS=$(sed -nE "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/'\1',/p" "${OMIM_PATH}/data/strings/strings.txt" | sort -u | tr '\n' ' ' | sed -E 's/, $//')

```suggestion SUPPORTED_LOCALIZATIONS=$(sed -nE "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/'\1',/p" "${OMIM_PATH}/data/strings/strings.txt" | sort -u | tr '\n' ' ' | sed -E 's/, $//') ```

biodranik commented

2022-11-12 09:54:03 +00:00

(Migrated from github.com)

Looks like gradle can't read list of values from gradle.properties file. You may need to change the script to generate string like this: en,be,ru,uk and then assign it as an array like this: https://stackoverflow.com/a/49614449

Looks like gradle can't read list of values from gradle.properties file. You may need to change the script to generate string like this: `en,be,ru,uk` and then assign it as an array like this: https://stackoverflow.com/a/49614449

Jean-BaptisteC commented

2022-11-12 16:26:41 +00:00

(Migrated from github.com)

I have fixed problems during compilation (space is not allow by gradle, and we need to - replace by _) and i have already add list in gradle.properties.
I have try to fixed with add new commands in pipeline but linux say too many arguments.
I have try with tr in new line ("$SUPPORTED_LOCALIZATIONS"=$(echo "$SUPPORTED_LOCALIZATIONS" | tr '-' '_' | tr -d " ")) but i have others errors :(

I missing knowledge about linux scripting, finish PR can be take a lot of time.

I have fixed problems during compilation (space is not allow by gradle, and we need to - replace by _) and i have already add list in gradle.properties. I have try to fixed with add new commands in pipeline but linux say too many arguments. I have try with tr in new line (`"$SUPPORTED_LOCALIZATIONS"=$(echo "$SUPPORTED_LOCALIZATIONS" | tr '-' '_' | tr -d " ")`) but i have others errors :( I missing knowledge about linux scripting, finish PR can be take a lot of time.

biodranik (Migrated from github.com) reviewed 2022-11-19 13:23:52 +00:00

tools/unix/generate_localizations.sh

biodranik (Migrated from github.com) commented

2022-11-19 13:23:51 +00:00

You definitely can generate the necessary list format in the above command, instead of fixing it here. Or no?

Jean-BaptisteC commented

2022-11-19 13:24:48 +00:00

(Migrated from github.com)

I have finish script bash:

I have replace - by _
I have remove space between languages and remove quotes
I have update list of languages in gradle.properties with latest languages from master
I have add condition to update gradle.properties only if languages have changed (To compare, i have add new variable with TEST like name, i don't have find good name for this)
I have used https://www.shellcheck.net/ to validate syntax

I have finish script bash: - I have replace `-` by `_` - I have remove space between languages and remove quotes - I have update list of languages in gradle.properties with latest languages from master - I have add condition to update gradle.properties only if languages have changed (To compare, i have add new variable with TEST like name, i don't have find good name for this) - I have used https://www.shellcheck.net/ to validate syntax

Jean-BaptisteC (Migrated from github.com) reviewed 2022-11-19 13:25:51 +00:00

tools/unix/generate_localizations.sh

Jean-BaptisteC (Migrated from github.com) commented

2022-11-19 13:25:50 +00:00

Unix tell me too many arguments

Jean-BaptisteC (Migrated from github.com) reviewed 2022-11-26 08:33:06 +00:00

tools/unix/generate_localizations.sh

Jean-BaptisteC (Migrated from github.com) commented

2022-11-26 08:33:05 +00:00

Fixed :)

biodranik (Migrated from github.com) requested changes 2022-11-26 10:45:16 +00:00

biodranik (Migrated from github.com) left a comment

Will it work if gradle.properties is empty? Will sed in L73 properly add it at the end?

tools/unix/generate_localizations.sh

biodranik (Migrated from github.com) commented

2022-11-26 10:43:32 +00:00

The shorter version is sed -nEe "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/\1/p" "data/strings/strings.txt" | sort -u | tr '\n' ',' | sed -e 's/-/_/g'

The shorter version is `sed -nEe "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/\1/p" "data/strings/strings.txt" | sort -u | tr '\n' ',' | sed -e 's/-/_/g'`

biodranik (Migrated from github.com) commented

2022-11-26 10:43:41 +00:00

No indent.

biodranik (Migrated from github.com) commented

2022-11-26 10:43:48 +00:00

if [ "$SUPPORTED_LOCALIZATIONS" != "$TEST" ]; then

```suggestion if [ "$SUPPORTED_LOCALIZATIONS" != "$TEST" ]; then ```

Jean-BaptisteC (Migrated from github.com) reviewed 2022-11-26 10:59:07 +00:00

tools/unix/generate_localizations.sh

Jean-BaptisteC (Migrated from github.com) commented

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

This version add comma after last element :(

Jean-BaptisteC commented

2022-11-26 11:00:52 +00:00

(Migrated from github.com)

Will it work if gradle.properties is empty? Will sed in L73 properly add it at the end?

Do you want i add condition with grep to check if line existed in gradle.properties ?

> Will it work if gradle.properties is empty? Will sed in L73 properly add it at the end? Do you want i add condition with grep to check if line existed in gradle.properties ?

biodranik (Migrated from github.com) reviewed 2022-11-26 11:03:23 +00:00

tools/unix/generate_localizations.sh

biodranik (Migrated from github.com) commented

2022-11-26 11:03:23 +00:00

Just add -e 's/,$//' to the last sed invocation.

Just add `-e 's/,$//'` to the last sed invocation.

biodranik (Migrated from github.com) reviewed 2022-11-26 11:09:10 +00:00

android/gradle.properties

					
				@ -9,2 +9,4 @@

				android.enableJetifier=true

				android.native.buildOutput=verbose

				# Autogenerated by tools/unix/generate_localizations.sh

				supportedLocalizations=ar,be,bg,ca,cs,da,de,el,en,en_GB,es,es_MX,et,eu,fa,fi,fr,fr_CA,he,hi,hu,id,in,it,ja,ko,mr,nb,ne,nl,pl,pt,pt_BR,ro,ru,sk,sv,sw,th,tr,uk,vi,zh_Hans,zh_Hant

biodranik (Migrated from github.com) commented

2022-11-26 11:05:30 +00:00

Can we add a comment above? Like
# Autogenerated by tools/unix/generate_localizations.sh
?

Can we add a comment above? Like `# Autogenerated by tools/unix/generate_localizations.sh` ?

tools/unix/generate_localizations.sh

biodranik (Migrated from github.com) commented

2022-11-26 11:08:59 +00:00

sed -i won't work on Mac (it requires -i '' syntax to avoid creating a backup file).

Does it make sense to load gradle.properties content into a variable, and do all replacements there, if necessary, and then overwrite the original file?

sed -i won't work on Mac (it requires `-i ''` syntax to avoid creating a backup file). Does it make sense to load gradle.properties content into a variable, and do all replacements there, if necessary, and then overwrite the original file?

Jean-BaptisteC commented

2022-11-28 21:46:32 +00:00

(Migrated from github.com)

I have fixed all things on my computer, but i can't rebase my PR because Github Desktop have generated merge branch commit can you help me ?
I have added commentary on gradle.properties and fixed script

# Generate list of languages and add list in gradle.properties to be used in build.gradle in resConfig
SUPPORTED_LOCALIZATIONS="supportedLocalizations="$(sed -nEe "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/\1/p" "data/strings/strings.txt" | sort -u | tr '\n' ',' | sed -e 's/-/_/g' -e 's/,$//')
if [ "$SUPPORTED_LOCALIZATIONS" != "$(grep listLanguages "$OMIM_PATH/android/gradle.properties")" ]; then
	sed -i 's/supportedLocalizations.*/'"${SUPPORTED_LOCALIZATIONS}"'/' "$OMIM_PATH/android/gradle.properties"
fi

I have fixed all things on my computer, but i can't rebase my PR because Github Desktop have generated merge branch commit can you help me ? I have added commentary on gradle.properties and fixed script ``` # Generate list of languages and add list in gradle.properties to be used in build.gradle in resConfig SUPPORTED_LOCALIZATIONS="supportedLocalizations="$(sed -nEe "s/ +([a-zA-Z]{2}(-[a-zA-Z]{2,})?) = .*$/\1/p" "data/strings/strings.txt" | sort -u | tr '\n' ',' | sed -e 's/-/_/g' -e 's/,$//') if [ "$SUPPORTED_LOCALIZATIONS" != "$(grep listLanguages "$OMIM_PATH/android/gradle.properties")" ]; then sed -i 's/supportedLocalizations.*/'"${SUPPORTED_LOCALIZATIONS}"'/' "$OMIM_PATH/android/gradle.properties" fi ```

biodranik commented

2022-11-28 23:24:05 +00:00

(Migrated from github.com)

You should learn two important things we actively use here in OM development:

Rebase
Resolving merge conflicts (they can be easily fixed in any text editor).

I've fixed that conflict for you using Github web interface.

You should learn two important things we actively use here in OM development: 1. Rebase 2. Resolving merge conflicts (they can be easily fixed in any text editor). I've fixed that conflict for you using Github web interface.

Jean-BaptisteC commented

2022-11-29 17:43:32 +00:00

(Migrated from github.com)

I have planned to rebase branch but i have need to tests on other branch to be sure i don't break config branch, because i always use Github Desktop, i have never use git CLI.

Thanks to have resolve conflicts :)

I have planned to rebase branch but i have need to tests on other branch to be sure i don't break config branch, because i always use Github Desktop, i have never use git CLI. Thanks to have resolve conflicts :)

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

No reviewers

rtsisyk

biodranik