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

Show tooltips for res:// and uid:// strings in ScriptEditor #100803

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

Conversation

larspet
Copy link
Contributor

@larspet larspet commented Dec 25, 2024

Closes godotengine/godot-proposals#5027

Following the addition of tooltips (#91060), this PR adds tooltips for res:// and uid:// strings.

res_uid_tooltips.mp4

@larspet larspet requested a review from a team as a code owner December 25, 2024 14:32
@dalexeev dalexeev added this to the 4.4 milestone Dec 25, 2024
@dalexeev dalexeev self-requested a review December 25, 2024 20:13
@dalexeev
Copy link
Member

  1. This should work not only for uid:// but also for res://.
  2. For consistency with other tooltips, I suggest using the following format:
    • Format: Resource <file name>: <Type>
    • Example: Resource icon.png: CompressedTexture2D
    • Or maybe: Resource res://icon.png: CompressedTexture2D
    • Rationale: Documentation uses Pascal style (var name: Type) instead of C style (Type name).
    • Also, we can make the resource type links clickable. You can probably reuse SYMBOL_HINT_ASSIGNABLE.

@larspet
Copy link
Contributor Author

larspet commented Dec 25, 2024

  1. This should work not only for uid:// but also for res://.

Makes sense, I will add that.

2. For consistency with other tooltips, I suggest using the following format:

   * Format: `Resource <file name>: <Type>`
   * Example: `Resource icon.png: CompressedTexture2D`
   * Or maybe: `Resource res://icon.png: CompressedTexture2D`

Resource <file_name>: <Type> is probably good, but I would not want Resource <file_path>: <Type> because paths can get pretty long, which is why i first decided on <Type> <file_name> <file_path>.

I will try adding the full resource path to the tooltip body instead of having it in the title.

@larspet larspet changed the title Show tooltips for UID strings in ScriptEditor Show tooltips for res:// and uid:// strings in ScriptEditor Dec 26, 2024
@larspet
Copy link
Contributor Author

larspet commented Dec 26, 2024

I have now added support for res:// as well. Since you can link to more types of files with res:// I split everything into different types:

  • Resource
  • Directory
  • TextFile (as defined by the docks/filesystem/textfile_extensions editor setting)
  • File (files that are not visible in FileSystem, opens in File Manager instead)

Comment on lines 1109 to 1114
String doc_symbol;
ScriptLanguage::LookupResult result;
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, a cleaner control flow looks like this:

String doc_symbol;

if (p_symbol.begins_with("res://") || p_symbol.begins_with("uid://")) {
    doc_symbol = "resource||" + p_symbol;
} else {
    ScriptLanguage::LookupResult result;
    const String code_text = code_editor->get_text_editor()->get_text_with_cursor_char(p_row, p_column);
    const Error lc_error = script->get_language()->lookup_code(code_text, p_symbol, script->get_path(), base, result);
    if (lc_error != OK) {
        return;
    }

    switch (result.type) {
        // ...
    }
}

if (!doc_symbol.is_empty()) {
    EditorHelpBitTooltip::show_tooltip(code_editor->get_text_editor(), doc_symbol, String(), true);
}

Yes, this will require increasing the indentation level of the large switch, but I think it's ok in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the if to the top of the function with an early return instead, since there was nothing but a call to show_tooltip after the large switch. Definitely cleanest this way.

@@ -277,6 +277,7 @@ class EditorHelpBit : public VBoxContainer {
String value;
Vector<ArgumentData> arguments;
String qualifiers;
String resource_path;
Copy link
Member

Choose a reason for hiding this comment

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

I think we could avoid adding a new property to the structure by moving some of the code from _update_labels() to parse_symbol(). If I'm not mistaken, you can generate the help_data.description in parse_symbol().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I wanted to create the description in parse_symbol(), but couldn't get the links working without directly using push_meta() (I think I tried using [url] BBCode but it didn't work very well), so I had to do it in _update_labels() instead.

Comment on lines 3984 to 3994
} else if (p_select.begins_with("open-ext")) {
String path = ProjectSettings::get_singleton()->globalize_path(p_select.substr(9));
OS::get_singleton()->shell_show_in_file_manager(path, true);
} else if (p_select.begins_with("open")) {
if (help_data.doc_type.type == "PackedScene") {
EditorNode::get_singleton()->load_scene(p_select.substr(5));
} else {
EditorNode::get_singleton()->load_resource(p_select.substr(5));
}
} else if (p_select.begins_with("show")) {
FileSystemDock::get_singleton()->navigate_to_path(p_select.substr(5));
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest using a colon to more clearly denote a "pseudo-protocol" (like open-res:). Historically we use the symbols ($, #, @, ^), but there are also real protocols http: and https:.

Comment on lines 3883 to 3885
content->push_meta("open " + help_data.resource_path, RichTextLabel::META_UNDERLINE_ON_HOVER);
content->add_image(get_editor_theme_icon(SNAME("Load")));
content->add_text(" " + TTR("Open"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
content->push_meta("open " + help_data.resource_path, RichTextLabel::META_UNDERLINE_ON_HOVER);
content->add_image(get_editor_theme_icon(SNAME("Load")));
content->add_text(" " + TTR("Open"));
content->add_image(get_editor_theme_icon(SNAME("Load")));
content->add_text(nbsp + TTR("Open"));
content->push_meta("open " + help_data.resource_path, RichTextLabel::META_UNDERLINE_ON_HOVER);
  1. Use a non-breaking space.
  2. Move the icon outside the link so it is not underlined.

The same below two more times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it might look slightly strange that the icon gets underlined, I still think it's better this way since it gives you more "hit area" to click the link, and it mimics context menus more this way. It feels weird to not be able to click the icon. I did add the nbsp though.

@larspet
Copy link
Contributor Author

larspet commented Jan 4, 2025

I made an attempt to make the width of the tooltip match the content width, but couldn't get it working because of problems similar to #93040. I do think it would make these tooltips look much nicer (regular code-related tooltips are good as they are however), but I'll leave it for someone with more Control knowledge to hopefully implement in another PR.

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.

Show tooltip with resource path when hovering the uid in ScriptEditor
2 participants