Add Shortcut tooltip to Editor Main Screen Plugins

[kushagra10025]

Fixes godotengine/godot-proposals#6266

Added tool-tip to 2D, 3D, Script, AssetLib (Main Screen Editor Plugins) buttons that shows keyboard shortcut.

• Open 2D Editor (Ctrl + F1)
• Open 3D Editor (Ctrl + F2)
• Open Script Editor (Ctrl + F3)
• Open Game View (Ctrl + F4)
• Open Asset Library (Ctrl + F5)

Video of the Change in action:

after-main-screen-plugin.mp4

Note: The easiest solution I figured was reordering the Shortcut Creation and directly getting its reference in add_main_plugin
Hopefully doesn't break any compatibility.

[KoBeWi]

Does not work with custom plugins.

[kushagra10025]

fair enough. will handle situation where if shortcut doesn't exist will create a dummy shortcut field in Editor Settings for that given plugin name. Ideally (as you stated in the follow-up comment) we indeed should have a defined api for handling shortcut (main screen and editor plugins in general)

[KoBeWi]

You can't just assume that any added main screen will have associated shortcut (and with half-hard-coded path). You could add a method to EditorMainScreen that assigns a Shortcut to a main screen Button based on its name.

Ideally we should have some API that allows defining a shortcut for a main screen plugin, but this is out of scope for this PR.

[kushagra10025]

Have handled the scenario for Custom Plugins - Though due to how ED_GET_SHORTCUT works, there will be an error like INTERNAL ERROR: Used ED_GET_SHORTCUT with invalid shortcut: editor/editor_XXXX for every custom plugin added - For the first time. Once the Shortcut path is created error won't popup again, and users can setup custom path - Can serve as a reminder to add shortcut to custom plugins.

Here's the new change in action (with two custom plugins - one enabled by default and one to be manually enabled)
PS: Error apart from INTERNAL ERROR are due to incompatible LimboAI version, kindly ignore (unless otherwise :) )

after-main-screen-plugin-custom-handled.mp4

[KoBeWi]

Have handled the scenario for Custom Plugins - Though due to how ED_GET_SHORTCUT works, there will be an error like INTERNAL ERROR: Used ED_GET_SHORTCUT with invalid shortcut: editor/editor_XXXX for every custom plugin added - For the first time. Once the Shortcut path is created error won't popup again, and users can setup custom path - Can serve as a reminder to add shortcut to custom plugins.

Uh, that will just cause errors for every main screen plugin and people will make bug reports. Also forcing a fixed path for plugin can easily cause other problems, like name clashes.

If you want to go this way, just check if the shortcut exists before trying to assign it. If it does not exist, don't do anything, don't add a dummy shortcut.

[kushagra10025]

Thanks for pointing these out.

Uh, that will just cause errors for every main screen plugin and people will make bug reports.
Yes to this, any new custom main screen plugin is added, it will throw an error - unless the shortcut path creation for that plugin is handled by the plugin itself - but then a way would be needed to ensure the shortcut path follows the string comparision format.

Also forcing a fixed path for plugin can easily cause other problems, like name clashes.
Had thought of this, but I didn't have a alternative to this, if two plugin names are same then that becomes an issue - is there any way to ensure that no two editorplugins have clashing names?

If it does not exist, don't do anything, don't add a dummy shortcut.
This would be the simplest way ahead, but wouldn't it cause inconsistency where the Core Main Screen plugins have a shortcut and its tooltip and the custom ones don't. If this is fine, I can remove the new shortcut path creation code and raise it again.

[KoBeWi]

This would be the simplest way ahead, but wouldn't it cause inconsistency where the Core Main Screen plugins have a shortcut and its tooltip and the custom ones don't.

Well, that's just missing functionality. I doubt most people will even notice it.
Also core main screen plugins had a shortcut for a long time already, so it's not that big change.

[kushagra10025]

ah alright, in that case will update the code to reflect this.

but here's a concern I've - this is what the updated code block would look like

Ref<Shortcut> shortcut = ED_GET_SHORTCUT(String("editor/editor_{0}").format(varray(p_editor->get_plugin_name().to_lower())));
if (!shortcut.is_null() && shortcut.is_valid()) {
	tb->set_shortcut(shortcut);
}

based on how the code is structured, it would still look for custom plugin shortcut_paths, say editor/editor_limboai and find it missing and will eventually throw the INTERNAL ERROR due to ED_GET_SHORTCUT

alternative to this would be handling the get_shortcut check with EditorSettings::get_singleton()->get_shortcut(p_path);
but is it recommended and if yes, can the previous approach of creating and exposing a custom plugins shortcut path viable - obviously with a message to the user in console that a shortcut is available, if they want to use it.

[AThousandShips]

No need to do !shortcut.is_null() && shortcut.is_valid() it's just the same as shortcut.is_valid()

[kushagra10025]

updated it locally for time-being, thanks :)

[KoBeWi]

alternative to this would be handling the get_shortcut check with EditorSettings::get_singleton()->get_shortcut(p_path);

That would be fine in this case.

can the previous approach of creating and exposing a custom plugins shortcut path viable - obviously with a message to the user in console that a shortcut is available, if they want to use it.

Not really. Until we have a proper API for main screen buttons (and there is a couple of ways to implement it), it's better to not mention this option anywhere, as it's basically a hack/workaround.

[kushagra10025]

cool then. updating it accordingly.

[kushagra10025]

not related to changes:
huge thanks @KoBeWi and @AThousandShips for bearing with me on my adventures of effectively changing two lines.

also - my bad as I missed to force-push the initial two commits.

[kitbdev]

Won't this duplicate the shortcut handling?
The shortcuts are already handled here:

godot/editor/editor_node.cpp

Lines 402 to 409 in e67074d

	} else if (ED_IS_SHORTCUT("editor/editor_2d", p_event)) {
	editor_main_screen->select(EditorMainScreen::EDITOR_2D);
	} else if (ED_IS_SHORTCUT("editor/editor_3d", p_event)) {
	editor_main_screen->select(EditorMainScreen::EDITOR_3D);
	} else if (ED_IS_SHORTCUT("editor/editor_script", p_event)) {
	editor_main_screen->select(EditorMainScreen::EDITOR_SCRIPT);
	} else if (ED_IS_SHORTCUT("editor/editor_game", p_event)) {
	editor_main_screen->select(EditorMainScreen::EDITOR_GAME);

So the old ones should be deleted as this changes it to be handled by the buttons.

[KoBeWi]

I'm not sure if the Buttons will handle the shortcut properly. And either way, the shortcut can be matched only once, so it won't be a problem.

EDIT:
I tested it and the Buttons do seem to handle the shortcuts, but the shortcut_input quoted above takes priority. Only one of them is activated as expected.

[kitbdev]

In that case, they can be deleted later when a proper API is made for it.

[allenwp]

I believe the following text should be used for these shortcuts:

• Open 2D Workspace (Ctrl + F1)
• Open 3D Workspace (Ctrl + F2)
• Open Script Editor (Ctrl + F3)
• Open Game View (Ctrl + F4) (or maybe something else? Game Screen?)
• Open Asset Library (Ctrl + F5)

The documentation is a bit inconsistent and should maybe be cleaned up, but I don't believe that existing documentation calls the 2D or 3D workspaces "editors". It's probably best to follow the existing names in the documentation, as those can terms can be assumed to be used in some external tutorials and resources.

"Workspace" term is used to describe what these buttons do: https://docs.godotengine.org/en/latest/getting_started/introduction/first_look_at_the_editor.html#first-look-at-godot-s-editor

"Screens" term is used to describe the 5 screens: https://docs.godotengine.org/en/latest/getting_started/introduction/first_look_at_the_editor.html#the-five-main-screens (It seems the "screens" term is a bit of an outlier and is only used in this one section of this one page?)

"Workspace" term is used for the 2D workspace: https://docs.godotengine.org/en/latest/tutorials/2d/introduction_to_2d.html#d-workspace

"Workspace" term is used for the 3D workspace: https://docs.godotengine.org/en/latest/tutorials/3d/introduction_to_3d.html#d-workspace

"Editor" term is used for the script editor: https://docs.godotengine.org/en/latest/tutorials/editor/script_editor.html

I'm actually not sure about the Game View; I can't find any documentation about it besides the overview of the "five screens", where it's called a Game Screen. But again, this "screen" term appears to an outlier, so I'm fine with whatever for this one.

[Repiteo]

@kushagra10025 Would you be able to integrate the new terms suggested above?

[kushagra10025]

@Repiteo if the above suggestions are alright then I'll update the PR with them soon.

[Repiteo]

@kushagra10025 I have no objections & nobody has opposed them, so go for it! The commits need to be squashed anyway, so this will be a good opportunity to handle that all at once

[kushagra10025]

Hey @Repiteo - Have updated the PR, I might have messed up (squashing too) and pushed after fetching latest commit 🥲. Didn't have any merge conflicts though - atleast locally!

Also @AThousandShips - in case this is merged wouldn't we need to update the docs here too - https://docs.godotengine.org/en/stable/tutorials/editor/default_key_mapping.html#general-editor-actions
I tried the doctool but it didn't update the XML file or similar, from what I understood from the docs. Is another PR required on godot-docs repo to update this.

Apologies for any inconvenience! Do guide with next steps please :)

[AThousandShips]

The page on the docs would need to be manually updated indeed, we (currently) don't have an automated system

[kushagra10025]

Alright! Based on the acceptance result of this PR! I'll raise one on that repo. Thanks for confirming!

[Repiteo]

Fixed up the Git history for you @kushagra10025, so this is now ready to merge 👍

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉

[kushagra10025]

Thanks to all involved in the thread!