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

Ability to display variable values in text aka Hello, {$playername} #59

Closed
blurymind opened this issue Feb 17, 2020 · 11 comments · Fixed by #73
Closed

Ability to display variable values in text aka Hello, {$playername} #59

blurymind opened this issue Feb 17, 2020 · 11 comments · Fixed by #73

Comments

@blurymind
Copy link
Collaborator

blurymind commented Feb 17, 2020

The yarn spinner guys are adding this, so I wonder if you would be open to add it to bondagejs as well :)

So we could say
I have {$applesCount} apples
or
My name is {$playerName}

I can try to do a PR if this feature is welcome

@blurymind blurymind changed the title Ability to display variables in text Ability to display variable values in text aka Hello, {$playername} Feb 20, 2020
@blurymind
Copy link
Collaborator Author

blurymind commented May 6, 2020

The {expression} syntax displays the result of an expression as a TextNode to the user.
You can for example do
I now have {$applesCount + 1} apples
see yarnspinners documentation for reference
https://yarnspinner.dev/docs/syntax

new things have been added to the yarn syntax that are missing in bondagejs

some yarn syntax instructions are now being added to yarn editor's help dialog. There is some discussion about using yarn spinner's documentation when yarn is using bondagejs which doesnt yet support some stuff
https://github.com/YarnSpinnerTool/YarnEditor/issues/153

@blurymind
Copy link
Collaborator Author

blurymind commented May 6, 2020

@hylyh
There is some other new stuff too, if you look at https://yarnspinner.dev/docs/syntax

I believe we are completely lacking all Format functions that yarnSpinner supports, example:
You have {$apples} [plural {$apples} one="apple" other="apples"]

not sure If I can implement this myself, seems like a bigger task

@ghost
Copy link

ghost commented Aug 20, 2020

That is called an inline expression. I'm working on implementing this in my fork.
The lexer is having a hard time with the end brace token when testing with '{$test}'.

The test string being lexed is {$test}
BeginInlineExp
{ first_column: 1, first_line: 1, last_column: 2, last_line: 1 } '{'
------------------------
Variable
{ first_column: 2, first_line: 1, last_column: 7, last_line: 1 } '$test'
------------------------
EndOfInput
{ first_column: 2, first_line: 1, last_column: 7, last_line: 1 } ''

It returns an EndOfInput for the third lex call, instead of the expected EndInlineExp.
I've noticed that all of the other "begin" and "end" type tokens are two characters, so this may be an existing lexer issue that was never exposed before. I'll keep digging.

I found and fixed the issue in lexer.js. The function isAtTheEndOfLine was using an incorrect condition. Should be like this (instead of ">=" it is ">"):

isAtTheEndOfLine() {
    return this.yylloc.last_column > this.getCurrentLine().length;
}

@blurymind
Copy link
Collaborator Author

I would very much welcome a PR that adds this feature :) even the basic ability to display the player name would be amazing.

More complicated syntax such as
You have {$apples} [plural {$apples} one="apple" other="apples"]
Could be done in another pr?
In any case, thankyou for looking into this

@ghost
Copy link

ghost commented Aug 21, 2020

I would very much welcome a PR that adds this feature :) even the basic ability to display the player name would be amazing.

I am currently working on that. The main difficulty is in dealing with how the lexer removes leading spaces from text. Simply taking out the code that removes the leading whitespace breaks many tests. So that will be a challenge.

Regarding whitespace importance, consider these test cases:
Hello, {$playername}. How are you?
Above should have no whitespace after inline expression.
We can call you {$playername} the Great!
Above should have a whitespace between the player's name and "the".

Because of these different cases, I can't simply add a whitespace after the inline expression in the runner. The lexer is what is stripping out the preceding whitespaces of the text following EndInlineExp token. I'll get it eventually.

More complicated syntax such as
You have {$apples} [plural {$apples} one="apple" other="apples"]
Could be done in another pr?

The syntax reference calls that "Format functions" and it is a totally different beast. I might do it later.

In any case, thankyou for looking into this

You're welcome!

@blurymind
Copy link
Collaborator Author

blurymind commented Jan 6, 2021

@alforno any progress on this? I noticed you have a branch of it. People keep asking about this feature

@ghost
Copy link

ghost commented Jan 6, 2021

I have not done any work on the format functions, but the issue I mention here is resolved in my fork. Last I check my previous pull request was not merged. I don't care if someone wants to take what I've done and make it work here, but I can't skip code revisions that led to this working.

@blurymind
Copy link
Collaborator Author

The format functions should be a separate pr anyways :)

Sorry I haven't reviewed the other pr yet. I need to get back to my computer after the 9th and will give it a spin if @hylyh hasn't had a look yet.

Thank you for working on these and sorry for the slow reactions. The fixes and features are very much welcome and requested by a wider community of users outside of this tracker

@Chrissdroid
Copy link

Chrissdroid commented Apr 9, 2021

If needed you can uses the javascript version of Message Format witch is used for YarnSpinner

like something like that (my code is not perfect, don't hesitate to correct me)

const MessageFormat = require('messageformat');
const mf = new MessageFormat('en');

if (result instanceof bondage.TextResult) {
	const msg = mf.compile(result.text);

	console.log(msg(runner.variables.data));
}

@blurymind
Copy link
Collaborator Author

blurymind commented Apr 12, 2021

@Chrissdroid that is a fantastic suggestion. If we can maintain less code by using this library, it would leave us in a much much better place. Whats even better, since YarnSpinner is also using it - I assume we will always have syntax parity with it without having to manually update it.

Would you be interested in creating a PR for this feature? It has been by far the most requested feature on this and other trackers using bondagejs.

If you do, your PR will get high priority and I promise will be reviewed quickly.
Your suggested code looks like a very clean/safe way of doing it, so I am intrigued if it does.

If you have trouble with writing a test for it, I can help out with that after you put up the pr.

As first step its great to have it in bondage.TextResult, but it could also be useful in returned bondage.OptionsResult, although we should confirm if that is supported in yarnSpinner

@alforno @hylyh what do you think?

@blurymind
Copy link
Collaborator Author

blurymind commented Apr 14, 2021

There is now a PR for this, however its formater syntax is different from the yarnSpinner one. I raised a question at the library if we can make it work
messageformat/messageformat#318

I am not sure if yarnSpinner actually does use MessageFormat. Its source code looks like they are doing it all manually
YarnSpinnerTool/YarnSpinner@c6e3756#diff-0c18cde9b6cbd9669fb7b6b5a0d69f3c49bdb72f965020e5be5466135dc8b358R402

They seem to be doing some string replacing there - maybe we can also do that before feeding it to MessageFormat?

although correct me if I'm wrong :)

We still need to get the syntax be the same somehow. Otherwise we would be deviating from the yarnSpinner docs

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 a pull request may close this issue.

2 participants