Byte block doesn't behave in the manner I think it should #62
Comments
|
I am VERY confused where we are with this - I'm re-opening but lets wait till all the other stuff is finished and then we can concentrate on this again :) I've re-read all the stuff we talked about back in 2019. I got myself up to speed back then on JS buffer's but I'd need to wind my brain back into advanced JS/Blockly mode to deal with again I only want go go round on this subject one last time :) |
|
@bartbutenaers |
|
I have tried to summarize our discussions from the past, which started to look like the Hundred Years' War ;-) Issue 1 - A byte block should return a single byte (or char)
That makes sense to me, but:
Issue 2 - Allow other types of inputs for buffer_set_index blockAt this moment the blocks in this branch allow this kind of input:
You wanted to allow extra input types on the buffer_set_index block (to have both blocks accepting the same input):
Although I understand your intentions, it feels a bit like cheating to me...
Some remarks:
|
|
Before we get into all the options over filling buffers and stuff like that I'd just want to get agreement on the fundemental issue e.g byte 65 should NOT return the number 65, it should return a single buffer object with a value of 65. Now, we can either correct this or we can drop the byte block altogether. I think it will be much easier to drop the block as it's not actually needed to produce buffer objects by the rest of the blocks. We can just use number and text blocks. I think we should try this approach and see where it leads. If it proves to be wrong, then roll it back and proceed down the path of making it return a single buffer object, length 1 So in direct response to:
|
|
Ok, suppose we remove the Byte-block from the toolbar. What happens then with the set-byte-at-index block? It will need to accept other values and get another shadow block. And the we arrive again at the question if we need to truncate the Number/String? Or am I mistaken? |
|
"Ok, suppose we remove the Byte-block from the toolbar." OK :) "What happens then with the set-byte-at-index block? It will need to accept other values and get another shadow block." "And the we arrive again at the question if we need to truncate the Number/String" The existing blocks can convert a long text string to a buffer. I would add an explicit block to concatenate two buffers. I think we also need a length of buffer block |
|
Ok, you win. Had too much discussions already lately :-( |
|
Hi guys, I wanted to join the discussion but found that this is a lengthy discussion.... Many historical considerations are there... I think I can only provide my two pennies here ( please excuse me if there is any misunderstanding): I think we can modify the byte block to return a single buffer object. Then, we modify the set-byte-at-index block to convert the input (now, it is a buffer object, length 1) back to value (number, 0-255). So, the existing users won't be affected while the byte block can return a single buffer object.... I hope this is feasible |
Doesn't that exist already?
I have added a new block:
Here is an example flow to concatenate buffer1 (containing "Hello ") and buffer2 (containing " world"):
Which returns this (which is best readable via the "raw" button):
|
|
"Doesn't that exist already?" Concat works for me as well :) |
|
I think we are done developing for 2.0.0-beta1 if we have fixed the byte block, to return a single byte buffer. |
|
Hi guys (@cymplecy, @jsccjj), It is not really 100% working, but the daily job is a bit of a hobby killer for me at the moment ... Here is my test flow: Thanks a lot !!! |
|
Not seeing any behaviour change on my system
|
|
I went and checked to see if you'd committed it and I see you've removed release-1.1.0 and merged everthing into master :) I didn't read the error msg when I updated :) I'll test using master
|
|
Working now :)
Will do some more testing |
|
How to you want to proceed with this. I've got ideas on how to improve things a bit. 1 Just post them here one at a time |
|
Hi Simon, Thanks for analyzing the buffer related blocks! I assume you are only changing the block definition and code generator files. Just do your changes in both files, and describe here what and why you have done it. Then we can try it and discuss it with you... |
|
1st issue to discuss is that the get byte from buffer doesn't return a byte (buffer length 1)
but I'd like to suggest that instead of returning a buffer length 1 - the text should be changed to get value of index ... of buffer ... as returning a single byte value isn't going to be used much or maybe it could be modified with a dropdown to return a value, single char text string or a byte??? |
|
Quick feedback, because I had to work today from 7:45 to 21:30 ...
Damn that is a pity. I had thought we only had to change code. Now I have to ask the translator guys again to translate again... But indeed it makes sense what you suggest.
Personally I don't really like that, because it will simply return the data that is in the buffer at the specified index... |
|
@cymplecy, |
|
No - leave it with me - had some family stuff to deal with over weekend but
will be working on it today :)
…On Sun, 25 Jul 2021 at 23:01, bartbutenaers ***@***.***> wrote:
@cymplecy <https://github.com/cymplecy>,
Do you like me to implement this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAR7RNBSKTQ7C5G4AEFX57TTZSCSNANCNFSM45S237FQ>
.
|
|
So to do the change just alter |
|
I've been playing with trying to get it to handle the differences between numbers,strings, buffer and variable types and come up with this Basic philosphy is that if it fails to find the datatype, then it's a variable and it returns the code to deal with that If it's a number - it makes sure its in the range 0 - 255 If its a string - it takes the value of the 1st char of the string if it's a byte - then it uses the value of the byte It seems to handle all 4 scenarios in my testing It will probably fail if the contents of the variable are not numeric but maybe some more JS could make it fail safely |
|
My testing flow |
|
Modified version as it doesn't need the try/catch test flow |
|
@cymplecy, |
|
I think we are good to go and put it out there for others to test. :) |
|
It's just failed for me in a loop test - so wait please :( |
|
Okay - I found two issues with this test code (Note paylod is string Hello)
The simple one is that the index cannot be auto-decremented at beginning of code as it might not be a number but may be a variable so the Block/JS 1/0 index correction needs to take place just before the code is returned. This effects the get value at index block as well The second one complicates things further. My solution is to create a tempoary variable called tempVariable and add the code to evaluate it at runtime and then use the result of that evaluation. It all seems to work but the code is quite a mess now :( |
|
Slight issue - a modified looping fill buffer script is converting the space between Hello and World to a 0 instead of 32 |
|
Hi @cymplecy, Can you please share your flow, so that I can quickly test it. And can you please explain a bit more in detail why the is not correct for an expression, while: would work better? Enlighten me please ;-) |
|
|
"And can you please explain a bit more in detail why the tempValue had to be introduced." In the test flow above, the byte at index is set to
value ends up being a string So if we just pass it thru the old code - it sees it as a string (and not a variable) so the old code just ended up delivering 40 (the ASCII code for at ( So I think it needs to be evaluated at runtime (I may have gone too far down the rabbit hole and it might be much simpler to do) So I came up with the idea of assigning the value of value (pardon using that expression) to a temp variable at runtime - decided on to avoid namespace clashes with real variable names that users might use . I'm pretty certain that the whole code can be simplfied but that can be done later on down the line - it just needs to work to get the beta out of the door |
|
I've fixed my " " issue getting treated as a number zero instead of ASCII 32
|
|
There is nasty thing in Blockly when you generate variable names, like e.g. the ``tempValue` variable. Because as soon as you add more than one instance of such a block, you end up with duplicate variable names... For example when I duplicate the buffer_set_index block:
Then code will be generated that contains an error:
P.S. Now you see why I wanted to have syntax error highlighting in this version of the blockly node ;-) I think there are 3 solutions:
Personally I would generate a function (which contains your code), but I'm not sure anymore how we have to generate such a function. Because if we have N instances of such a block, we only want to generate the function code only once... I will search how to do that... |
|
I like 1 - I don't think this code is very readable at the moment so having a variable like isn't going to make things much worse :) I vote for 1 just to get the beta out of the door :) This whole code can be tidied up later :) |
|
With "later" you mean in 3 years from now ;-) My time is up for today. If I don't get a simple to implement example, we will continue with one of the other options... |
|
Doing a bit of googling, if I switched to using var tempValue instead of let wouldn't that solve the problem?
Current code |
|
Just consolidated the code and tidied it up |
|
Ah, I didn't know that about "let". Good catch!!! The code generator code looks very compact and nice this way! I will try tonight to:
Remark: I still would like to generate a function for the "buffer_set_index" block: because if we have N of such blocks in a workspace, then we generate N times the same duplicate code snippet (resulting heavy readable code ...). Would be nice if we could generate the code snippet once in a function, and call that function N times. |
|
@cymplecy, @jsccjj, |
|
All good - minor suggestion just to clarify |
|
Suggestion is implemented and beta published on npm. Will put it on Discourse later on. Now I'm off ... |
@cymplecy, When you have now e.g. multiple buffer_set_byte blocks, with a variable as input:
Then a single function will be generated (containing your code), which is called by all of the buffer_set_byte blocks:
So no code duplication anymore. I would appreciate it if you could do your tests one more time. |
|
Initial tests seem to indicate an off by one error somewhere - just investigating further |
|
Simon, |
|
I don't want to waste your time if its my testing - give me a few more minutes |
|
No hurry!! Just please don't start swearing here :-D |
|
It's all OK - after restarting and clearing cache and spinning around 10 times your nice tidy code behaves the same as my old rough code :) We have a go to launch :) |
|
Thanks!!!!! |
|
@cymplecy : I have created release notes for beta 2... |
|
Looks good |
|
I'm officially closing the issue :-) |
Since we are restarting :)
Back in the mists of time, I raised the point that I didn't think the way byte blocks are interpreted was correct.
We had lots of "discussion" and I never manged to convince you - maybe I did but who can remember after so long :) but this is BIG issue for me so I'm re-raising it again :)
In summary, I believe a byte block should return a single byte (or char) e.g byte(10) should give a single buffer with a value of 10 (e.g. a newline char) not the decimal number 10 as it does at the moment
This should not produce 10
it should give
The text was updated successfully, but these errors were encountered: