rikukissa / node-red-contrib-image-output

🏞 Easy way of previewing and examining images in your flows
MIT License
13 stars 6 forks source link

Add 'Width' option to node settings #6

Closed rikukissa closed 6 years ago

rikukissa commented 6 years ago

@bartbutenaers

As a first step of making this happen, I'd like to suggest we add the image width input to node settings like you suggested. Would you mind checking if it looks alright to you, so we can get this in asap :)

https://github.com/rikukissa/node-red-contrib-image-output/issues/2

bartbutenaers commented 6 years ago

Hey Riku (@rikukissa),

That is indeed already a great feature!!

Perhaps you can change following 'minor' issues (all in the html file). This is all about the (50) users that have already installed your node, but without a 'width' property in their nodes. For their ImageOutput nodes that are already in their flow, following will go wrong:

All other functionality seems to be working fine !!!

Bart

rikukissa commented 6 years ago

Once again, thanks for your input. Seems like you've thought this through before! Made the changes requested and indeniably it works better

bartbutenaers commented 6 years ago

Evening Riku,

When I have to create a new version of one of my Node-Red contributions, I always execute following steps:

You learn to stand up by falling. I have already been falling a lot :-)

Bart

rikukissa commented 6 years ago

Yes, you're right! Automated testing would be super sweet for developing these nodes, but I have no clear picture how long it would take to develop something like that. Maybe something to consider in the future!

bartbutenaers commented 6 years ago

Inside the core Node-Red code, automatic testing has been setup. A couple of days ago, Mike has isolated the core test code into a separate node. This new test node is now maintained by the core team. But I haven't used it yet...

P.S. I have been looking at your proposal to grab the image to change it's width. But it is not clear to me how it works. Currently when hoovering above the image, Node-Red displays a 'move' mouse cursor (which I have replaced in the pull request by a splithair cursor). And when you hold the left mouse down and start moving around, Node-Red is drawing a rectangle selection area:

image

Don't know how to get rid of that default NR behaviour, and change it by some custom drag-and-dropping stuff (to change the width)...

bartbutenaers commented 6 years ago

Hi @rikukissa,

I have create a new (test) version that allows to grab the image to resize it. However I'm stuck with on my Git knowledge ;-(

I need your 'Width' pull request code, because I need (during dragging) to update the node's with configuration field. But my trunk doesn't contain that code. No clue how to get around that. Do you have any suggestions???

In my pull requests there is now also an error "This branch has conflicts that must be resolved" ...

Thanks !!! Bart

rikukissa commented 6 years ago

Really? That's amazing mate! What I would do in that situation is:

Add my repo as a second remote to your local repository (while being on your own branch):

git remote add riku git@github.com:rikukissa/node-red-contrib-image-output.git

Then rebase or merge this PR's branch into yours:

git pull riku feature/image-resize (This fetches the changes and merges them to your branch) OR git pull riku feature/image-resize --rebase if don't want to create a merge commit. I'd suggest the first approach in your case since it might be easier

So this would then leave you with your own branch, but with all the commits from this one.

Let me know if you need further guidance. Glad you asked!

bartbutenaers commented 6 years ago

Hey @rikukissa,

Have been reading this evening about git, and updated my fork with your current code. However Node-Red still uses the old red-contrib-image-output, instead of the new node-red-contrib-image-output.

So I tried to remove the old red-contrib-image-output, however that fails: image

Seems there is somewhere a link between the old and new npm package: he tries to uninstall the new package, which fails because it contains a .git folder. Do you have any idea how I can workaround this?

I'm afraid that other users might run into the same problem, if they have installed your old red-contrib-image-output node before...

Thanks ! Bart

rikukissa commented 6 years ago

Well that's odd! First of all where does it even get the node-red-contrib-image-output name, or even know about an npm module named like that 😕

I was thinking that the second one might have been my mistake. I didn't have a proper .npmignore file in the project, so I assumed that would've caused the .git directory to slip into the npm package. However, after trying the latest version with npm v5.5.1, I can't seem to be able to reproduce that.

Can you check if theres a .git directory under node_modules/node-red-contrib-image-output or node_modules/red-contrib-image-output

bartbutenaers commented 6 years ago

Both node modules had a .github folder. For red-contrib-image-output it only contained a preview.png, and for node-red-contrib-image-output it contains a whole bunch of files and directories.

I have removed the .github folder for red-contrib-image-output, but I have still the same issue. NPM does somewhere link the old module to the new renamed one ...

rikukissa commented 6 years ago

Alright, well as long as they do not have a .git directories, it should be fine. What happens if you do rm -rf node_modules && npm install?

bartbutenaers commented 6 years ago

Damn, now only following 5 Node-Red nodes have been installed again:

image

And about 40 other contributions haven't been installed ;-(
Will have a look tomorrow evening what has gone wrong ...

rikukissa commented 6 years ago

Do you haw the other libraries in your project's package.json?

bartbutenaers commented 6 years ago

Hi Riku, seems that only those 5 contributions were part of the package.json file (for some reason ...).
Anyway I fixed it, and now all my pull requests are up-to-date (so they don't contain conflicts anymore). And I got rid of the red-contrib-image-output node. Pfff ... At least now I can continue my developments.