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

Editor Help: Improve appearance of script property setters/getters #80765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Aug 18, 2023

## a description.
var a = 1:
    set(value):
        a = value
    get:
        return a

## b description.
var b = 2: set = set_b, get = get_b

func set_b(value):
    b = value

func get_b():
    return b


Comment on lines 1758 to 2097
if (cd.properties[i].setter.begins_with("@")) {
// GDScript inline setter.
class_desc->add_text("<inline setter>" + TTR("(value)"));
} else {
class_desc->add_text(cd.properties[i].setter + TTR("(value)"));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to prevent user confusion. Ironically, the following code works:

call("@a_setter", 123)

@dalexeev dalexeev changed the title Editor help improve script setter getter apperance Editor Help: Improve appearance of script property setters/getters Aug 18, 2023
@dalexeev dalexeev force-pushed the editor-help-improve-script-setter-getter-apperance branch 3 times, most recently from 53905a7 to 80497a9 Compare August 21, 2023 07:17
@dalexeev
Copy link
Member Author

dalexeev commented Aug 21, 2023

  1. Undocumented setter/getter functions are not displayed (this should be documented here), but documented ones are now displayed.
  2. Fixed signatures for setters/getters with additional arguments (registered via ADD_PROPERTYI() macro).
    • Online docs already shows this, but uses the full signature. I'm not sure if this makes sense, as it's more cumbersome and there is no info in DocData about the signatures of normal setters/getters.
## a description.
var a = 1:
    set(value): a = value
    get: return a

## b description.
var b = 2: set = set_b, get = get_b
func set_b(value): b = value
func get_b(): return b

## c description.
var c = 3: set = set_c, get = get_c
## set_c description.
func set_c(value): c = value
## get_c description.
func get_c(): return c

@dalexeev dalexeev marked this pull request as ready for review August 21, 2023 07:46
@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

Are you interested in reworking this PR, especially after your recent clean-up within EditorHelp?

@dalexeev dalexeev force-pushed the editor-help-improve-script-setter-getter-apperance branch 2 times, most recently from c93c32b to c0e4a54 Compare February 19, 2024 16:28
@dalexeev dalexeev force-pushed the editor-help-improve-script-setter-getter-apperance branch from c0e4a54 to b946b74 Compare February 19, 2024 18:29
@@ -1139,7 +1141,7 @@ void EditorHelp::_update_doc() {
bool overridden_property_exists = false;

for (const DocData::PropertyDoc &prop : cd.properties) {
// Ignore undocumented private.
// Ignore undocumented private properties.
const bool is_documented = prop.is_deprecated || prop.is_experimental || !prop.description.strip_edges().is_empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here

Suggested change
const bool is_documented = prop.is_deprecated || prop.is_experimental || !prop.description.strip_edges().is_empty();
bool is_documented = prop.is_deprecated || prop.is_experimental || !prop.description.strip_edges().is_empty();

Copy link
Member

Choose a reason for hiding this comment

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

Not a requirement, personal preference

Comment on lines +1315 to +1316
const int argc = method.arguments.size();
const int skip_flags = skip_methods[method.name];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const int argc = method.arguments.size();
const int skip_flags = skip_methods[method.name];
int argc = method.arguments.size();
int skip_flags = skip_methods[method.name];

class_desc->push_cell();
_push_code_font();
class_desc->push_color(theme_cache.text_color);
const bool has_setter_link = method_line.has(prop.setter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const bool has_setter_link = method_line.has(prop.setter);
bool has_setter_link = method_line.has(prop.setter);

class_desc->push_cell();
_push_code_font();

const bool has_getter_link = method_line.has(prop.getter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const bool has_getter_link = method_line.has(prop.getter);
bool has_getter_link = method_line.has(prop.getter);

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

Successfully merging this pull request may close these issues.

[4.0] GDScript custom class documentation doesn't mention name of setter / getter methods
3 participants