fixed a few crashes when properties have unexpected types

ArnoldSmith86 / virtualtabletop

a virtual surface in the browser on which you can play board, dice and card games

https://virtualtabletop.io

GNU General Public License v3.0

173 stars 31 forks source link

fixed a few crashes when properties have unexpected types #2312

Closed ArnoldSmith86 closed 2 months ago

ArnoldSmith86 commented 2 months ago

While testing #1538, we noticed that using an actual number as an id leads to crashes.

I searched for occurences of widget.get(property).function in the code to see where we are having similar issues. Because widget.get(property) can basically always return something that does not know function.

This PR fixes a couple of obvious places but there are probably still plenty more. It is already hard enough to review and test the scope as it is.

There are still plenty of crashes for widgets with actual numbers as their id. But now I know that the only real solution is to enforce strings for id. I will do that in a separate PR though.

PR-SERVER-BOT: You can play around with it here: https://test.virtualtabletop.io/PR-2312/pr-test (or any other room on that server)

96LawDawg commented 2 months ago

I don't know how to test this.

ArnoldSmith86 commented 2 months ago

[x] Do widgets still snap to grid?
[x] Do counters with decimal places still fix their rounding errors?
[x] Does the CANVAS operation (especially with an empty colorMap array) still work?
[x] Does owner still properly hide widgets from other players?
[x] Does handlePosition of piles still work?
[x] Do cards still get created honoring the faceTemplates?
[x] Does the tree still display cardType or text for widgets with auto-generated id? This probably doesn't even work in production anymore because I don't think anyone adds the class wide to it ever.
[x] Does the editor "Properties" UI for dice still work?
[x] Do the dropTarget buttons still render correctly for the "Properties" UI of holders?

It's more likely that they quickly cause Javascript errors instead of showing subtle behavior changes.

You could, of course, also test wrong types for all of those properties. But it's not really important as long as we don't break the correct things..

96LawDawg commented 2 months ago

Does the CANVAS operation (especially with an empty colorMap array) still work?

It works the same as it does on production, which is to say that it does not display anything because it is white on white. But if you draw and then manually change the color map to just black, it shows up.

Does the tree still display cardType or text for widgets with auto-generated id? This probably doesn't even work in production anymore because I don't think anyone adds the class wide to it ever.

I don't remember ever seeing this. I definitely don't see it on production or on the test server.

ArnoldSmith86 commented 2 months ago

I don't remember ever seeing this. I definitely don't see it on production or on the test server.

Before the new editor layout, you could toggle the sidebar between narrow and wide and the wide mode showed those details.

ArnoldSmith86 commented 2 months ago

Sounds like everything is working then? Apart from #2314 which is unrelated.