-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add support for marking custom functions as deprecated with visual warnings #8156
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
base: master
Are you sure you want to change the base?
Add support for marking custom functions as deprecated with visual warnings #8156
Conversation
|
Hello! Thanks for opening this PR. The extension editor UI is still a bit rough today (the extension editor has changed a lot lately and we still try to improve it). The "Private" checkbox should have been named "Hidden": when you check it, you will see that existing usages of the function appear in yellow. So I wonder if this would have fitted your need? More context: We try to make depreciation as transparent as possible for users. This means:
In your case, I guess you want to have the deprecation explanation shown to your users? |
Hi, I know about the "Hidden" checkbox, but it's intended for use within the extension itself, isn't it? |
| if (showDeprecatedInstructionWarning) { | ||
| hasDeprecationWarning = validationResult.hasDeprecationWarning(); | ||
| } | ||
| validationResult.delete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably create a crash/memory corruption. Indeed, the JS/C++ bindings are using a [Value]:
[Value] ParameterValidationResult STATIC_ValidateParameter
This means (you can check that in glue.cpp, in the C function generated for interfacing) that the returned object is a static object in C++. There is always one living in memory, which is reused across calls. Which means:
- Multiple calls will refer to the same object (the second call "erases" the first one)
- If you delete it, you actually destroyed static memory which is basically undefined behavior I guess in C++/wasm. In short, it's a crash next time this memory is accessed.
This fix is simple: don't delete it :) This is sadly the hard to find/detect bugs with the C++/JS bindings (unclear lifetime of returned C++ objects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, removed delete call for static object
| }; | ||
|
|
||
| gd::String MetadataDeclarationHelper::GetFreeFunctionSentence(const gd::EventsFunction &eventsFunction) { | ||
| // Note: [DEPRECATED] prefix is now added in the UI layer (Instruction.js) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this note is useful? This seems like a detail for the reader of the code, this could be removed I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, removed
4ian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I left a comment. My main other comment is that "[DEPRECATED]" is very "heavy" on the screen and I would rather have it as an additional preference (or we remove the existing preference and replace it by a preference which is an enum: "showDeprecatedOrHiddenWarning": "no" | "icon" | "icon-and-deprecated-warning-text").
Let me know if you can rework or introduce this preference :)
|
Hi @4ian, the idea sounds great! I've made appropriate changes, please take a look. |
767fcc1 to
81330bd
Compare
81330bd to
a0277ab
Compare












Summary
Changes
Core/GDCore:
SetDeprecationMessageandIsDeprecatedmethods toInstructionMetadata,ExpressionMetadata, andMultipleInstructionMetadataDeprecatedExpressionerror type toExpressionParserErrorInstructionValidatorwithValidateParameterthat returns both validity and deprecation status via newParameterValidationResultstructEventsFunctionmodel (isDeprecated,deprecationMessage)Events Sheet (UI):
[DEPRECATED]prefix for deprecated actions and conditionsshowDeprecatedInstructionWarningpreference flagExtension Editor: