aroberge / reeborg

Enhanced Karel-the-robot clone
http://reeborg.ca/reeborg.html
Other
47 stars 36 forks source link

"onLoad" issue when switching human language #436

Closed onelife closed 6 years ago

onelife commented 6 years ago

Hi @aroberge,

I'm trying to add Chinese language support and encountered an issue.

When switching human language, e.g. cn -> en, there is mismatch within RUR.state properties as shown below.

animated_robots:false
code_evaluated:false
creating_menu:true
current_menu:"worlds/menus/reeborg_intro_cn.json"
...
human_language:"en"
input_method:"python"
onload_programming_language:"javascript"
...
world_name:"worlds/menus/reeborg_intro_cn.json"
world_url:"worlds/menus/reeborg_intro_cn.json"
...

The "human_language" is "en". However "current_menu", "world_name" and "world_url" are still "cn" version.

So when doing initialization, the code will try to load the "cn" version of menu with "en" version of JS library (reeborg_en.js) and failed at line 222 below (reeborg_en.js doesn't recognize "cn" version of MakeCustomMenu()).

https://github.com/aroberge/reeborg/blob/4d18c3ba83afc112aae9d35f889b0e8609e40c65/src/js/index.js#L207-L229

I think "current_menu", "world_name" and "world_url" should be invalid or set to proper values when switching human language.

onelife commented 6 years ago

https://github.com/aroberge/reeborg/blob/4d18c3ba83afc112aae9d35f889b0e8609e40c65/worlds/menus/select_collection_en.json#L62-L66 Between line 62 and 63, there should add "RUR.world_selector.empty_menu();", to clear the previously loaded menu (in other language).

aroberge commented 6 years ago

@onelife I think the issue is elsewhere. I suspect that what is missing is a proper choice in https://github.com/aroberge/reeborg/blob/4d18c3ba83afc112aae9d35f889b0e8609e40c65/src/js/ui/custom_world_select.js#L43-L65 Currently, on English and French are supported for world menus.

Also, various menu items should be added in https://github.com/aroberge/reeborg/blob/4d18c3ba83afc112aae9d35f889b0e8609e40c65/src/js/ui/human_language.js if they are not already.

If you give me a copy of your translated files, I can try to add them to my version to confirm that this is all that is required to make it work.

aroberge commented 6 years ago

I should add some more explanation. The world file select_collection_en.json is intended to show only worlds which can be programmed using reeborg_en.js or reeborg_en.py -- that is, the English version. The onload code is meant to be used with the English version. If you want to add a collection of worlds that can be programmed in Chinese, it must be selectable from a world like select_collection_cn.json

I apologize if there is no documentation that explains all of this; I really should write all the steps required to add a new language as it has changed significantly.

onelife commented 6 years ago

Please check my new branch Chinese.

aroberge commented 6 years ago

It's too late today for me. I will look at it tomorrow morning - in 8 hours or so

On Thu, Jun 28, 2018, 11:07 PM onelife, notifications@github.com wrote:

Please check my new branch Chinese https://github.com/onelife/reeborg/tree/Chinese.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aroberge/reeborg/issues/436#issuecomment-401225858, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmbwpBMr_vydI82ks2C28DnvqEr-gylks5uBYvZgaJpZM4U6sFv .

onelife commented 6 years ago

Of course you should. Have a good rest!

The reported issue can be observed before 8108a7b4e348aff4143cb44a30b50542269c9062.

aroberge commented 6 years ago

Very nice!

When you are ready, could you merge your changes in the main branch and do a pull request? Or, if you prefer, I can do a local comparison between your version and mine, and integrate your changes in my version.

For blockly: I use a very old version. I should update all my blockly code to make sure it works with the latest version. I am guessing that the blockly language file is either zh-hant.js or zh-hans.js from https://github.com/google/blockly/tree/master/msg/js If so, when you will include it, feel free to rename it cn.js for consistency.

onelife commented 6 years ago

I raised a pull request for the code currently I have. Some portions are not translated yet will be included in the next pull request. And if you think 8108a7b4e348aff4143cb44a30b50542269c9062 is valid, then "select_collection_fr.json" should apply this fix also.

aroberge commented 6 years ago

I had not noticed any problem without ...empty_menu() but, given the purpose of select_collection_xx.json, it makes a lot of sense to include it. I suspect that it may prevent some problems if the "World info" window is dismissed without choosing a collection.

aroberge commented 6 years ago

@onelife I have updated Blockly. The new version can be tested at https://aroberge.github.io/reeborg-staging/ (This site is from a special repository which is a copy of the main repository, updated when I want to do some live tests served from the Internet https://github.com/aroberge/reeborg-staging)

Feel free to close this issue.