fix(network) selected image: setting image doesn't affect current image

[mehl321]

In our private project, I encountered the case where a selected image was redrawn. In that case, the node switched back to the unselected image. This PR fixes this.

[wimrijnders]

Hi there,

For my understanding: Is this a fix for something that happens during the resetting of the options?
Because then the change makes sense to me. Or are the circumstances different?

[mehl321]

Hi there,
I found that the node image object is sometimes constructed again, calling setOption() and setImages() again. I can't tell you why exactly. Is it normal?
When this happens the node current image is reinitialised to its default unselected image even if it was selected and the unselected image appears when it should be selected.

[wimrijnders]

The issue I have with the changes is that they assume that options can be reset. Which is fine, but they don't take into account that the images themselves can be reset.

To be honest, most of the code does not take the resetting of the options into account. So, the underlying question for me becomes, should we support it?

My opinion is that is should be possible, and I'm willing to restrict the implementation to the images. I have to think about how to handle this myself. Will get back on this.

[wimrijnders]

OK, I think I'm overextending my thoughts here. This PR is obviously an itch you're scratching. I'll pick up the gist of the previous comment in a separate issue/PR.

[wimrijnders]

I'm doing my best to find a test case for this fix, without success. Can you perhaps provide a test example where the selection disappears after calling setOptions()?

I found that the node image object is sometimes constructed again, calling setOption() and setImages() again. I can't tell you why exactly. Is it normal?

I found exactly one case in which the node is remade from scratch: when option nodes.shape is changed in the call to setOptions(). And even in this case, the selection works properly.

I'm currently at a loss here; I don't see the fix as-is solving anything.

Perhaps we're using different vis.js versions? I've been testing with latest from the develop branch, this might give a difference. Will try with latest stable.

Update: using latest stable doesn't make a difference. Select is still working properly.
I urge you to provide a working example of the issue, otherwise I can't continue.

[mehl321]

This issue is happening on our customised private version of Vis. Maybe my use case is not even possible with the normal version? I'll try to put together an example that reproduces this issue. Until then stay put :)

[mehl321]

I managed to set up an example that reproduces this issue:
http://codepen.io/mehl321/pen/PpLjPr

Steps to reproduce:

1. Select and deselect the node only by clicking inside the blue rectangle. The image is toggling as expected.
2. Select the node. It is now showing the "selected" image.
3. Click on the "Update Node" button. Notice that the image is reset to its "unselected" state although it is still selected. This is the bug.
4. Unselect the node by clicking in the blue zone. Notice that from now on the wrong image is shown.

I tried the exact same example on my machine with the fix applied and the bug was not there.

[wimrijnders]

👍 Thanks for taking the trouble to make a demo! Much appreciated.

I can see now how your changes address an issue. However, I with my pedantic frame of mind, see a whole more bunch of related problems here. For example, it's now possible to initialize an image node without any images, which leads to errors on drawing. Methinks it's ripe for further improvement. But that's not the topic of this PR.

[wimrijnders]

Your issue has uncovered many things wrong with the setting of the node images. Rather than having you take the full brunt fixing this, I'll make a separate PR for all stuff outside your code changes. I'll still review the code you submitted.

[mehl321]

Ok then I leave it to you.
Thank you

[wimrijnders]

imageObjAlt needs to be set always, not just if it is defined. It should be possible to remove the alt image in an update.

This also applies to the uses of imageObjAlt below.

[mehl321]

If I remove those checks and create an image node only containing an unselected image or even no image (which is stupid but possible to do) then we get a JS error.
You may want to update the code yourself.

[wimrijnders]

Well, yes, the code needs to be rearranged somewhat.

You may want to update the code yourself.

If you mean that I create another PR to fix this, that's perfectly fine with me.
Shall I close this PR and create a new one?

[mehl321]

Yes please. I don't feel like refactoring the codebase myself. I'm too used to TypeScript now :)

[wimrijnders]

OK fine.

I'm too used to TypeScript now :)

Oh, you lucky bastard. I'm itching to go there. There is no hope for TypeScript in vis.js, but I've been rabidly refactoring my own current project to make it possible.

Thank you sincerely for your effort here. The bug is genuine and it needs to be tackled. All the best!

[mehl321]

There is no hope for TypeScript in vis.js

I understand that it would be an important update but maybe would it bring vis.js (or .ts??) to the next step! An intermediary step could be to switch to strict ES6 using eslint (with no vars etc.)

[wimrijnders]

An intermediary step could be to switch to strict ES6 using eslint

Oh yes, definitely. That is actually an active discussion between the admins here 😄. Let's hope something good will come out of that!

[wimrijnders]

Note: imageObj must always be defined on the call of this method. For the intent of your code changes, you can assume this is so.

[wimrijnders]

Ok then I leave it to you.

I wasn't implying in the review that I would do all the work.

After extensive testing, I did find one issue, see the review notes. I would appreciate it if you can make this fix; however, if you prefer that I do it, that's actually OK with me.

Shall I take it on me?

[mehl321]

I just read your message. I will have a look at it soon.

[wimrijnders]

Closed because superseded by #3053.