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

Set doc_name even when categories are hidden in the inspector #92457

Merged

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented May 28, 2024

As far as I'm aware, this bug only happens with the content_margin_* properties in StyleBoxFlat.

See #92457 (comment).

@YeldhamDev YeldhamDev added this to the 4.3 milestone May 28, 2024
@YeldhamDev YeldhamDev force-pushed the extreme_corner_case_but_still branch 2 times, most recently from c16477c to 3a2f38a Compare May 28, 2024 13:22
@akien-mga akien-mga requested a review from dalexeev May 28, 2024 13:24
@dalexeev
Copy link
Member

We have 2 ways to refer to a class member in documentation:

  1. Local, within the current class/page. URL analogue: #member.
  2. Global, requires a exact class prefix, inheritance is not taken into account. URL analogue: class#member.

That is, if we want to refer to Node.name property, then there are only two ways to do it:

  1. [member name] - only in Node.xml.
  2. [member Node.name] - in all other classes.
  3. [member Control.name] is an invalid link.

If links and/or tooltips do not work somewhere, it may be for the following reasons:

  1. We specified an incorrect doc link in some XML file, and our linter did not detect it.
  2. The editor has a bug with incorrect processing of the correct doc link.

Of course, we could take inheritance into account. But then this should be changed not only for properties, but also for other member types. Plus check that other places (EditorHelp::_class_desc_select(), EditorHelp::_help_callback(), EditorHelpBit::_meta_clicked()) handle this consistently.

Finally, even if we want to add inheritance considerations for primary links, I think that there is little point in checking this for secondary identifiers. All three references [member name], [member Node.name], [member Control.name] must resolve to the same internal identifier property|Node|name. property|Control|name is an invalid internal id.

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good as a hot fix and considering it's already done this way for theme properties. But in the future we should refactor this (again).

@YeldhamDev
Copy link
Member Author

Sorry, I should've been clearer as why this bug happens: the inspector doesn't seem to acknowledge class categories when displaying sub-resources:

Main Resource Sub-Resource
Screenshot_20240528_113443 Screenshot_20240528_113418

So when a tooltip is requested, the class solicited when hovering a content_margin_* property is StyleBoxFlat instead of StyleBox.

@dalexeev
Copy link
Member

I should've been clearer as why this bug happens: the inspector doesn't seem to acknowledge class categories when displaying sub-resources:

We could fix it here:

} else if (p.usage & PROPERTY_USAGE_CATEGORY) {
// Setup a property category.
group = "";
group_base = "";
subgroup = "";
subgroup_base = "";
section_depth = 0;
vbox_per_path.clear();
editor_inspector_array_per_prefix.clear();
// `hint_script` should contain a native class name or a script path.
// Otherwise the category was probably added via `@export_category` or `_get_property_list()`.
const bool is_custom_category = p.hint_string.is_empty();
if ((is_custom_category && !show_custom_categories) || (!is_custom_category && !show_standard_categories)) {
continue;
}
// Hide the "MultiNodeEdit" category for MultiNodeEdit.
if (Object::cast_to<MultiNodeEdit>(object) && p.name == "MultiNodeEdit") {
continue;
}
// Iterate over remaining properties. If no properties in category, skip the category.
List<PropertyInfo>::Element *N = E_property->next();
bool valid = true;
while (N) {
if (!N->get().name.begins_with("metadata/_") && N->get().usage & PROPERTY_USAGE_EDITOR &&
(!filter.is_empty() || !restrict_to_basic || (N->get().usage & PROPERTY_USAGE_EDITOR_BASIC_SETTING))) {
break;
}
// Treat custom categories as second-level ones. Do not skip a normal category if it is followed by a custom one.
// Skip in the other 3 cases (normal -> normal, custom -> custom, custom -> normal).
if ((N->get().usage & PROPERTY_USAGE_CATEGORY) && (is_custom_category || !N->get().hint_string.is_empty())) {
valid = false;
break;
}
N = N->next();
}
if (!valid) {
continue; // Empty, ignore it.
}
// Create an EditorInspectorCategory and add it to the inspector.
EditorInspectorCategory *category = memnew(EditorInspectorCategory);
main_vbox->add_child(category);
category_vbox = nullptr; //reset
// Do not add an icon, do not change the current class (`doc_name`) for custom categories.
if (is_custom_category) {
category->label = p.name;
category->set_tooltip_text(p.name);
} else {
String type = p.name;
String label = p.name;
doc_name = p.name;
// Use category's owner script to update some of its information.
if (!EditorNode::get_editor_data().is_type_recognized(type) && ResourceLoader::exists(p.hint_string)) {
Ref<Script> scr = ResourceLoader::load(p.hint_string, "Script");
if (scr.is_valid()) {
StringName script_name = EditorNode::get_editor_data().script_class_get_name(scr->get_path());
// Update the docs reference and the label based on the script.
Vector<DocData::ClassDoc> docs = scr->get_documentation();
if (!docs.is_empty()) {
// The documentation of a GDScript's main class is at the end of the array.
// Hacky because this isn't necessarily always guaranteed.
doc_name = docs[docs.size() - 1].name;
}
if (script_name != StringName()) {
label = script_name;
}
// Find the icon corresponding to the script.
if (script_name != StringName()) {
category->icon = EditorNode::get_singleton()->get_class_icon(script_name);
} else {
category->icon = EditorNode::get_singleton()->get_object_icon(scr.ptr(), "Object");
}
}
}
if (category->icon.is_null() && !type.is_empty()) {
category->icon = EditorNode::get_singleton()->get_class_icon(type);
}
// Set the category label.
category->label = label;
category->doc_class_name = doc_name;
if (use_doc_hints) {
// `|` separators used in `EditorHelpBit`.
category->set_tooltip_text("class|" + doc_name + "|");
}
}
// Add editors at the start of a category.
for (Ref<EditorInspectorPlugin> &ped : valid_plugins) {
ped->parse_category(object, p.name);
_parse_added_editors(main_vbox, nullptr, ped);
}
continue;

We should always change doc_name, even if we are not displaying the category.

@YeldhamDev YeldhamDev force-pushed the extreme_corner_case_but_still branch from 3a2f38a to d7e9c83 Compare May 28, 2024 15:34
@YeldhamDev YeldhamDev changed the title Fix failure to get a property's tooltip description on rare cases Set doc_name even when categories are hidden in the inspector May 28, 2024
@YeldhamDev YeldhamDev requested a review from dalexeev May 28, 2024 15:36
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_help.cpp Outdated Show resolved Hide resolved
Otherwise, tooltips will fail to fetch descriptions.
@YeldhamDev YeldhamDev force-pushed the extreme_corner_case_but_still branch from d7e9c83 to 1bfcb6e Compare May 28, 2024 18:44
@akien-mga akien-mga merged commit 21810ca into godotengine:master May 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the extreme_corner_case_but_still branch May 29, 2024 12:44
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.

3 participants