Scripting: Tool function updateEnabledState() no longer usable

[TehCupcakes]

Describe the bug
As of 1.10.0, one can no longer use updateEnabledState() to override when a tool should be enabled. This is likely a regression introduced with the changes to accommodate targetLayerType. While this covers the most common method of enabling/disabling a tool, there are still more complex requirements which call for a way to hook into layer changes to determine enabled status.

To Reproduce
Steps to reproduce the behavior:

1. Create an extension which calls tiled.registerTool() and has updateEnabledState() defined in the Tool definition.
2. Inside updateEnableState(), set this.enabled = true;
3. Observe that the tool is always disabled, regardless of current layer or other extension settings.

Expected behavior
Tool extensions should be able to dynamically set enabled status inside updateEnabledState() when either:

• No targetLayerType is defined
• The current layer matches the targetLayerType

Specifications:

• OS: Windows 10
• Tiled Version: 1.10.1

NOTES

• This might have been introduced by the change to call AbstractTileTool:updateEnabledState() instead of AbstractTool:updateEnabledState()? (diff)
• My current workaround is to disable in the mousePressed() event if an incorrect layer is selected, since this tool happens to be related to tile placement. But it would be better to disable it sooner when the layer is selected.
• The types for tiled.registerTool include the full Tool interface, which has no optional properties. I think the types could be improved to indicate most fields are optional. As it is, I had to ignore a Typescript error in order to avoid defining dozens of properties/functions on my tool.

[bjorn]

• The types for tiled.registerTool include the full Tool interface, which has no optional properties. I think the types could be improved to indicate most fields are optional. As it is, I had to ignore a Typescript error in order to avoid defining dozens of properties/functions on my tool.

Hmm, how does this work? Maybe this means we can't simply reuse the Tool interface for the argument to registerTool? These properties are not optional in the ScriptedTool class they are documenting, and marking them as such might result in other TS issues when trying to use these properties? Also, does it give errors even for readonly properties like map and selectedTile, which are never meant to be provided by the user?

I guess the proper solution would be to have a separate interface ToolDefinition, though the this object it gets is actually a Tool instance... maybe these tricks we're doing can't be properly represented in a TS definition:

tiled/src/tiled/scriptedtool.cpp

Lines 77 to 80 in cb4c33c

	// Make members of ScriptedTool available through the original object
	auto &scriptManager = ScriptManager::instance();
	auto self = scriptManager.engine()->newQObject(this);
	mScriptObject.setPrototype(self);

[bjorn]

@TehCupcakes Regarding the actual bug report, it seems what broke this is the initial call setTargetLayerType(0) in the ScriptedTool constructor. This in turn causes a call to ScriptedTool::updateEnabledState, which then calls into the script before the above setup on mScriptObject has been performed. I'll need to look into rearranging this code, but unfortunately no longer have time before leaving on holiday.

The issue can be worked around as follows:

tiled.registerTool('test', {
    name: "test",
    updateEnabledState() {
        if (this.toString() === '[object Object]')
            return; // unintended and too early call from ScriptedTool constructor
        this.enabled = true;
    }
});

This avoids overriding the enabled property of the C++ class before it has been set as the prototype.

[TehCupcakes]

• The types for tiled.registerTool include the full Tool interface, which has no optional properties. I think the types could be improved to indicate most fields are optional. As it is, I had to ignore a Typescript error in order to avoid defining dozens of properties/functions on my tool.

Hmm, how does this work? Maybe this means we can't simply reuse the Tool interface for the argument to registerTool? These properties are not optional in the ScriptedTool class they are documenting, and marking them as such might result in other TS issues when trying to use these properties? Also, does it give errors even for readonly properties like map and selectedTile, which are never meant to be provided by the user?

I guess the proper solution would be to have a separate interface ToolDefinition, though the this object it gets is actually a Tool instance... maybe these tricks we're doing can't be properly represented in a TS definition:

tiled/src/tiled/scriptedtool.cpp

Lines 77 to 80 in cb4c33c

	// Make members of ScriptedTool available through the original object
	auto &scriptManager = ScriptManager::instance();
	auto self = scriptManager.engine()->newQObject(this);
	mScriptObject.setPrototype(self);

Yes, it errors on read only properties. readonly only protects a value from changing after it has been set. TS behaves this way because the initial value still may be required to be passed in (such as in a class constructor.) The error message looks like this:

I think the argument should have its own type, while the return type is still Tool. You can derive the type for the argument from Tool using a combination of Partial<T>, Required<T>, and Omit or Pick. (See docs.) I imagine something like this if you wanted to omit the readonly properties, require name, and indicate that all other values are optional:

export type RegisterToolParams = Partial<Omit<Tool, 'map' | 'selectedTile' | 'tilePosition'>> & Required<Pick<Tool, 'name'>>;

Furthermore, since you pointed out the handler functions operate on a Tool, there's another area for improvement. Functions do not automatically know the type of this, so you have to specify a this parameter in the TS definition for each function or else this will be of type any. For example, Tool should have:

mouseEntered(this: Tool): void;
mouseLeft(this: Tool): void;
mouseMoved(this: Tool, x: number, y: number, modifiers: number): void;

etc.

Thanks for the workaround! Glad you were able to track down the issue. :)

[bjorn]

@TehCupcakes I have so much still to learn about TS, thanks for explaining these solutions! Would you consider opening a pull request to make these changes to docs/scripting-doc/index.d.ts?

[bjorn]

#3800 addresses the issue with the initial updateEnabledState call and also updates the TS definitions to have separate ToolDefinition and Tool interfaces, where the latter extends the former.

I went with the inheritance rather than using Partial, Required, etc. because I think this is likely going to be easier to follow in the generated documentation.

@TehCupcakes Would you have time to give #3800 a quick review?