Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'StringList' as new parameter type for extensions #911

Merged
merged 14 commits into from
Feb 8, 2019

Conversation

Wend1go
Copy link
Contributor

@Wend1go Wend1go commented Jan 28, 2019

There is just a small "uglyness" (see below)

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 28, 2019

stringlistfield

@4ian
Copy link
Owner

4ian commented Jan 28, 2019

Sweet!
Two things as you can see on SemaphoreCI: https://semaphoreci.com/4ian/gd/branches/pull-request-911/builds/2

  • Flow is warning you that Cannot call parameterMetadata.getExtraInfo because property getExtraInfo is missing in undefined
    image

This means that you have to add a if case or handle in some way the case where "parameterMetadata" is undefined (which can happen if the field is used in some strange conditions).

  • Run npm run format/yarn format to auto format your code.

Do you intend to use this soon for an extension? Or is it more to prepare things for later extensions?

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 29, 2019

I wanted to use it in the Tween extension. But I still need to do some research on how Tween.js works and how it best fits into the GD workflow.

I'll add the check and format this evening.
Did I get you right that getExtraInfo() will keep the ExtensionName:: prefix for the foreseeable future?
In this case I would change the replace function into "erase everything on the left up to the double colon ::.

@4ian
Copy link
Owner

4ian commented Jan 29, 2019

Did I get you right that getExtraInfo() will keep the ExtensionName:: prefix for the foreseeable future?

No I think I'll remove the prefix (precisely, I'll apply the prefix only for parameters that are objects).

Perform auto formatting
@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 29, 2019

Ok done 😃

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 30, 2019

I wonder if it is possible to edit the list of items bound to the StringListField from within the extension that uses it.
That way it would be possible to automatically show a list of identifiers from already existing tween animations or identifiers of scene and object timers.

@4ian
Copy link
Owner

4ian commented Feb 5, 2019

FYI, I'll do the changes but got carried out by other things when working on translations :)

@Wend1go
Copy link
Contributor Author

Wend1go commented Feb 5, 2019

No problem, I'll probably spend the next two weeks on the tweening extension. I'll switch to the StringLists shortly before the extension is ready.

@Wend1go Wend1go mentioned this pull request Feb 5, 2019
21 tasks
@4ian
Copy link
Owner

4ian commented Feb 6, 2019

I've fixed the "extra info" and added handling of "stringWithSelector", understood as a string by the game engine. "stringWithSelector" sounded better because the type sent to the JS function is a string, not a list/array of strings.

To use this:

  • Merge master branch into your branch.
  • Remove newIDE/app/srclibGD.js file
  • Relaunch npm run start or yarn start.

Let me know if it works.
You'll have to change "stringlist" to "stringWithSelector" and rename your file for consistency. Apart from this, should work like before!

Wend1go and others added 2 commits February 7, 2019 20:11
Remove workaround for preceeding classname in extraInfo
@Wend1go
Copy link
Contributor Author

Wend1go commented Feb 7, 2019

I couldn't find libGD.js under the path you mentioned. Or did you meanGDevelop/newIDE/app/public/libGD.js?
After removing the file it got downloaded again but when I parse extraInfo there is still the preceeding ClassName::
In other words, I couldn't test this yet.

EDIT:
BTW, is this possible?

@4ian
Copy link
Owner

4ian commented Feb 7, 2019

You have to merge master branch in your branch ;)

you mean GDevelop/newIDE/app/public/libGD.js

Yes sorry, this file. Merge master, delete it and relaunch npm run start so that it get downloaded

@4ian
Copy link
Owner

4ian commented Feb 7, 2019

I wonder if it is possible to edit the list of items bound to the StringListField from within the extension that uses it.

No easy way, we would need some kind of abstraction allowing to declare types of identifiers.
And have IDE scan the events to detect those who are declared, like variables, and a place where to store them, like variables

@Wend1go
Copy link
Contributor Author

Wend1go commented Feb 8, 2019

Ok the items are correctly shown now.
But the returned value in my test extension is an empty string.
Have to go to work now, I'll look into this when I come home.

This should be the culprit. Will test it tonight.
@Wend1go
Copy link
Contributor Author

Wend1go commented Feb 8, 2019

Guess I found the culprit. Will test it tonight.

@4ian
Copy link
Owner

4ian commented Feb 8, 2019

Oh thinking about it now, as this is considered as a string, you have to ensure that quotes are in your values:

        dataSource={this._choices.map(choice => ({
          text: choice,
          value: '"' + choice + '"',
        }))}

@Wend1go
Copy link
Contributor Author

Wend1go commented Feb 8, 2019

Now it is working :)

@4ian
Copy link
Owner

4ian commented Feb 8, 2019

Sweet! Looks good for me to merge

@4ian
Copy link
Owner

4ian commented Feb 8, 2019

Ran Prettier on your file, remember to run yarn format before committing ;)

@4ian 4ian merged commit 6a02f1b into 4ian:master Feb 8, 2019
@4ian
Copy link
Owner

4ian commented Feb 8, 2019

Merged! Thanks for adding this and look forward to see it being used in the Tween extension :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants