Autocompletion using expression parameter sub-types (layer, color...)

[D8H]

Use case

Expressions like MouseX(:Layer, :number) have a general string autocompletion for its layer parameter. It could be interesting to refine autocompletion using the subtype "Layer" instead of the type "string".
https://trello.com/c/hdJPfWtC/200-add-dropdown-with-list-of-points-for-parameters-asking-for-a-sprite-point-name-in-events

Code limitations

The GUI calls gd.ExpressionCompletionFinder.getCompletionDescriptionsFor() which only gives string or number as type for parameters no matter the subtype.

GDevelop/newIDE/app/src/EventsSheet/ParameterFields/GenericExpressionField/index.js

Lines 340 to 343 in 98303f7

	const completionDescriptions = gd.ExpressionCompletionFinder.getCompletionDescriptionsFor(
	expressionNode,
	cursorPosition - 1
	);

Looking at the Core code, the parser loses the information on the subtype when it builds the tree:

GDevelop/Core/GDCore/Events/Parsers/ExpressionParser2.h

Lines 579 to 586 in 98303f7

	const gd::String &type = parameterMetadata[parameterIndex].GetType();
	if (parameterMetadata[parameterIndex].IsCodeOnly()) {
	// Do nothing, code only parameters are not written in expressions.
	} else if (gd::ParameterMetadata::IsExpression("number", type)) {
	parameters.push_back(Expression("number"));
	} else if (gd::ParameterMetadata::IsExpression("string", type)) {
	parameters.push_back(Expression("string"));
	} else if (gd::ParameterMetadata::IsExpression("variable", type)) {

The subtype to type mapping is in IsExpression()

GDevelop/Core/GDCore/Extensions/Metadata/ParameterMetadata.h

Lines 174 to 196 in 98303f7

	/**
	* \brief Return true if the type of the parameter is an expression of the
	* given type.
	* \note If you had a new type of parameter, also add it in the IDE (
	* see EventsFunctionParametersEditor) and in the EventsCodeGenerator.
	*/
	static bool IsExpression(const gd::String &type,
	const gd::String &parameterType) {
	if (type == "number") {
	return parameterType == "expression" || parameterType == "camera" ||
	parameterType == "forceMultiplier";
	} else if (type == "string") {
	return parameterType == "string" || parameterType == "layer" ||
	parameterType == "color" || parameterType == "file" ||
	parameterType == "joyaxis" ||
	parameterType == "stringWithSelector" ||
	parameterType == "sceneName";
	} else if (type == "variable") {
	return parameterType == "objectvar" || parameterType == "globalvar" ||
	parameterType == "scenevar";
	}
	return false;
	}

Solution

How should the subtype make its way to the GUI?
I guess that adding it to ExpressionNode may not be ideal as it would only be useful for instances that are the parameter child of a FunctionCallNode.
But when a user is writing a parameter, getCompletionDescriptionsFor will give the description of the ExpressionNode that represents the parameter.

[D8H]

I think it could be done by modifying ExpressionCompletionFinder to:

• Get the parent of nodeAtLocation from ExpressionNodeLocationFinder
• Make a specific behavior in OnVisitNumberNode, OnVisitTextNode and OnVisitVariableNode if the parent is a FunctionCallNode.

GDevelop/Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

Lines 178 to 197 in 822146e

	/**
	* \brief Given the expression, find the node at the specified location
	* and returns completions for it.
	*/
	static std::vector<ExpressionCompletionDescription>
	GetCompletionDescriptionsFor(gd::ExpressionNode& node,
	size_t searchedPosition) {
	gd::ExpressionNode* nodeAtLocation =
	gd::ExpressionNodeLocationFinder::GetNodeAtPosition(node,
	searchedPosition);
	if (nodeAtLocation == nullptr) {
	std::vector<ExpressionCompletionDescription> emptyCompletions;
	return emptyCompletions;
	}
	gd::ExpressionCompletionFinder autocompletionProvider(searchedPosition);
	nodeAtLocation->Visit(autocompletionProvider);
	return autocompletionProvider.GetCompletionDescriptions();
	}

[4ian]

Make a specific behavior in OnVisitNumberNode, OnVisitTextNode and OnVisitVariableNode if the parent is a FunctionCallNode.

I'm a bit worried of this kind of approach, intuitively it sounds a bit hacky (but maybe I've not entirely understood the issue).

[D8H]

At first, I though that passing the refined type to the children could be a solution.
But, for instance, "Layer" in MouseX("Layer", 0) is a layer only because of its parent MouseX. So, to me, it feels more logical to use the parent rather than a side effect of the parent on its child.

And in case of autocompletion, having the refined node type is not enough because if the expression is MouseX("Layer" + Variable(layerIndex), 0) and the cursor inside "Layer", I think the autocompletion should stay generic.

[4ian]

But, for instance, "Layer" in MouseX("Layer", 0) is a layer only because of its parent MouseX

Sure, but this is not forbidding us to pass to the children expression node "your a string, but more precisely you represents a layer" right?

The parser will create a representation like this:

FunctionCallNode with 2 parameters:
- TextNode (which is a ExpressionNode with type "string")
- NumberNode (which is a ExpressionNode with type "number")

if we pass more information, we might be able to create completions for TextNode that are the possible values for the layers.

having the refined node type is not enough because if the expression is MouseX("Layer" + Variable(layerIndex), 0) and the cursor inside "Layer", I think the autocompletion should stay generic.

We could surely display the additional completions (here, the layer names) only if the expression is composed of a single text node or empty?

[D8H]

Sure, but this is not forbidding us to pass to the children expression node "your a string, but more precisely you represents a layer" right?

Do you think it should affect only the 1st depth child or propagate until another function call?

We could surely display the additional completions (here, the layer names) only if the expression is composed of a single text node or empty?

Yes, I think the parent is needed to write this condition. This is not really an issue because it can be get without much calculus:

• Adding parentNode = foundNode; before foundNode = &node;
  

  GDevelop/Core/GDCore/IDE/Events/ExpressionNodeLocationFinder.h

  Lines 108 to 118 in 516b1d9

  	private:
  	bool CheckSearchPositionInNode(ExpressionNode& node, bool inclusive = false) {
  	if (node.location.GetStartPosition() <= searchedPosition &&
  	((!inclusive && searchedPosition < node.location.GetEndPosition()) ||
  	(inclusive && searchedPosition <= node.location.GetEndPosition()))) {
  	foundNode = &node;
  	return true;
  	}
  	return false;
  	}

[4ian]

I guess that adding it to ExpressionNode may not be ideal as it would only be useful for instances that are the parameter child of a FunctionCallNode.

It could be maybe still useful to make the "subtype" survives the parsing. Could maybe be used for something else.
So it might be worth checking this approach (passing the "original, concrete type" to the nodes).

[D8H]

I did an implementation here: #2702

[4ian]

Looks nice, I think using the expressionMetadata is the way to go! 👍 Sounds fairly clean and adapted.