Skip to content

Conversation

@MartinDelille
Copy link

This is my first PR to the project so don't hesitate to ask me for improvements or any kind of modification.

I was a little bit annoyed that opening a shader with the Quick open dialog didn't focus it in the editor, so I added a line to focus the editor when opening a shader.

I'm not sure all the case are handled so feel free to comment on that.

@JekSun97
Copy link
Contributor

I approve, this matches the behavior of the script editor.

@MartinDelille
Copy link
Author

I wondered how I could make this changed merged. Shall I add it to the next call agenda ?

@paddy-exe
Copy link
Contributor

@MartinDelille
First off congrats on your first PR!
Currently it's beta phase so there's a feature freeze. Your PR counts as a feature so it will have to wait until 4.4 stable is released.

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

Tested and works as expected. Since this is a new feature, this will have to wait until 4.5.

Not sure if there are any style nitpicks applicable here.

@MartinDelille
Copy link
Author

Thank you for the feedback @paddy-exe !

@paddy-exe paddy-exe modified the milestones: 4.x, 4.5 Feb 13, 2025
@akien-mga akien-mga modified the milestones: 4.5, 4.x Jun 12, 2025
@akien-mga akien-mga requested a review from kitbdev June 12, 2025 14:41
Copy link
Contributor

@kitbdev kitbdev left a comment

Choose a reason for hiding this comment

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

This PR currently only works for shaders that are already open (even if not switched to). If the shader hasn't been opened yet it won't get focused. It also doesn't work with ShaderIncludes.
Hovever, doing it for shaders that are closed (ideally in _switch_to_editor), would probably cause issues since it would take focus from the ScriptEditor when the editor is loaded. I'm not sure if that is easily solvable, so it's fine if we want to keep the current approach for now.

@MartinDelille
Copy link
Author

I moved the focus grab to _switch_to_editor so it handles ShaderIncludes. The focus is indeed taken when the editor is loaded but I don't know how I can prevent that. Does it look acceptable now ?

@Chaosus
Copy link
Member

Chaosus commented Nov 3, 2025

Does it look acceptable now ?

You need to squash the commits, if it's ready - see https://contributing.godotengine.org/en/latest/organization/pull_requests/creating_pull_requests.html#modifying-a-pull-request

@MartinDelille
Copy link
Author

Done: e761c71

@MartinDelille
Copy link
Author

Has I said there is a little side effect: when reponing a project with opened shader, the shader editor will be focused. I think this is can be acceptable. If not I would appreciate a hint to optionally do the focus at startup. I think I would need to add the p_focus parameter to the edit() method but since it is an overriden method from the EditorPlugin class, it might have implication elsewhere.

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.

7 participants