Fixed Tool.updateEnabledState behavior and related changes

[bjorn]

See #3756

[TehCupcakes]

This looks great! 😃 Thank you for implementing this!

The type update made me realize I was setting statusInfo while registering the tool, which might not be valid. I updated it to updateStatusInfo() which seemed to work. 🙂 But I noticed a couple other things:

• map is defined as possibly null in the Tool interface. When would this be null? I thought the toolbar only shows when a map is active. If it is always defined, removing the null saves me an if condition.
• The ergonomics of updateStatusInfo() and updateEnabledState() could be a bit awkward. Initially I thought I should return a boolean, but when that did nothing I realized they are void functions and I have to set the internal property directly. Thoughts?

[bjorn]

• map is defined as possibly null in the Tool interface. When would this be null? I thought the toolbar only shows when a map is active. If it is always defined, removing the null saves me an if condition.

I changed it because I realized it would sometimes be null. If there is no map open, then it will always be null here:

var tool = tiled.registerTool(name, toolDefinition);
tool.map; // null

And when closing the last map, it will also be null during the call to mapChanged (I realize now this function's parameters also need | null). The definitions are currently missing | null in a bunch of places, because I originally assumed that like in Java and C#, any object reference is allowed by be null.

That said, in most cases, there will be a map, and most functions will not need to check this variable because they can't be called while the tool.map is null. I'm not sure if there's any way to communicate this, or whether this is reason to just leave out the | null even if that's not entirely correct?

• The ergonomics of updateStatusInfo() and updateEnabledState() could be a bit awkward. Initially I thought I should return a boolean, but when that did nothing I realized they are void functions and I have to set the internal property directly. Thoughts?

These functions were already present in the AbstractTool and AbstractTileTool classes in C++, where they were mostly introduced for some convenience and code sharing between tools. It might be easier to have functions that return updated values, but I'm not sure what they should be called then, because they can't be called the same way as the property getters. I personally would prefer to remove these calls entirely, possibly replacing them with more specific calls similar to mapChanged, like selectedLayersChanged, so that it is more clear in response to what we are updating those properties, but I think it needs a small refactoring and not just a change in the scripting API.

[TehCupcakes]

Ah, I see. That all makes sense. The most accurate type is the best; even if it's rarely null, it should indicate that it could be to prevent runtime errors. TS forces us to check if it is null even in situations we know it won't be, but the extra safety is worth it. 🙂

The only alternative I could think of is that you could have types that extend from Tool and use those in the functions instead, which would set certain properties as non-null if we know from context they won't be null then. But it's probably not worth introducing extra types for that.

[bjorn]

The only alternative I could think of is that you could have types that extend from Tool and use those in the functions instead, which would set certain properties as non-null if we know from context they won't be null then. But it's probably not worth introducing extra types for that.

Hmm, right, I guess we could have something like:

interface ActiveTool extends Tool {
  /**
   * Currently active tile map.
   */
  readonly map: TileMap;
}

And then use this: ActiceTool for the callbacks that can only happen for active tools. But yeah, I'm not sure if it's worth the clutter, especially since it also affects the generated documentation.