I18n allow reorder of input fields

[kristiank]

I am quite satisfied with how the number_constrain part works now, as it keeps the message parts together the same way as your original code. Maybe you have some comments or remarks on code readability? I should refactor out common functions of this to reduce copying code between blocks, but too much refactoring probably would tie it too tightly with Blockly.

[kristiank]

I now implemented the GPIO block turn_pin_variable and noticed that it behaves differently than the previous mathematics blocks. I would suggest that the localization message for turn_pin_variable consists of two input references %1 and %2. One of them is referencing the pinState (turn on/off) and the second is referencing the pinNumber. Would you agree, or am I missing something here?

[kristiank]

I have gotten us into a mess :-) but I see two ways to proceed:

1. I try redefine the code generation to accomodate the two input field references (risky)
2. Go on with changing the rest of the translation messages from % to %1 style and we can look on adding the two fields later. How do you feel?

[RichoM]

Mmm... I see your point. I think you're right in that we should have two input references %1 and %2. I haven't been very consistent when implementing these blocks, unfortunately, and there are a couple other blocks with the same issue.

I think I would rather try to find a way of using your %1 style for more blocks and worry about these "special" blocks later. I bet we'll understand the problem better by then.

[kristiank]

The last commit fob9ade (made 'is_pin_variable' word orderable) illustrates my issue with having one reference %1 instead of two references %1 and %2. You can see how my current code crams both fields into one in the inputFields array. Could you verify this does not affect current code generation?

It does affect how the block looks like, but I was able to make the block for turn_pin_variable look nice by changing the translation string from "%1 pin" to "pin %1". Later on this problem would be solved, when we implement two references instead of one.

[RichoM]

I can confirm code generation works as expected.

[kristiank]

Great that it works. I now found a next problem. Currently my code appends the text previous (or left of) a field specified in the inputFields array. For regular Blockly input fields this is the "correct" way of doing it, but it doesn't work for the first %1 field of the for block. It doesn't work because the field is specified as

() => this.appendDummyInput()
    .appendField(new Blockly.FieldDropdown(currentVariablesForDropdown), "variableName")

What happens is that (exactly as the code states) that the dropdown is appended first and the text is appended afterwards. This results in the dropdown being visualized before the text. With regular Blockly input fields, text can be appended but still are shown left of the input (not on the right side).

I see two possible fixes to this:

• specify a type as dropdown or input in the inputFields (e.g. {type: 'dropdown', func: () => this.appendDummyInput()}) and have the buildBlocksFromMsg treat them respectively
• change the dummyInput with dropdown to a real input that accepts a variable

I sympathize with both strategies. The first strategy is similar to Blockly's localization strategy. I see the second strategy as fixing a flaw in the for block, but I don't know if changing the dummyInput to real input breaks current code generation. What do you think?

[RichoM]

I've been thinking about this for the last couple of days and I think I have a better solution: we could use dummy inputs as temporary placeholders for fields such as dropdowns and then, after all the inputs have been appended, merge the placeholders forward with actual inputs. I think this would require the least number of changes to the code and should be flexible enough to handle all special cases. However, I haven't got the time to actually write the code yet so I'm not really sure it'll work. I'll try to find some time to test this in the afternoon and get back to you.

[RichoM]

I couldn't finish this last night so I pushed the code just now, see: 647c5b6fb998141c8eae04249b79c3628bd5baf1

I added a new initBlock function using your buildBlocksFromMsg as reference but I added the placeholder logic I was telling you about. The code is not very pretty but we can refactor it later once we make sure it works in all cases. I did a couple of blocks just to test the idea and I think it's working. The "task" block is particularly interesting because we can now use your %1 style to refer to statement inputs and thus completely rearrange the block from the translations. That's very nice!

PS: I don't know how this collaboration is usually handled in github. I tried pushing the code to your branch so that it would be added to the pull request but I didn't have permissions so I pushed it to a local branch. I'm sorry if this messes up something.

[RichoM]

Just a little explanation of the code because it's very ugly: The initBlock function works by passing a placeholder dummy input as argument to the inputField functions. The functions can either use the placeholder or ignore it, the only requirement is that they should return the added/modified input. In the "turn_pin_variable", for example, the first function appends the dropdown field into the placeholder but the second function ignores the placeholder and instead appends its own value input to the block. After invoking the inputField function the initBlock checks the return value to decide if the placeholder was used or not. If it wasn't it's removed from the block, otherwise it's added to the placeholder list. After all the inputs have been collected the initBlock goes over all the block inputs merging the placeholders into the next valid input (if any is available). This seems overly complicated but if you look at how the blocks are being defined using this function I think it solves the problem nicely. What do you think?

[kristiank]

The code works excellent. I will need to study it a bit more to be able to add some comments on how it works. This is great. I will change my earlier blocks to use the new initBlock function and get rid of my buildBlocksFromMsg function. Thanks!

About Git and GitHub, neither do I know how these things go, but I eventually was able to merge your changes to my branch. So, "I'm sorry if this messes up something" too.

[kristiank]

I wanted to mention that the number_modulo block brings forth some funny behaviour. The Estonian translation starts with a %1 reference and the whole block is kept on one line(e.g. inline). But the Spanish and English translations has text in the beginning and gets therefore layed out on two lines. I haven't had any thoughts on this yet and I don't know if there might be any hard cases lurking around with blocks that we want to control the layout of. E.g. don't want to use this.setInputsInline(true) for the whole block, but still don't want to have each input field on separate rows. Maybe you already have more thoughts on this.

[kristiank]

Could you verify code generation works correctly for block move_dcmotor, because I added a reference %3 for the speed variable in the block. Thanks. And I wanted to say that I really enjoy writing code with your initBlock function.

[RichoM]

I don't mind the two lines in the number_modulo block but if it bothers you I don't oppose using this.setInputsInline(true).

Also, the code generation for move_dcmotor works fine. Thank you very much for taking care of this work.

[kristiank]

In the block get_sonar_distance it breaks the block into two rows for Spanish and English, but not for Estonian and I can't figure out why it does so. Textually they contain text in the same places, only Estonian doesn't have any whitespace between %1 and the consecutive text. Could it be the trim function that creates this odd behaviour?

I strike my last -- the Estonian translation includes text after the last field %2, whereas the other translations does not.

[RichoM]

That is interesting. I wasn't expecting that behavior. It might be a bug in initBlock. It seems harmless, though, so for now I wouldn't worry too much about it. But I'll look into it later when I find the time.

[kristiank]

I consider this branch's work done now. The function and procedure definitions split their arguments in separate rows, but I guess that's okay for now (although maybe your initBlock code could mitigate it using an input, I'm unsure whether I understood this yet). And it would be good to verify that I haven't broken any code generation.

I think there's more work to be done, but perhaps in new branches. (1) There's many messages that are not translated currently, among others names of variables, procedures and functions, they are hardcoded in English. (2) For this I suggest to make the translations.js array indexed by a language independent name. This would allow contextualising the messages to different blocks, because simple messages like on and off might need different words in morphologically rich languages (e.g. asking if something is in a state, versus changing a state to some state). (3) I also would like to make most of the blocks defined by a translatable text, as an example func_call_0args doesn't allow to change the ordering of the text and the dropdown menu.

[kristiank]

Should I fetch and merge upstream/master into this branch before you are able to merge it?

[RichoM]

Should I fetch and merge upstream/master into this branch before you are able to merge it?

I think the merge can be done automatically. Let me see.

[RichoM]

I think there's more work to be done, but perhaps in new branches.

I agree.

(1) There's many messages that are not translated currently, among others names of variables, procedures and functions, they are hardcoded in English

I don't think we should translate the user inputs. Or do you mean the default names?

(2) For this I suggest to make the translations.js array indexed by a language independent name. This would allow contextualising the messages to different blocks, because simple messages like on and off might need different words in morphologically rich languages (e.g. asking if something is in a state, versus changing a state to some state).

Yes. That is a problem. However, I don't like having the code filled with language independent keywords. I like having the strings in some default language to give them context and make the code easier to read. I think we can make a compromise: treat the english language as any other but keep the current text indices. That means we would have the english strings twice but I don't think that is a problem. And this could allow us to use a language independent keyword when appropriate (like the simple 'on' and 'off') while keeping the code readable. Do you think that could work?

(3) I also would like to make most of the blocks defined by a translatable text, as an example func_call_0args doesn't allow to change the ordering of the text and the dropdown menu.

Yes. Also, all the function/procedure call blocks should have their argument labels updated with whatever names the user chose.

Anyway, nice work. I'll have to test this thoroughly but so far it looks great. Thanks!