bartbutenaers / node-red-contrib-blockly

A Node Red node for visual programming a function using Blockly
Apache License 2.0
89 stars 22 forks source link

Adding new features to version 1.1.0 #58

Closed jsccjj closed 3 years ago

jsccjj commented 3 years ago

Hi Bart,

I spent some free time looking into your wonderful package. I modified some codes and added new features based on version 1.1.0.

Major modifications (in my opinion):

  1. Upgraded Blockly to version "5.20210325.1"
  2. Added an Ace XML editor. Now load-from/save-to library is enabled. Saved files are XML that can properly re-create Blockly workspace
  3. Created an expand button for the Blockly workspace using Red.trays api

Minor modifications:

  1. When loading XML toolbox files, parse the categories in a forward fashion instead of backward so that nested categories can be parsed properly
  2. used Blockly.Xml.domToPrettyText instead of Blockly.Xml.domToText

    I hope this may add value to you package.

Thanks, Jeff

bartbutenaers commented 3 years ago

Morning Jeff,

A fullscreen-mode was for me the most important requirement, but I was completely stuck with. Even with the help from the Blockly team (see issue 1 and issue 2). As a result the devolpment of this node was completely halted, and I had to disappoint my partner Simon (@cymplecy) several times. I think he already gave up all hope for a new major version of this node...

But seems you have managed to implement this feature. Awesome!!! Thank you so much!! Moreover this is the first large pull request that I have ever received on any of my nodes...

I am not developing at the moment, since I'm recovering from an operation. But if everything goes well, I hope to start working on a new release of this node in around 6 weeks from now.

I have not reviewed your code in detail, but could you:

I will try to review your changes in detail as soon as possible. Will get back to you with my feedback...

Have a nice sunday, Bart

jsccjj commented 3 years ago

Hi Bart,

Thank you for your recognition! I am glad that I could contribute. Also, my best wishes to you for your recovery from the operation.

Please see my responses below:

- Add yourself to the contributor section. I think you have earned that title ...

Jeff: My pleasure!

- Seems you have added some extra blocks. Can you explain those, so Simon can give his 'functional' feedback.

Jeff: I removed most of the added blocks since they are only useful for my building automation projects. I only kept "Javascript Expression" block
image This is a, I think, a complementary block that is useful while users want to use methods such as isNaN(), isFinite(), and etc with Blockly core blocks.

- You expand only the Blockly editor tabsheet. I think it is usefull that the full-screen mode contains the 3 tabsheets. Suppose I want to learn programming Javascript. I start with this node, and every time that I have dragged a block in the editor, I want to switch to the "Javascript" tabsheet over and over again. But this now means I have to leave and enter your fullscreen mode every time, which is not really user friendly. Is it possible to show the tabsheet control with 3 tabsheets in full-screen??

Jeff: I created the expand view by using Red.tray() directly. Creating tabs in the expanded view reduce the editor area and make the code much longer since we have to redefine tabs as well as html part of it. So, I just created an alternative by using buttons:
image
By clicking "See Generated Javascript" button, users can get to the readonly Javascript editor:
image
Users can get back to the Blockly workspace by clicking "Back to Blockly Editor".

I have committed this change to my fork. I will create another pull request if you think it's appropriate.

- You have upgraded the Blockly version. Do you have seen any other new features that we should support?

Jeff: I have not investigated into it yet. Is there any features you are expecting?

bartbutenaers commented 3 years ago

So, I just created an alternative by using buttons

That looks very user-friendly to me. Nice development work!!!

I will create another pull request if you think it's appropriate.

No please do it all in this PR. That makes it easier for me to review it afterwards.

Is there any features you are expecting?

I was just wondering if you had upgraded a new version of Blockly because you required new features. But now I assume you just wanted to get my node in sync with Blockly.

Last year I had already implemented some changes (in a local version on my pc):

I will commit those changes to Github when I'm back into business...

cymplecy commented 3 years ago

Hi Jeff 1st thoughts image

Is different order - could it be changed to the original order image

Also, the resizing buttons could do with being fully visible without need to scroll image

cymplecy commented 3 years ago

On a separate issue the extra JavaScript function block comes up from time to time but it's never made it into the project as it's not really needed.

If you think that you have a use case that requires it - let me know what it is and I'll show you that it isn't needed :)

Simon

cymplecy commented 3 years ago

And your JS blocks in your example aren't needed :)

image

jsccjj commented 3 years ago

So, I just created an alternative by using buttons

That looks very user-friendly to me. Nice development work!!!

Thank you!

I will create another pull request if you think it's appropriate.

No please do it all in this PR. That makes it easier for me to review it afterwards.

Got it. I just double checked that the pull request is reflecting my latest commits.

Is there any features you are expecting?

I was just wondering if you had upgraded a new version of Blockly because you required new features. But now I assume you just wanted to get my node in sync with Blockly.

Last year I had already implemented some changes (in a local version on my pc):

  • Extra nodes since the API of the Function-node sandbox has been extended with new features.
  • Removed the Blockly libs from this repo, and implemented a Blockly NPM dependency (so new minor Blockly versions are automatically installed).

I will commit those changes to Github when I'm back into business...

I look forward to it! Let us know what else we can contribute.

jsccjj commented 3 years ago

Hi Jeff 1st thoughts image

Is different order - could it be changed to the original order image

Hi Simon, I changed the order per your comment but left the "Misc" at the bottom for the reason that the toolbox of "Misc" contains a "Logic" type of block. When joining the toolboxes this may lead all "Logic" blocks into "Misc" category. Of course this can be changed again if you think it's more proper. New look is here: image

Also, the resizing buttons could do with being fully visible without need to scroll image

This part is fixed by changing the CSS style. The min-height setting of the expanded blockly view was set to 700px. Now, it is 350px. This should fix the problem. Please let me know whether this works at your end.

jsccjj commented 3 years ago

On a separate issue the extra JavaScript function block comes up from time to time but it's never made it into the project as it's not really needed.

If you think that you have a use case that requires it - let me know what it is and I'll show you that it isn't needed :)

Simon

The "Javascript Statement" block is powerful already.

The situation I have is like:

Considering to generate the following code by blockly

if ((msg['topic']) == 'disabled_op') { disabled_op = Number(msg.payload); if (isNaN(disabled_op)) { disabled_op = 0; } }

We can simply use the Statement block like this: image and get this: image

Below would get the same thing image By using "Javascript Expression", I would need additional blocks to achieve the same goal. However, I view this is the purpose of Blockly that is to visually learn coding. If I had know how to write all the JS statements, I would just use the default function node.

Also, in the situation below, the expression block may be useful: image image

My two pennies here...

Thanks, Jeff

cymplecy commented 3 years ago

My view is that the use of Blockly in Node-RED isn't to teach/learn JavaScript.

It is an alternative to using text-based JS and JSONata, so as to continue the low-code philosophy of Node-RED itself.

The JavaScript block is intended to be a block of last resort.

So if we need to return a value using it, then this sort of thing does the job image

When someone finds a need for the JavaScript block, then this usually generates a discussion on whether what they are seeking to achieve should be provided by a built-in Blockly function or whether it's too rare a case to do so.

cymplecy commented 3 years ago

"Please let me know whether this works at your end." The layout changes look/work fine for me :)

jsccjj commented 3 years ago

Hi Simon, I understand. I will remove the node.

bartbutenaers commented 3 years ago

Hi Jeff,

Simon is completely right that the purpose of this node is an alternative for the function node, for those that don't feel confident with Javascript. But on the other hand we understand your use case.

So we had a very short discussion about it, and we agreed that you can keep your Expression-node. Consider it as a thank-you gift from our side, for all the time you have spend to help us improving this node ...

Hopefully you still have the source code somewhere ;-)

jsccjj commented 3 years ago

So we had a very short discussion about it, and we agreed that you can keep your Expression-node. Consider it as a thank-you gift from our side, for all the time you have spend to help us improving this node ...

Thank you Bart and Simon. I appreciate you guys' approval! I will add the code back :)

BTW, have you guys checked the "Multiline text input fields" which seems added to blockly last Nov. https://developers.google.com/blockly/guides/create-custom-blocks/fields/built-in-fields/multiline-text-input

image

It seems it can be used on Comment and Statement blocks but may be incompatible to current blocks...

bartbutenaers commented 3 years ago

@cymplecy, Is it ok for you if @jsccjj adds the multiline text input block in our toolbox?
I don't have any objections...

cymplecy commented 3 years ago

Fine by me :)

jsccjj commented 3 years ago

Hi guys,

I added the new block. Please give it a try.

the default: image the generated JS: image

In action: image The generated JS: image

Please let me know how you want to improve the block once you have tried it.

Thanks, Jeff

cymplecy commented 3 years ago

Hi Jeff I'm having an issue when using normal editor (not expanded one) If make edits in Editor mode and then switch to Generated JavaScript mode and switch back to Editor mode - the edits are lost. Simon

jsccjj commented 3 years ago

Hi Simon, That was a bug. I got it fixed 2 days ago. Please clone my fork (https://github.com/jsccjj/node-red-contrib-blockly). And let me know if it works for you.
Sorry for the bug.

Thanks, Jeff

cymplecy commented 3 years ago

I thought I was on the latest but I hadn't cleared my cache :)

All OK now :)

bartbutenaers commented 3 years ago

Jeff,

I have a series of new features for this Blockly node in an old local version of mine at home. Would like to have those features also in the 1.1.0 release, so we can remove both old branches and my local changes. Would like to have a clean Github repository again, with only a master branch. That allows us to create smaller releases in the future, to avoid that we end up again with years of inactivity ...

Some of my old changes might not work anymore after your pull-request, so I will have to review those. Which might take some time.

I will create a series of issues in this repository, so you can do a follow-up and get an idea of the progress...

Thanks!! Bart