Hotspot editor won't allow deletion of hotspot with "&" in the label?

hulsmacmbsc commented 3 years ago

I may have stumbled upon (or created!) a glitch in the pano tour editor. I'll explain the steps: I have been building a tour of the Library over the last day or two. On the second pano in (Butler Library 2021 Tour West Lobby Hold Shelf) I created a hot spot for the stacks area that says verbatim: Stacks (Currently Closed, Place a Hold & We Will Get it For You! Upon noticing that I had forgotten to close the parenthesis, I went to delete the hotspot to fix it, browser exits me out of editor with the "changes you made might not be saved"

DiegoPino commented 3 years ago

Thanks @hulsmacmbsc we will investigate and will keep you update on this side. Just to double verify, Can you share the URL of the affected Object?

DiegoPino commented 3 years ago

@hulsmacmbsc. Ok, I got it. Its not the description (which really should be JSON encoded better for safety) but its a different issue...

Look at this (commons Scene)

For this 3 hotspots, all have the same IDs.. panorama_tour_187_6 This Ids are autogenerated and should be unique. The fact that they are not unique is a BUG and since JS uses those to target which Button triggers which deletion... and in HTML ids need to be unique, Ajax does not trigger.

I will check on the code now to understand why id's are recycled and not kept unique and will come back and fix. Thanks for reporting this. Super strange

Ping @alliomeria too since you have used this more than i

DiegoPino commented 3 years ago

@hulsmacmbsc ok, found the bug.

This line: https://github.com/esmero/webform_strawberryfield/blob/main/src/Element/WebformPanoramaTour.php#L833

The Hotspots ids are added based on how many Hotspots you have. You added seven to the scene with ID 187, the next time you add one it will be id panorama_tour_187_8

That silly (and my fault) logic works until! you decide to delete a few. If you delete, the previous id's (not deleted ones) will stick. So let's say you delete the first 4. You will still keep panorama_tour_187_5, panorama_tour_187_7, panorama_tour_187_8. Next time you add a hotspot, the next ID will be now panorama_tour_187_4 because it is counting the numbers, not actually using the HIGHEST index. Eventually by adding you will hit one that already exists and BUM. Conflict.

Good news I know where the error is!
bad news.

I need to edit your panorama RAW first so you have no duplicated Ids (can do that) and then I need to patch that so the Ids are based on the highest existing one +1 so they never repeat.

DiegoPino commented 3 years ago

And fixed. There is an open pull request, maybe @giancarlobi, @alliomeria or @pcambra want to give it a sanity check/double eyes check to the changed lines. It is not a big thing really but I'm old and cranky today.

@hulsmacmbsc also: I manually patched https://esie.space with this and tested adding/removing/adding/removing/adding and guess what? removing. Since you are a few months behind on code I could not move you all to RC1 or RC2 yet, But the patch is the same as this one and It worked (here..). Could you please try with a fresh Panorama tour (just a demo thing with 2 scenes and a lot of hotspots of every type) and do the same adding/removing X N times + a few extra times and then saving and then looking/interacting with the final product? Hope all goes well there.

@alliomeria also went and fixed via the RAW JSON editor your current Panorama so that one is good to go (Gracias Allison!!). @hulsmacmbsc: you can always use the raw editor (as last resort/quick hack) to fix a label or improve some wording

giancarlobi commented 3 years ago

@DiegoPino I think code is good at a visual check. Have a nice day, friend.

DiegoPino commented 3 years ago

Thanks @giancarlobi! Tested a bit and all seems to be working. Will merge

DiegoPino commented 3 years ago

Resolved via https://github.com/esmero/webform_strawberryfield/commit/eaebb03e5b8db8f9c7ca455b99a348f5716db3d3

esmero / webform_strawberryfield

Hotspot editor won't allow deletion of hotspot with "&" in the label? #95