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 description to some parameters #4716

Merged
merged 6 commits into from
Dec 20, 2022

Conversation

AlexandreSi
Copy link
Collaborator

@AlexandreSi AlexandreSi commented Dec 19, 2022

Fixes #4701

When using the utils UseStandardRelationalOperatorParameters, UseStandardOperatorParameters and UseStandardParameters, this PR adds a new object ParameterOptions that can specify the parameter description and typeExtraInfo (used for selectors). It might holds units in the future.

This allows to replace generic descriptions Value and Value to compare with things like:

image

image

image

@AlexandreSi AlexandreSi force-pushed the add-description-to-some-parameters branch from d193d20 to 0de933d Compare December 19, 2022 18:36
@AlexandreSi AlexandreSi marked this pull request as ready for review December 20, 2022 08:18
@AlexandreSi AlexandreSi requested a review from 4ian as a code owner December 20, 2022 08:18
Comment on lines -108 to +109
.UseStandardRelationalOperatorParameters("number");
.UseStandardRelationalOperatorParameters(
"number", gd::ParameterOptions::MakeNewOptions());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be the default parameter value.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When talking about this with Alex, I was worried of default values in JS. In C++ I guess it could work, and I guess if we put the "optional" flag in Bindings.idl, Emscripten would generate a call with the default value (well, a call to the function without the parameter, so it would take the default value).

I think we should:

  • Still keep the MakeNewOptions for safety for now and consistency.
  • Add the default parameter so that we can then remove things to make them less verbose.

Comment on lines +156 to +159
.UseStandardOperatorParameters(
"number",
ParameterOptions::MakeNewOptions().SetDescription(
_("Volume (0-100)")))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the method could be directly in the metadata to make the declaration faster to write and easier to read.
What do you think about renaming UseStandardOperatorParameters to something like AddOperatorAndValueParameters and declaring methods like SetLastParameterDescription?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking initially of using SetLastParameterDescription, but I have the feeling that it lacks some robustness... what if, one day for some reason, the value is not the "last parameter" anymore?
What if the parameter options include something like an acceptable range, that would have an impact on what operator is valid to use?

Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. As discussed, this ParameterOptions might be removed entirely in the future in favor of a ValueTypeMetadata that would contain more information about a type (type, "extra info", acceptable value, unit - at least :)). It could help represents:
number (without units), string, but also things like "degrees", "speed" (number with a unit) or even "volume" (number with a unit and an acceptable range), "string with selector" (string with a list of acceptable options), etc...

@AlexandreSi AlexandreSi merged commit 41fa153 into master Dec 20, 2022
@AlexandreSi AlexandreSi deleted the add-description-to-some-parameters branch December 20, 2022 12:59
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.

People are stuggling with the unit in audio events.
4 participants