search

MaterialFoundry / LockView

Foundry VTT module: Locks the view for the purpose of using Foundry on a digital playmat, such as a horizontally mounted TV. Scales the scene so the gridsize is always displayed corresponding to a real-world size, and can block zooming and panning
15 stars 7 forks source link

Put null check before any use of canvas in onRenderSceneControls #80

Closed wickermoon closed 2 years ago

wickermoon commented 2 years ago

See #79

CDeenen commented 2 years ago

Moving the canvas == null check forward is a good solution for the errors. However, removing the name field in module.json will probably break v9 compatibility. I expect it'll be fine to have both an id and name field at the same time.

cas206 commented 2 years ago

Noticed same issue. I tracked it down to the deactivate() function call when canvas['lockview'] is null. Should the entire routine be canceled using the proposed solution? Or just the deactivate() function call?

 if (compatibleCore('10.0') && controls.activeControls != 'LockView') 
      canvas['lockview'].deactivate();
wickermoon commented 2 years ago

I updated the module.json. I tested it and it seems like foundry doesn't care whether both exist at the same time. It doesn't even produce a warning, which is nice!

wickermoon commented 2 years ago

Noticed same issue. I tracked it down to the deactivate() function call when canvas['lockview'] is null. Should the entire routine be canceled using the proposed solution? Or just the deactivate() function call?

 if (compatibleCore('10.0') && controls.activeControls != 'LockView') 
      canvas['lockview'].deactivate();

The whole function doesn't make sense, if canvas is null. Also, the null check was in before, just further down the function, after canvas was already used. So if anything, deactivate should be the only thing to be called, which is impossible, because canvas is null. Maybe the combatTrigger could still be set, since it doesn't require the canvas, but I don't think it makes sense to execute anything from that function when canvas is null.

wickermoon commented 2 years ago

Okay, I did some refactoring. Just in case, I tested the function and it still works just as before. If you want, I can revert those refactoring changes, but it makes the whole function easier to read and understand. :)

CDeenen commented 2 years ago

The refactoring is fine :) It all seems to work fine like this, so I've pushed an update. Thanks for the help!