-
-
Notifications
You must be signed in to change notification settings - Fork 350
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
Chore: cleanup translations #3082
Conversation
@HThuren: I am honestly not quite sure what we have signed up to try and do :) My understanding was that we should check whether there are unused strings in the firmware translation files, and if so suggest to have them removed. But only after testing that no firmware builds fail, of course. We would first need to find those unused strings, for example by searching for each and all of the TR_* strings in all appropriate files (*.cpp, *.h, except translations.cpp, translations.h and the various language files themselves, e.g. se.h/da.h etc) and if a TR string is not found in any of those files have it noted. I am sure that can be done in many different ways. In other words: how do we find those TR_strings that are not included in any of the radio/src/*.cpp or *.h files? |
Hey guys, I see different ways of cleaning up in the order of easy to a little difficult: 1. missing translations 2. Search for obsolete defines
So if you want to find out if the 3. Look for duplicate definition If you are sure all the definitions have the same meaning this could be made single source by: Or even better do a follow up search for e.g. where |
Thanks @mha1. We will do this bit by bit and it may take a while but we will give it a shot and see how it goes. |
No hurry, no pressure, this PR is marked as Draft - work in progress. |
STR_GASSUIT_AVG_FLOW In untranslated.h; but also in all language files except fi.h |
But we don't always have the connection between TR_defines and STR_defines[] ! |
do you have an example? |
None of |
TR_THROTTLE_START is spelled TH_THROTTLE_START in en.h |
The entries below are NOT in en.h; but are defined in one or more of the other language files as listed: INDENT - fi.h |
TR_GYRO and related are no longer used... replace with TR_IMU |
Lots of work as it seems. The very first input (thanks @philmoz) shows we need a common understanding of how working with language dependent stuff should be done. TR_GASSUIT_AVG_FLOW is part of how and where telemetry sensors are named. Most of the telemetry sensor defines are in untranslated.h starting at line 158. But there are also telemetry sensors defined (and directly used in code) in language specific files. Furthermore there are duplicate definitions for the same sensor name, eg STR_RSSI "RSSI" in language specific files and TR_SENSOR_RSSI "RSSI" untranslated.h. Both are used in code (STR_RSSI and STR_SENSOR_RSSI). It is also worth to note that none of the telemetry sensor definitions in the language specific files are actually translated. My options proposal for a cleanup:
Note: both options require some editorial updates in /src/telemetry/.cpp files. From a flash usage and performance perspective I believe both options should have the same footprint. From a cleanup point of view option 1 is much more work than option 2. The main question here is: do we want to ever translate telemetry sensor names? Any thoughts? |
Well, in danish ex 'Alt' are translated, so I'm afraid it's diffrent when to translate sensors. |
Do you mean |
I think of: |
I am not advocating for/against removal, just trying to analyze potential cleanups. The two definitions you mentioned are not part of what I was talking about. The two are part of the UI code and need to be in the language specific xx.h files. I was talking about the telemetry sensor names e.g. as they appear in the list of discovered telemetry sensors. Most of the names you'll find in this list are defined in untranslated.h. But some others are defined in xx.h. Have a look at the block line 1186 (STR_VFR) to line 1339 (STR_FLOW) in da.h. You'll find an identical block in all other xx.h files. All having the same English definitions. So telemetry sensor names have never been translated so far. @pfeerick would translated sensor names jeopardize the effort to move away from index based telemetry to name based telemetry? |
Sorry, I have to correct myself. da.h does translate at least one telemetry sensor name |
In addition to the TR_VOICE_? entries mentioned earlier, the following are defined in en.h; but do not appear to be used anywhere in the radio code: TR_ACCEL |
It's a mix. Most are really not used Some are used by further defines which are used
Some are used by further defines which are also never used And a few are used to declare constexpr int's in translations.h. (should be done in translations.cpp and externed in translations.h)
|
@HThuren No rule without exception :-). TR_VOICE_ defines are used in tts_xx.cpp. They should really be STR_VOICE_ definitions. I couldn't find any other TR_ in other .cpp/.h files other than the translation related files
|
I made the PR with the naitive languge name, will look into (in another PR) make the STR definition. |
Right, I think this to be translated one day, the ones who releate to other transated terms, ie. STR_ALT, STR_GALT, TR_CURR, TR_ASPD, TR_GSPD, etc |
@HThuren There is actually duplicate definitions for Roll/Pitch/Yaw/throttle. Search for example for STR_ROLL in xx.h and STR_SENSOR_ROLL in translated.h. The ones used in code is STR_SENSOR_ROLL/PITCH/YAW/THROTTLE. It looks despite your translation efforts those sensors are used non-translated. STR_SPEED (the one with the funny translation) has no duplicate and is only used in a single MPM Protocol (Multiplex MLink mlink.cpp). From briefly scanning other xx.h files I'd be inclined to say you might be the only one having translated some sensor names so far. In reality only one is used in code (STR_SPEED). The question is will there be more language wide effort to translate sensor names. But in any case I really don't like the duplicate definitions. They are misleading. |
@HThuren I have compared the telemetry sensor name definition sections of en.h (reference) to all other language files. Here's the differences:
Currently there are about 164 telemetry sensor names defined. Telemetry sensor names are limited to 4 characters and are mostly short names or abbreviations, not words you'd find in a dictionary. This and the fact that up to now only very limited use was made of translations for sensor names makes me believe there is no strong requirement to have 160+ nearly identical definitions in the 15 xx.h files. As a huge fan of the single source principle I propose a change to implement a single source but still allowing for overrides in xx.h. An override in da.h would look like this: I created a branch on my fork for the telemetry sensor names cleanup:
Please have a gracious look at https://github.com/mha1/edgetx/tree/choreCleanupTelemetryNamesDefs and if possible run the language test builds. My local tests and package firmware run was successful for all radios. |
@philmoz @pfeerick Here's my feedback. Most of your input should be safe to remove. It'll be a lot of work to do manually. Is there some wizard out there that can to a shell script to delete entries (lines) based on a black list? I can compile the black list. The entries below are NOT in en.h; but are defined in one or more of the other language files as listed:
I think this should be removed as this is already defined in translations.h #if defined(COLORLCD)
As per @pfeerick should be removed. IMU strings are used now
#if defined(TRANSLATIONS_CN) || defined(TRANSLATIONS_TW)
Should be removed In addition to the TR_VOICE_? entries mentioned earlier, the following are defined in en.h; but do not appear to be used anywhere in the radio code:
They are used through const int declarations in translations.h. The declarations are used in code
They are used to form the list TR_GVAR_HEADERS which is not used. Can all go
They are used to form the list TR_LIMITS_HEADERS which is not used. Can all go
They are used to form the list TR_LSW_HEADERS which is not used. Can all go.
They are used to form the list TR_PHASES_HEADERS which is not used. Can all go.
I checked also, not used in code, should be removed |
Should be easy enough to script (I created a script to generate the initial list). |
I'll try - will call for help eventually |
Wow, seems that the translations really were in need of some TLC from you developers. A lot of old stuff that can be removed. Great work! |
@philmoz No call for help, worked out a script to do the removals @pfeerick While scanning for obsolete definitions I found radio\src\gui\128x64\model_templates.cpp is not used in any radio firmware build. Shouldn't this file be removed from the source tree? I have updated my fork with all discussed and some additional removals of obsolete definitions and translations. "Run tests and package firmware" completed with no errors, EN and DA companion builds are working without problems. Woohoo, and about 280 Bytes of flash memory saved. @ulfhedlund I kindly ask you to run your language builds using https://github.com/mha1/edgetx/tree/choreCleanupTelemetryNamesDefs @HThuren All sensor name definitions are now in one place (untranslated.h) but it is still possible to override sensor names with language specific translations in xx.h. It is pretty straight forward. Undefine the sensor name definition and define a new one. I have done this to da.h, it.h and fr.h as those are the only ones having translated very very few sensor names. Here's the part in da.h to show the concept:
Having all sensor name definitions in one place (untranslated.h) is not stopping translations. If translators want to override sensor names they can do it using the above concept. To show the concept is working I built a Danish language TX16s Companion overriding GSpd with "Fart" using the above concept: |
Great job, I think it will do, and all the cleanup make the xx.h more easy to cover. :-) |
I do the compile all models and x10s all languages with your |
@HThuren Great, thanks! I built all radio models but doing it again won't hurt. |
All compile done - all language for x10, all models EN and DA, good to go :-) |
Possibly. The templates screen was something I found quite useful on the Turnigy 9x/9XR/9XR Pro, but for some reason it doesn't seem to have been used on the other radios, and is something I actually wanted to see re-instated ... but if that code is needed it can be resurrected from the github repo so there is no harm in removing it not if it's not used. One less bit of noise.
Any savings in flash we can claw back are worth it... means the radios like the X9D+ family get a little more wiggle room for features. It seems from a recent breakage on the 2.8 branch there was a hidden overflow causing those radios to randomly brick when bootloader flashed which we've been able to resolve... and there are some more flash savings coming soon... so the future is still looking good for those radios continuing to learn new tricks and be actively supported. Plus, it means I may be able to turn more of the firmware translations back on, as a couple had to be dropped for those radios. |
@pfeerick, thank you for this comment and point of view |
@pfeerick model_templates.cpp's fate is in your hands. I won't touch it here. I mean it doesn't hurt at all, just noticed it is not referenced as part of any build (not in \src\gui\128x64\CMakeLists.txt). |
da6a0e0
to
b046f6e
Compare
I think this served its purpose |
The workshop is open ...