-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Enhance NodePath property editing #75274
Conversation
menu->connect(SNAME("about_to_popup"), callable_mp(this, &EditorPropertyNodePath::_update_menu)); | ||
hbc->add_child(menu); | ||
|
||
menu->get_popup()->add_item(TTR("Clear"), ACTION_CLEAR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: Why not take a reference/pointer to the popup and then popup->add_item(x)
? I'm seeing such pattern (repeating getters or whatever on multiple lines) quite often in Godot's code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk, I just haven't thought about that. It's not like it's done consistently.
I assume most people mainly need editing and clearing, so I'm wondering if we can spare the extra click so clearing a bunch of node paths doesn't take 4 times as long...
|
I'm not too sure about this but would it be possible to add them it to the menu that pops on right click and has "copy/paste value" instead of creating a new menu.
It feels like "copy / paste Value" should work like that instead of creating another functionality for it
I think this should replace the clear button. But maybe this is just me as i mostly never use it and use the revert to default button. |
Needs rebase. Then maybe @Calinou can review? |
Rebased. I ended up doing some refactoring, but I kept the behavior for now. |
I'd suggest supporting both if possible 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
f5696c3), it works as expected. Code looks good to me too.
A small UX issue I noticed is that when you use Show node in Tree, the focus will appear on the node in question (with the outline), but the old node remains selected:
I wonder if it should switch to the new node in the inspector automatically (and remove the selection on the current node instead). Still, this isn't blocking and can be refined in a future PR (like warning users if they enter a node path that isn't known to exist at compile-time).
I could experiment with selecting single node, but either it would switch the inspector (so the node you were editing right now would disappear) or, if it's possible to preserve edited node, the inspector would show old node with another one selected in scene tree. Maybe the latter is fine, same happens when you are dragging a node. |
Thanks! |
Code_k9ydA6CGlm.mp4
Closes #25289