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

While the Script Editor Plugin is active, makes it optional to switch Editor Plugins when selecting Nodes in the Scene Tree #63344

Conversation

alfredbaudisch
Copy link
Contributor

@alfredbaudisch alfredbaudisch commented Jul 23, 2022

Problem

  • While the Script editor is active, selecting a Node in the Scene Tree in order to inspect it, makes the editor_plugin_screen to be changed to the Node's main plugin.
  • The same happens when right-clicking a Node
  • This makes it hard, and tiresome to inspect and contextualize Nodes while writing Scripts, because it forcibly causes unintentional repetitive viewport and editor switching when interacting with Nodes while in the Script Editor.
  • It makes it even more cumbersome when dealing with long UI-centric Scene Trees, where it's necessary to gather Node Paths and adjust Inspector properties
  • The current behaviour is more than an annoyance than a desired effect - I never faced any use case where I wanted to switch viewports when clicking a Node while in the Script Editor - you end up FORCIBLY SWITCHING CONTEXTS back hundreds of times in a single day of work. If I want to switch to the 2D or 3D viewports I can clearly click their toolbar buttons or use the switch hotkeys.
  • It also makes the process of dragging and dropping Nodes into the Text Editor repetitive and tiresome, requiring 4 clicks: (1) click a Node, it switches to the 2D/3D viewport, (2) click to go back to Script, (3) click the Node again, (4) drag and drop it into the Text Editor to get the Node Path.
  • Examples are reported in this blog post, under "Inconsistent Viewport and Script Editor tab switching when clicking Nodes (2D/3D)".
GodotSwitchjingAnnoying.mp4

Solution

  • Make it optional to switch Editor Plugins when selecting Nodes in the Scene tree while the current active Editor Plugin is the Script Text Editor.
  • It works both with the Left Click (PRIMARY EFFECT: to access the Node's Inspector) and with Right Click (SECONDARY EFFECT: to access the Node's Context Menu), both more than common and repetitive behaviors needed while writing Scripts.
  • The feature is toggeable via text_editor/behavior/navigation/node_select_toggles_inspector_only.
GodotSweitchSolved.mp4

Is it really needed?

@mhilbrunner
Copy link
Member

Please refrain from adding comments that don't add to the discussion of this change. If you just want to express your support for this change, use the reaction on the PR itself for that. Thank you!

(Comments add some noise for maintainers who then see the PR has received an update; some also get an email about it. It also makes discussion harder to follow in longer PR comment chains.)

@samdze
Copy link
Contributor

samdze commented Jul 23, 2022

I don't know if I like this solution, feels not optimal to me.
I'd better like a unified solution that works best almost all the time since the default behaviour is still preferred occasionally.

What about the switching only happening if an already selected node gets clicked again? (click down + release, to allow dragging it without a switch)

@Calinou
Copy link
Member

Calinou commented Jul 23, 2022

Note that this is already available when holding Alt while clicking in the scene tree dock. Unfortunately, most window managers on Linux will intercept Alt + clicking by default, so Godot will never see those clicks.

@Calinou Calinou added this to the 4.0 milestone Jul 23, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Jul 23, 2022

The Alt was a hack that got removed in #61162

I think the proper solution would be extending #61162 to handle script editor properly (i.e. only switch editor when not dragging the node).

@alfredbaudisch
Copy link
Contributor Author

alfredbaudisch commented Jul 24, 2022

the default behaviour is still preferred occasionally.

Isn't the default behaviour more of a legacy leftover than actually being useful? Since, if you are in the Script editor, the intended behaviour when navigating Nodes is to inspect and/or contextualize Nodes. If you are to switch to the 2D/3D contexts, it's clear that they can be accessed through their corresponding toolbar buttons and hotkeys.

(i.e. only switch editor when not dragging the node).

If it requires you to drag the node to not switch the editor, how would you inspect the Nodes in the Inspector and access their context menu, while staying in the Script editor?

Wouldn't that require you to actively loosely drag Nodes and let them go, without the intention of actually dragging, but just because you want to access the Inspector for that Node?

Dragging and dropping to get path nodes is just a secondary benefit from this, the two biggest pain points to be fixed are getting distraction-free and hassle-free access to Inspecting any Node and access to the context menu of any Node, while staying in the Script Editor, without toggling a context switching.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 24, 2022

Ok it makes sense to prevent switching on node click.

Isn't the default behaviour more of a legacy leftover than actually being useful?

idk I sometimes use it, but I agree that more often it's annoying and not switching is more useful.

What about the switching only happening if an already selected node gets clicked again?

Or maybe we could switch when Alt is held or something.

But editor setting is fine too probably; it should be enabled by default though. Also the name could be better, like stay_in_script_editor_on_node_selected (up to discussion).

editor/editor_node.cpp Outdated Show resolved Hide resolved
@8to16apps
Copy link

Hi all, first comment event in github.

I would like introduce another posible solution.
A pin icon in the script tool bar, after File menu o next to rigth buttons,
so you can pin to make script visible even if change the node or unpin to have the current behavior.
This make this function more visible and move easy to switch on/off

@reduz
Copy link
Member

reduz commented Jul 24, 2022

I agree with@8to16apps and also think a pin icon in the script toolbar is definitely a cleaner solution. Very discoverable and easy to toggle on and off.

@alfredbaudisch
Copy link
Contributor Author

@reduz @8to16apps I'm not sure I understand where exactly it should be. A mockup would be welcome.

@8to16apps
Copy link

@alfredbaudisch @reduz I am thinking in something like this. This is the same concept that the animation tap in the bottom.

mock_up

@alfredbaudisch
Copy link
Contributor Author

@8to16apps Good point. Doing this way would keep it consistent with the pin behaviour of the AnimationPlayer.

image

@KoBeWi
Copy link
Member

KoBeWi commented Jul 24, 2022

btw the pin button can simply toggle the editor setting, so you can keep your current changes.

@alfredbaudisch
Copy link
Contributor Author

btw the pin button can simply toggle the editor setting, so you can keep your current changes.

You're fast as I was about to comment: my implementation wouldn't change.
The Pin would simply be a shortcut to toggle my new setting.

@reduz
Copy link
Member

reduz commented Jul 24, 2022

We put pins on the right side, so I guess I would put it on the right side here too.

@alfredbaudisch alfredbaudisch requested a review from a team as a code owner July 24, 2022 12:03
@alfredbaudisch
Copy link
Contributor Author

alfredbaudisch commented Jul 24, 2022

@reduz @8to16apps Done, thanks for the suggestion. This makes a lot of sense.

GodotSwitchScriptPin.mp4

editor/editor_node.cpp Outdated Show resolved Hide resolved
@YeldhamDev
Copy link
Member

YeldhamDev commented Jul 24, 2022

The commits need to be squashed before the PR can be merged. You can follow the instructions for that here: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

You can use git commit --amend in the subsequent commits to avoid this process in the future.

@alfredbaudisch alfredbaudisch force-pushed the feature-select-node-toggle-inspector-only branch from d10ee6c to 238c790 Compare July 24, 2022 13:27
@alfredbaudisch
Copy link
Contributor Author

alfredbaudisch commented Jul 24, 2022

@YeldhamDev Done

@YuriSizov
Copy link
Contributor

The pin feels like a lot of micromanagement for something that should probably work one way out of the box. Like Emi mentioned on Twitter, I'd probably get annoyed by the pin button wasting space and remove it with a plugin. I don't believe it's something you need to constantly change to justify having it so visible.

@alfredbaudisch
Copy link
Contributor Author

alfredbaudisch commented Jul 24, 2022

The pin feels like a lot of micromanagement for something that should probably work one way out of the box. Like Emi mentioned on Twitter, I'd probably get annoyed by the pin button wasting space and remove it with a plugin. I don't believe it's something you need to constantly change to justify having it so visible.

I'm neutral about it actually, since the problem is fixed with or without the pin. So how things like this are normally decided? @YuriSizov

@YuriSizov
Copy link
Contributor

So how things like this are normally decided?

I guess we'll discuss it during the next PR review meeting. You are welcome to join, it's on Tuesday (watch out in #devel channel on RocketChat).

…s "Script" and if text_editor/behavior/navigation/stay_in_script_editor_on_node_selected is true, force inspector_only in order to not switch the EditorPlugin to the Node's main plugin.
@alfredbaudisch alfredbaudisch force-pushed the feature-select-node-toggle-inspector-only branch from 238c790 to c4433c3 Compare July 25, 2022 15:37
@alfredbaudisch
Copy link
Contributor Author

The pin has been removed in favor of keeping the first basic implementation of just the Editor setting (on by default), as per some Twitter conversation.

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.

Looks good to me.

Not 100% convinced by the setting name but this could be improved later on.

@akien-mga akien-mga merged commit 1d9e1ac into godotengine:master Jul 26, 2022
@akien-mga
Copy link
Member

Thanks!

@Anutrix
Copy link
Contributor

Anutrix commented Jul 26, 2022

Can this be cherry-picked to 3.x branch?

@KoBeWi KoBeWi added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 26, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 26, 2022

Cherry-picked for 3.5.


@alfredbaudisch Just noticed it but for future commits, please try to keep the commit message title (first line) at a reasonable length. It should typically be less than 72 chars, the current one is 255 (i.e. your commit title is a full description instead of a title).
It's also describing the diff pretty much instead of describing the changed functionality. Something like this would typically be clearer:

Script Editor: Don't switch to 2D/3D viewports when selecting nodes

Selecting nodes in the Scene dock automatically switches to the relevant 2D
or 3D viewport. This behavior can be annoying while using the Script Editor
and wanting to inspect node properties, so it's now disabled by default when
the Script Editor is active.

This new behavior can be changed back to the previous auto-switching using
the `text_editor/behavior/navigation/stay_in_script_editor_on_node_selected`
editor setting.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 26, 2022
@alfredbaudisch
Copy link
Contributor Author

@akien-mga Thanks Rémi for fixing the commit message and for the tips! I'll keep those conventions in mind for possible future contributions.

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.

None yet