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

Stopped canvas/spatial editor from becoming active when clicking a node in the scene tree if script editor is currently active. Can use alt + click to switch editor. #40438

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Jul 16, 2020

Closes #39539, Closes #14862, Closes #33704

If the script editor is open, clicking on a node in the scene tree no longer switches you to the canvas item or spatial editors. The inspector is updated so you can change properties, or drag and drop the node path onto the script easily.

If you want to switch to the other editor, simply hold down the Alt key.

QBocbLiUOt

@EricEzaM EricEzaM changed the title Clicking on a node in the scene tree does not switch to canvas/spatial editor if the script editor is currently active Stopped canvas/spatial editor from becoming active when clicking a node in the scene tree if script editor is currently active. Can use alt + click to switch editor. Jul 16, 2020
@Calinou Calinou added this to the 4.0 milestone Jul 16, 2020
@Calinou Calinou added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 16, 2020
editor/editor_node.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga requested a review from groud July 26, 2020 14:48
@groud
Copy link
Member

groud commented Aug 25, 2020

In the first issue you mention, @KoBeWi talks about a can_lose_focus_on_node_selection() function. Shouldn't this simply return false ? I haven't checked the code but I guess that instead of having an hardcoded check it would be a cleaner solution no ?

@EricEzaM
Copy link
Contributor Author

@groud can_lose_focus_on_selection() is a method in ScriptEditorBase, not on EditorPlugin, so a cast would need to be made to ScriptEditorPlugin anyway.

Also, currently there is no way to access ScriptEditorBase from a public API in ScriptEditorPlugin, so a few changes would need to be made to expose this.

@groud
Copy link
Member

groud commented Aug 26, 2020

I am not really sure I understand. I might be wrong but I assumed the returned value of this function causes the script editor to loose focus, and that, maybe, it should return false instead of true (so you would have to modify it directly in script_editor_plugin.h). If this is enough to solve the problem, you won't need any cast or exposing something.

@EricEzaM
Copy link
Contributor Author

Derp... sorry, I understand now. I didn't properly understand what you meant earlier. Yes, changing the method return to false resolves the issue in a much more simple way. Thanks.

@EricEzaM EricEzaM force-pushed the keep-script-editor-active-when-clicking-on-scene-tree-dock-nodes branch from d8f476f to 6195e71 Compare August 26, 2020 08:21
@groud
Copy link
Member

groud commented Aug 26, 2020

Derp... sorry, I understand now. I didn't properly understand what you meant earlier. Yes, changing the method return to false resolves the issue in a much more simple way. Thanks.

Oh cool. :)

Just to mention, it might be worth checking the usage of this function though. Those are all usages I see:

modules/visual_script/visual_script_editor.h:323:	virtual bool can_lose_focus_on_node_selection() override { return false; }
editor/plugins/script_editor_plugin.h:158:	virtual bool can_lose_focus_on_node_selection() { return true; }
editor/plugins/script_editor_plugin.cpp:1540:		return current->can_lose_focus_on_node_selection();
editor/plugins/text_editor.h:135:	virtual bool can_lose_focus_on_node_selection() override { return true; }

as we can see, the only place where it would return true now is going to be in text_editor.h, but I am not sure it should be kept returning true. If it should be returning false too, the can_lose_focus_on_node_selection function could be removed completely and we could make can_take_away_focus always return false.

@EricEzaM
Copy link
Contributor Author

Yeah I did notice that. Also can_take_away_focus() also only has one call... the one right here in editor_node.cpp. It is not an API exposed to scripting so... I guess there is no harm in removing it.

…l editor if the script editor is currently active
@EricEzaM EricEzaM force-pushed the keep-script-editor-active-when-clicking-on-scene-tree-dock-nodes branch from 6195e71 to 3653a7d Compare August 26, 2020 09:02
@EricEzaM
Copy link
Contributor Author

Ok, all done. Clicking node does not switch by default. User can alt + click a node to switch to the canvas item/spatial editor. This will need to be documented.

@aaronfranke
Copy link
Member

@EricEzaM Is this still desired? If so, it needs to be rebased and tested on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 10, 2021

I'd remove can_take_away_focus() too. It might be "nice to have" for potential future use, but the reality is it will just end up as dead useless code (and re-adding it if necessary isn't difficult). Alternatively, you could move the KEY_ALT check to be done by this method.

btw this part of condition (ScriptEditor::get_singleton()->can_take_away_focus() || Input::get_singleton()->is_key_pressed(KEY_ALT)) doesn't need brackets.

@akien-mga
Copy link
Member

Superseded by #61162.

@akien-mga akien-mga closed this May 23, 2022
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants