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

Multi-language support #15453

Conversation

marcio-ao
Copy link
Contributor

  • Language strings are now in their own namespaces.
  • Added a GET_TEXT() macro for retrieving strings in the current language.
  • It is now possible to have multiple languages compiled into the firmware at one time.
  • TODO: Add a ultralcd menu for selecting among languages.
  • TODO: Save language selection to EEPROM

This was an extremely tedious PR to make and I probably made some mistakes along the way, but I wanted to get this pushed as a PR so get some help finalizing and testing it.

@thinkyhead
Copy link
Member

thinkyhead commented Oct 6, 2019

This could be useful if many strings work this way.

#define SRAM_STRING(VAR,SNAME) \
  PGM_P __PSTR##VAR = GET_TEXT(SNAME); \
  char VAR[strlen_P(__PSTR##VAR) + 1]; \
  strcpy_P(VAR, __PSTR##VAR)

SRAM_STRING(tmp, MSG_INFO_PSU);

It must certainly waste a lot of space to have a whole different string for each extruder, and I am currently working on an improvement to allow for any number of extruders without needing to add a new E entry for everything that deals with multiple tools.

Going forward, let us find a way to get "E0, E1, E2..." strings without having to make several copies of strings with only one character differing. I suggest that we include %i in the translatable strings where the E number is supposed to go.

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 11 times, most recently from 57a70ce to 3de7b21 Compare October 6, 2019 15:12
@thinkyhead
Copy link
Member

Ok, all of Saturday was spent sharing the tedium and getting this patched up. It has some implications for my upcoming PR #15452 so it was good to get familiar with all the changes. This might still not be passing all the tests, but it is a lot closer!

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 2 times, most recently from 577ec64 to 5d49f9c Compare October 7, 2019 03:30
@thinkyhead
Copy link
Member

Rebased……

git fetch origin
git checkout pr-consolidate-strings-for-localization-2
git reset --hard origin/pr-consolidate-strings-for-localization-2

@marcio-ao
Copy link
Contributor Author

Going forward, let us find a way to get "E0, E1, E2..." strings without having to make several copies of strings with only one character differing. I suggest that we include %i in the translatable strings where the E number is supposed to go.

@thinkyhead: Just as an FYI, in my first attempt at doing this PR I made a new type called PGM_P_PAIR which contained two PGM_P elements. This object could contain either a single string, or two strings. I then replaced all implicit concatenations in calls to MENU_ITEM_EDITwith these PGM_P_PAIR and made it so the UltraLCD code did the concatenation while drawing. This worked as expected, but I found that in the simple case of a single extruder printer it increased the compiled binary size by about 2-3k (this was probably due to the compiler having to generate extra code for each MENU_ITEM_EDIT call in order to construct those objects). So I guess the caution here is that any attempt to shrink multi-extruder printers by string de-duplication is likely to cause the single extruder firmware to become larger, which may or may not be an acceptable trade-off.

@thinkyhead
Copy link
Member

I guess the caution here is that any attempt to shrink multi-extruder printers by string de-duplication is likely to cause the single extruder firmware to become larger, which may or may not be an acceptable trade-off.

It may be possible to apply static strings only when it is cheaper to do so. For example, if we find that it's cheaper to use static strings over a sprintf_P technique with fewer than 3 extruders, we can add a switch for that.

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch from ff8cb6c to 8958d1a Compare October 7, 2019 21:35
@thinkyhead
Copy link
Member

Ok, patched up the last (?) few typos. If all is well, this will be merged pretty soon… Then I get to have fun resolving 100 conflicts in 15452!

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 5 times, most recently from 323c2be to 2c5c149 Compare October 7, 2019 23:06
@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 2 times, most recently from c35d436 to f429a48 Compare October 9, 2019 01:19
@thinkyhead
Copy link
Member

I had to make a change to the language codes because el-gr, pt-br, and jp-kana can't be concatenated onto things to make proper symbols. This is rebased on those changes. I believe that will solve the last compilation error here. Since we're not doing any tests of the touch-ui stuff, Travis might still not be catching some things. But I'll merge this tonight regardless so we can continue to plow forward.

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 3 times, most recently from f0c267a to 52663fe Compare October 9, 2019 02:03
@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch 2 times, most recently from c96be8f to 2798f7e Compare October 9, 2019 02:19
marcio-ao and others added 4 commits October 9, 2019 02:40
- Language strings are now in their own namespaces.
- Added a GET_TEXT() macro for retrieving strings in the current language.
- It is now possible to have multiple languages compiled into the firmware.
@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch from 2798f7e to 82788a3 Compare October 9, 2019 09:49
@thinkyhead
Copy link
Member

Ok, I put in another 10 hours on this today, for a total of about 50 hours just on this PR this week! When you said it was a tedious task, you weren't kidding. I have been running tests and patching for the last few hours and it is looking pretty close at this point.

The language strings themselves are the trickiest part. I expect to set up an online database at some point so that we can manage our translations with a proper web interface. It could just be done as a Google spreadsheet, but I think that would be limiting. @shitcreek and I both have had illustrious web careers, so we'll have to flip a coin to see who gets the honor…

@marcio-ao
Copy link
Contributor Author

Ok, I put in another 10 hours on this today, for a total of about 50 hours just on this PR this week! When you said it was a tedious task, you weren't kidding.

Yeah, sounds about right, I've been struggling with this one for a few weeks. I appreciate you helping. When it's all done, I think it will be worthwhile.

I have been running tests and patching for the last few hours and it is looking pretty close at this point.

Yay!

The language strings themselves are the trickiest part.

Yes, there are many of them, which makes changes hard. I did the bulk of the work with a Python script, but it took me most of a week to write that script, so it's hard to know whether I saved any time!

Anyhow, at least I think it would be easy to convert that script into a "lint" program for the language files, something that will identify missing or unused strings, that might make the job easier going forwards.

@thinkyhead
Copy link
Member

I have never used lint, but it sounds like the right tool for the job.

@thinkyhead
Copy link
Member

Since Marlin wants to send static strings to onUserConfirmRequired as a standard practice I've added onUserConfirmRequired_P to the ExtUI API.

@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch from bf2ef7f to 3f2ad53 Compare October 9, 2019 22:31
@thinkyhead thinkyhead force-pushed the pr-consolidate-strings-for-localization-2 branch from 3f2ad53 to 835a512 Compare October 9, 2019 22:46
@@ -293,7 +293,7 @@ FORCE_INLINE void probe_specific_action(const bool deploy) {
host_prompt_do(PROMPT_USER_CONTINUE, PSTR("Stow Probe"), PSTR("Continue"));
#endif
#if ENABLED(EXTENSIBLE_UI)
ExtUI::onUserConfirmRequired(PSTR("Stow Probe"));
ExtUI::onUserConfirmRequired_P(PSTR("Stow Probe"));
Copy link
Member

Choose a reason for hiding this comment

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

@InsanityAutomation@marcio-ao — It sounds like you should confer on whether this is the proper use case for onUserConfirmRequired.

@thinkyhead thinkyhead merged commit 6a865a6 into MarlinFirmware:bugfix-2.0.x Oct 10, 2019
@thinkyhead thinkyhead changed the title Improved localization code. Multi-language support Oct 10, 2019
rolkun pushed a commit to rolkun/Marlin that referenced this pull request Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants