String autocompletion in expressions for scene names, layer names and choices #2702

D8H · 2021-06-03T01:20:01Z

This add autocompletion for strings in expressions.
It uses the delimiters found by the parser to replace the existing text to avoid to have a pseudo parser on UI side.

4ian

Very cool! Added some comments/nitpickings/small things to fix.

It uses the delimiters found by the parser to replace the existing text to avoid to have a pseudo parser on UI side.

Very nice! This pseudo parser of separators on the UI side has always been a pain :) Very happy to see this gone.
Passing the positions of the replacement is a better idea.

Two things important to know:

In the IDE, the typing is using just number. There is a also a few places to update, see https://semaphoreci.com/4ian/gd/branches/pull-request-2702/builds/1
There are also C++ tests that you can run by compiling the native tests:
- mkdir build-tests, cd build-tests, cmake .. -DBUILD_GDJS=FALSE -DBUILD_TESTS=TRUE, make -j 4 and run the tests: Core/GDCore_tests.
- This is not well documented, sorry! I can help fixing these tests, so concentrate on the Flow typing errors for now :)

I've also seen the issue with globalObjectsContainer casted to gdProject, I think we can avoid this as this assumption may/will not always hold, I'll post a comment about this too.

Overall great improvement, looking forward to this being complete :)

4ian · 2021-06-03T08:32:49Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+  size_t GetReplacementEndPosition() const { return replacementEndPosition; }
+
+  /**
+   * \brief Set if the expession is the last child of a function call.


(here and below) expression

4ian · 2021-06-03T08:33:18Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

  }
  void OnVisitNumberNode(NumberNode& node) override {
    // No completions
  }
  void OnVisitTextNode(TextNode& node) override {
-    // No completions
+    FunctionCallNode* functionCall = dynamic_cast<FunctionCallNode*>(parentNodeAtLocation);
+    if (functionCall != NULL) {


nit: prefer nullptr

4ian · 2021-06-03T08:33:58Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+      if (parameterIndex < 0) {
+        return;
+      }
+      // Search the metadata parameter index skiping invisible ones


typo: skipping + period at the end of the sentence.

4ian · 2021-06-03T08:35:26Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+        }
+        metadataParameterIndex++;
+      }
+      const gd::String &type = functionCall->expressionMetadata.parameters[metadataParameterIndex].GetType();


Will this crash if there is for example no parameter in the expression metadata but you still tried to write a parameter in the function?

I think it only worked because no completion is asked by the GUI when there is a parsing error.

4ian · 2021-06-03T08:37:08Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+        }
+        metadataParameterIndex++;
+      }
+      const gd::String &type = functionCall->expressionMetadata.parameters[metadataParameterIndex].GetType();


But I like the idea of looking at the expression metadata 👍👍

4ian · 2021-06-03T08:41:38Z

newIDE/app/src/ExpressionAutocompletion/index.js

+
+    const lowercaseSearchText = searchText.toLowerCase();
+
+    return list.filter((variableName: string) => {


Why variableName?

4ian · 2021-06-03T08:43:02Z

newIDE/app/src/ExpressionAutocompletion/index.js

-  ) {
-    wordEndPosition++;
-  }
+  let wordStartPosition: integer =


Use const + number.

4ian · 2021-06-03T08:43:26Z

newIDE/app/src/ExpressionAutocompletion/index.js

@@ -345,47 +430,17 @@ export const insertAutocompletionInExpression = (
    };
  }

-  // Start from the character just before the caret.


Much much better without this 👍 So happy to see this gone.

4ian · 2021-06-03T08:45:56Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+    gd::ExpressionNode* nodeAtLocation = finder.GetNode();
+    gd::ExpressionNode* parentNodeAtLocation = finder.GetParentNode();
+
+    std::cout << "GetCompletionDescriptionsFor Test" << std::endl;


Let's remove this if not useful anymore

4ian · 2021-06-03T08:46:18Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

@@ -6,6 +6,8 @@
 #ifndef GDCORE_EXPRESSIONAUTOCOMPLETIONPROVIDER_H
 #define GDCORE_EXPRESSIONAUTOCOMPLETIONPROVIDER_H

+#include <fstream>


I think we can remove these headers if you remove the std::cout <<?

4ian · 2021-06-03T08:58:50Z

newIDE/app/src/ExpressionAutocompletion/index.js

+      autocompletionTexts.push(`"${layout.getLayerAt(index).getName()}"`);
+    }
+  } else if (type === 'sceneName') {
+    const project = (globalObjectsContainer: gdProject);


We should be able to pass the project in the expressionAutocompletionContext when calling getAutocompletionsFromDescriptions :)

4ian · 2021-06-03T09:03:10Z

newIDE/app/src/ExpressionAutocompletion/index.js

+
+  let autocompletionTexts: string[] = [];
+  if (type === 'layer') {
+    const layout = (objectsContainer: gdLayout);


Same as the project: we should be able to pass the layout when calling getAutocompletionsFromDescriptions and have it passed down thanks to the context. You can find it into the scope (of type EventsScope) in GenericExpressionField :)

But we have to be even more cautious! It's possible that we're in a context where we don't have a layout (so the type will be a ?gdLayout), for example when editing a function.

D8H · 2021-06-03T13:16:07Z

There is still one flow issue. I don't know how to make gdExpressionCompletionDescription has the new enum element Text. I bound it but I must have missed something.

4ian

I don't know how to make gdExpressionCompletionDescription has the new enum element Text. I bound it but I must have missed something.

Yes this is manually typed actually! See generate-types.js in GDevelop.js/scripts.

4ian · 2021-06-03T17:18:14Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

@@ -80,13 +80,15 @@ struct ExpressionCompletionDescription {
   */
  static ExpressionCompletionDescription ForText(
      const gd::String& type_,
+      const gd::ParameterMetadata *parameterMetadata_,


Uber nitpicking: using a pointer to store an "optional reference to something, that you're not owning" is fine, but prefer to keep using references in the interface (i.e: in ForText and in the SetParameterMetadata/GetParameterMetadata).
In other words, you can pass a const gd::ParameterMetadata & parameterMetadata_ (and store its pointer parameterMetadata = &parameterMetadata_;).
With a pointer, there is a possible misunderstanding about if the class is taking ownership of the pointer or not.

(You can create a "HasParameterMetadata" if you need to know if this was set or not).

D8H · 2021-06-03T19:07:45Z

It creates a lot of file when executing cmake -DBUILD_GDJS=FALSE -DBUILD_TESTS=TRUE from repository root.
Is this why you said to create this directory mkdir build-tests ?
Should I clone something in it?

4ian · 2021-06-03T22:51:12Z

You should not need to clone anything, but I forgot in my instruction to tell CMake (the build generator system) that the sources root are in the parent directory. This should work better with the ..:
mkdir build-tests, cd build-tests, cmake .. -DBUILD_GDJS=FALSE -DBUILD_TESTS=TRUE, make -j 4 and run the tests: Core/GDCore_tests

The mkdir build-tests && cd build-tests is to avoid building directly in the sources directory. This is called an "out of source build". It means that all the generated/compiled binaries files will be in this folder (useful if you want to restart your build from scratch, just delete the folder. It's a bit like deleted the node_modules folder).

Give it another try and let me now if the cmake works better now (with the ..). In theory it should generate a bunch of files in build-tests, notably a few folders and a "Makefile". You can then run the makefile with make -j 4.
All of this is automated by GDevelop.js when targeting Webassembly, but we should probably offer a few shortcuts or documentation for the native tests build like this.

D8H · 2021-06-04T00:06:33Z

The cmake shows no errors, the makefile is not created (I'm on windows, should I try with WSL?).

build-tests> make -j 4
make: *** No targets specified and no makefile found.  Stop.

Edit: I tried with WSL Ubuntu 20.04. It produces the Makefile but Make crashed at 7% because of this I guess:

In file included from /mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/Unix/GlxContext.hpp:32,
                 from /mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/GlContext.cpp:51:
/mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/Unix/GlxExtensions.hpp:123:3: error: conflicting declaration ‘typedef struct GLXHyperpipeNetworkSGIX GLXHyperpipeNetworkSGIX’
  123 | } GLXHyperpipeNetworkSGIX;
      |   ^~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/GL/glx.h:333,
                 from /mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/Unix/GlxExtensions.hpp:36,
                 from /mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/Unix/GlxContext.hpp:32,
                 from /mnt/c/WorkspaceGDevelop/GDevelop/ExtLibs/SFML/src/SFML/Window/GlContext.cpp:51:
/usr/include/GL/glxext.h:771:3: note: previous declaration as ‘typedef struct GLXHyperpipeNetworkSGIX GLXHyperpipeNetworkSGIX’
  771 | } GLXHyperpipeNetworkSGIX;
      |   ^~~~~~~~~~~~~~~~~~~~~~~

From a fresh install, I installed these libs:

sudo apt install libx11-dev libxrandr-dev libgl1-mesa-dev libudev-dev libjpeg-dev libopenal-dev libflac-dev libvorbis-dev libfreetype-dev

D8H · 2021-06-04T20:02:42Z

The tests seem to be ok now.
New tests are needed to cover the new cases but I would rather being able to launch them locally before writing new ones (unless it blocks this PR).
I used this project to test it manually:
UndefinedVariableTest.zip

4ian · 2021-06-04T23:53:41Z

Edit: I tried with WSL Ubuntu 20.04. It produces the Makefile but Make crash at 7% because of this I guess:

Right, seems like the unused by outdated SFML dependency is causing us issue on Linux in general. We should remove it..
I don't have an immediate solution, so I suggest you can write a TODO with tests to add, I can maybe write a few and then we go ahead and merge this, as the rest looks very nice :)

4ian · 2021-06-05T12:03:58Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

    if (nodeAtLocation == nullptr) {
      std::vector<ExpressionCompletionDescription> emptyCompletions;
      return emptyCompletions;
    }

-    gd::ExpressionCompletionFinder autocompletionProvider(searchedPosition);
+    gd::ExpressionCompletionFinder autocompletionProvider(searchedPosition, *parentNodeAtLocation);


This is dereferencing a pointer that can (and is) null. I'm fixing this, as this can generate horrible crashs if used.

4ian · 2021-06-05T12:32:36Z

Core/GDCore/IDE/Events/ExpressionCompletionFinder.h

+  /**
+   * \brief Return the parameter metadata if the completion is about a parameter.
+   */
+  const gd::ParameterMetadata& GetParameterMetadata() const { return *parameterMetadata; }


It's generally a good idea to be safe to use the "null object pattern".
We keep a pointer internally (so that we can modify what is pointed easily) but instead of setting it to nullptr when it's empty, we point it to an empty object (here, a ParameterMetadata constructed without anything and stored as a static member of the class).
This is almost equally performant (to check if the parameter metadata is defined, you just check if the pointer is not pointing to this empty object, called badParameterMetadata).
And this is a bit safer, in the sense that if someone tries to read the parameter metadata in a context where it's not valid, it's not crashing.
Because these classes are used in the IDE, it's fine being a bit more "forgiving" in case of misusage.

(An better alternative, safe and always correct would be to use an Optional like type, so that you would be forced to check if the parameter metadata is defined before using it. But we use this "null object pattern" a lot and it works well enough!)

EDIT: I fixed this myself :)

…ests for the parent node finder and completion finder

4ian · 2021-06-05T13:27:52Z

Merged! Thanks a lot for working on this, that's a great improvement over what we had 👍

D8H added 6 commits June 1, 2021 02:10

WIP: String autocompletion in expressions

a495cdd

WIP: String autocompletion insertion

4e1020f

Let the parser tell what to replace for autocompletion

cb8120a

Remove logs

715914e

Remove useless todo

3c5e9b7

Remove new empty line

b6a0980

4ian reviewed Jun 3, 2021

View reviewed changes

D8H added 3 commits June 3, 2021 14:13

Fix flow

11fd8ca

Fix last parameter detection

65f4e98

Add a test on layer parameters

6505294

Remove logs

317ff5f

D8H changed the title ~~String (Scene and Layer) autocompletion in expressions~~ String autocompletion for scene names, layer names and choices in expressions Jun 3, 2021

D8H changed the title ~~String autocompletion for scene names, layer names and choices in expressions~~ String autocompletion in expressions for scene names, layer names and choices Jun 3, 2021

4ian reviewed Jun 3, 2021

View reviewed changes

D8H added 2 commits June 3, 2021 23:17

Add CompletionKind.Text

7616743

Autocompletion for strings with selector

c7711ea

D8H force-pushed the ValueExpressionAutocompletion branch from 708e1d8 to c7711ea Compare June 3, 2021 22:12

Move getParameterChoices to fix imports.

ddee1ca

D8H added 3 commits June 4, 2021 02:20

Fix imports

d58cfb9

Attempt to fix tests

d791fd9

Include ExpressionMetadata

ee3a640

D8H marked this pull request as ready for review June 4, 2021 19:54

4ian added the 🏁PR almost ready: final fixes The PR is almost ready and needs final fixes and/or polishing before merge label Jun 4, 2021

D8H mentioned this pull request Jun 5, 2021

Variable autocompletion in expressions #2692

Merged

4ian reviewed Jun 5, 2021

View reviewed changes

Fix null dereference, make parameter metadata usage safer, add some t…

12ecc68

…ests for the parent node finder and completion finder

4ian merged commit 4f91545 into 4ian:master Jun 5, 2021

D8H deleted the ValueExpressionAutocompletion branch June 7, 2021 12:27

D8H mentioned this pull request Jul 4, 2021

Doubled extension prefix #2195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

String autocompletion in expressions for scene names, layer names and choices #2702

String autocompletion in expressions for scene names, layer names and choices #2702

D8H commented Jun 3, 2021

4ian left a comment •

edited

Loading

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

D8H Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

4ian Jun 3, 2021

D8H commented Jun 3, 2021

4ian left a comment

4ian Jun 3, 2021

D8H commented Jun 3, 2021 •

edited

Loading

4ian commented Jun 3, 2021

D8H commented Jun 4, 2021 •

edited

Loading

D8H commented Jun 4, 2021 •

edited

Loading

4ian commented Jun 4, 2021

4ian Jun 5, 2021

4ian Jun 5, 2021 •

edited

Loading

4ian commented Jun 5, 2021


		const lowercaseSearchText = searchText.toLowerCase();

		return list.filter((variableName: string) => {

String autocompletion in expressions for scene names, layer names and choices #2702

String autocompletion in expressions for scene names, layer names and choices #2702

Conversation

D8H commented Jun 3, 2021

4ian left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D8H commented Jun 3, 2021

4ian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D8H commented Jun 3, 2021 • edited Loading

4ian commented Jun 3, 2021

D8H commented Jun 4, 2021 • edited Loading

D8H commented Jun 4, 2021 • edited Loading

4ian commented Jun 4, 2021

Choose a reason for hiding this comment

4ian Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

4ian commented Jun 5, 2021

4ian left a comment •

edited

Loading

D8H commented Jun 3, 2021 •

edited

Loading

D8H commented Jun 4, 2021 •

edited

Loading

D8H commented Jun 4, 2021 •

edited

Loading

4ian Jun 5, 2021 •

edited

Loading