lvgl / lv_i18n

Internationalization (i18n) for LVGL
MIT License
58 stars 17 forks source link

use lv_i18n_language_pack as default language pack during init #36

Closed glory-man closed 1 year ago

glory-man commented 3 years ago

so we can call lv_i18n_init(NULL); without using external constant lv_i18n_language_pack

If someone needs lv_i18n_language_pack data (such us "locale_name") yo can add some functions to generated c-api, such as int lv_i18n_get_locales_num(void) // gets total count of locales in activated current_lang_pack const char * lv_i18n_get_locale_name(int locale_idx) // gets locale name from current_lang_pack by it id

puzrin commented 3 years ago

I don't understand added value of this request, please, clarify. Also, please, format you post with markdown, it's not readable now.

glory-man commented 3 years ago

Generated files lv_i18n.c/h implemented as an isolated module. To initilize module I need to call _lv_i18ninit() with an argument value equal to _lv_i18n_languagepack which is defined inside module. So _lv_i18ninit() could use _lv_i18n_languagepack as default language pack if argument is NULL, or initilize with some external language pack. To get options of language pack, such as number of languages and locale names in it we can use appropriate api-function. UPD: Besides, I think it will be useful to have such functions (get_number_of_languages and get_locale_name) at all.

puzrin commented 3 years ago

You describe what can be done, but don't explain WHY this should be done. IMO requested change will improve almost nothing, and i tend to reject "coding just for fun".

glory-man commented 3 years ago

Maybe I came from the wrong side. :) My first suggestion was to add into c-api count_locales and get_locale_name functions, which can be used in different application places to receive options of current_lang_pack. This info I can receive by scanning _lv_i18n_languagepack, but _current_langpack does not necessarily equal _lv_i18n_languagepack.

kisvegabor commented 3 years ago

Generated files lv_i18n.c/h implemented as an isolated module. To initilize module I need to call lv_i18n_init() with an argument value equal to lv_i18n_language_pack which is defined inside module. So lv_i18n_init() could use lv_i18n_language_pack as default language pack if argument is NULL, or initilize with some external language pack.

I do see value in it. Normally you want to use the language pack from lv_i18n.c so there is no reason to make the user repeat it.

puzrin commented 3 years ago

I do see value in it. Normally you want to use the language pack from lv_i18n.c so there is no reason to make the user repeat it.

IMO the size of this value is about zero. "the repeat" is done once per project, and not difficult.

Also, code can be generated/composed somehow different. No need to limit API for one way only, if benefit is not significant.

Maybe I came from the wrong side. :) My first suggestion was to add into c-api count_locales and get_locale_name functions, which can be used in different application places to receive options of current_lang_pack.

I'm open to suggestions, but my first question is the same - need use cases to show notable added value for all (or for "many", at least).

kisvegabor commented 3 years ago

Also, code can be generated/composed somehow different. No need to limit API for one way only, if benefit is not significant.

It's a backward compatible change, not a limitation.

lv_i18n_init(NULL);
//Same as
lv_i18n_init(lv_i18n_language_pack);
puzrin commented 3 years ago

I see. IMO not enough to make changes. But i don't pretend to be project owner. If that not takes my time - do as you wish.

glory-man commented 3 years ago

I'm open to suggestions, but my first question is the same - need use cases to show notable added value for all (or for "many", at least).

If we have multilanguage UI, we can provide user to change language. In this case we need to tell user names of available locales. Get_locales_number and get_locale_name functions made application code more flexible.

puzrin commented 3 years ago

That's certainly valid use case, but i don't understand why special method required. User could create entry my_locale_name and iterate language pack, using lv_i18n_set_locale(lang_pack_entry.locale_name) + _(my_locale_name).

As far as i understand, you propose to have special method call instead of lv_i18n_set_locale(..) + _(..) call. Doesn't look like significant simplificatrion.

glory-man commented 3 years ago

That's certainly valid use case, but i don't understand why special method required. User could create entry my_locale_name and iterate language pack, using lv_i18n_set_locale(lang_pack_entry.locale_name) + _(my_locale_name).

As far as i understand, you propose to have special method call instead of lv_i18n_set_locale(..) + _(..) call. Doesn't look like significant simplificatrion.

Such methods will increase the level of abstraction from the internal implementation of language pack. For example, If I want to recieve parent of obj in lvgl-library obj->parent helps me, but I prefer to call lv_obj_get_parent(obj)

puzrin commented 3 years ago

I follow different principle - if something is doable without api change => no changes needed. That helps to keep api clean.

Also, i don't like to reinvent the wheel. This package tries to follows gettext subset. If you say "we need feature X from gettext" - that's ok. If you don't know gettext and try to reinvent your own feature - that's not ok.

kisvegabor commented 3 years ago

@puzrin lv_i18n.template.c is used as template for the final lv_i18n.c, right? So anything we add there will be in the final C file?

puzrin commented 3 years ago

All except sample data /*SAMPLE_START*/ ... /*SAMPLE_END*/

kisvegabor commented 3 years ago

Thanks!

I've added it here: https://github.com/lvgl/lv_i18n/commit/cbfe106994ff0dd7b7d4362a36faa45c1a1808ea

puzrin commented 3 years ago

CI tests failed

kisvegabor commented 3 years ago

Fixed.

puzrin commented 2 years ago

See my comment https://github.com/lvgl/lv_i18n/commit/593707dedd0afda097e4cc3bc139782f360797af#r73332794, that's why this issue stays open.

Personally, i'd prefer to revert that. But i would agree with other kind of decoupling of data / code. I can understand reasons of request, but added value is questionable, and current implementation is not nice for architecture.

@kisvegabor please, take a look what can be done. We will fix "linebreak bug" soon, and i'd like to close all pending issues at once before release. IMO this issue is still pending.

puzrin commented 1 year ago

https://github.com/lvgl/lv_i18n/commit/310c6f003e4d49106eb6f1adefb752d7c9f4b368

Added lv_i18n_init_default() method