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

Correctly set up shortcut context in the shader editor #84614

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Nov 8, 2023

Fixes the main bug in #84601 (the discussion should still continue about the usability, but this resolves the bug that gets in the way of saving scenes).

@YuriSizov YuriSizov added this to the 4.2 milestone Nov 8, 2023
@YuriSizov YuriSizov requested a review from KoBeWi November 8, 2023 12:08
@YuriSizov YuriSizov requested a review from a team as a code owner November 8, 2023 12:08
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested locally, I confirm this fixes the inability to save a scene while the shader editor is open.

Now it behaves as I would expect it (even if it's subpar UX) where the shortcut depends on the focus context, so Ctrl+S in the shader editor saves the shader, and if you focus another editor (scene tree, viewport, inspector, etc.) it will fall back to the main Save Scene function.

This also fixes an unreported bug where a floating shader editor's Ctrl+S would actually always save the scene instead of the script, which is the opposite of the behavior we want in a floating window where the focus is in the window.

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Nov 8, 2023

Thanks! My bad, I did not know I need to call set_shortcut_context and thus missed it.

@akien-mga akien-mga merged commit 57a0ac2 into godotengine:master Nov 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the editor-set-shader-shortcut-context branch November 9, 2023 10:18
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 participants