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

Allow copying property path from inspector. #39404

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Jun 9, 2020

If you hover over a property label in the inspector dock and press
the "property_ediotr/copy_property_path" shortcut (ctrl+c by default),
the complete property path will be copied to the system clipboard. This
is especially useful for the AnimationTree, where code might reference
properties like "parameters/state/aim/move/blend_position".

Resolves godotengine/godot-proposals#106.

One issue is that if you click a property, then click on the node you
currently have selected in the node tree, then press ctrl+c, it will
still copy the selected property path rather than the node path. If you
click on a different node in the nodetree, however, ctrl+c will return
to copying the nodepath.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 9, 2020

There are 2 related PRs right now: #39398 and #34892

The reason I bring this up is that if the all 3 were going to be merged, the shortcut for your feature would need to changed to Ctrl + Shift + C for consistency (my scene tree PR changes Copy Node Path to this shortcut). But this is probably not important for now, as it's uncertain which of these PRs are going to make it 🤔

@rcorre
Copy link
Contributor Author

rcorre commented Jun 9, 2020

@KoBeWi it seems like your PR adds ctrl+c as an accelerator, not as a global keyboard shortcut. I just tried it out and it seems to work for me, as in ctrl+c will focus the menu item for copy, but does not automatically copy if the menu isn't open.

I'm thinking we should add two separate editor shortcuts, "property_editor/copy_property_path" and "property_editor/copy_property", so the user can create individual bindings for either, both separate from "copy_node_path".

@rcorre
Copy link
Contributor Author

rcorre commented Jun 9, 2020

(BTW I'm trying to combine the two efforts now)

@rcorre
Copy link
Contributor Author

rcorre commented Jun 9, 2020

NVM, I misread. Your interpretation of accelerators is correct.

@rcorre
Copy link
Contributor Author

rcorre commented Jun 9, 2020

@KoBeWi I think I combined the two in a way that makes sense. Now the popup menu has options for "copy property (ctrl+c)", "paste property (ctrl+v)", and "copy property path (ctrl+shift+c)". The keybindings work -- I followed the same pattern that seemed to be used for other popup menus with shortcuts, e.g. https://github.com/rcorre/godot/blob/d110aa421b04312e428baf93e97f40049e452381/editor/scene_tree_dock.cpp#L2467.

@rcorre
Copy link
Contributor Author

rcorre commented Jun 9, 2020

I'm getting these warnings though, which I haven't figured out, as InspectorDock seems to be constructed before InspectorProperty:

ERROR: Used ED_GET_SHORTCUT with invalid shortcut: property_editor/copy_property.
   at: ED_GET_SHORTCUT (editor/editor_settings.cpp:1514)
ERROR: Cannot add item with invalid ShortCut.
   at: add_shortcut (scene/gui/popup_menu.cpp:711)
ERROR: Used ED_GET_SHORTCUT with invalid shortcut: property_editor/paste_property.
   at: ED_GET_SHORTCUT (editor/editor_settings.cpp:1514)
ERROR: Cannot add item with invalid ShortCut.

Its also weird that it throws for those two, but not copy_property_path. The shortcuts seem to work in spite of the warnings.

@HEAVYPOLY
Copy link
Contributor

HEAVYPOLY commented Aug 10, 2020

Heads up this doesn't seem to be working on Mac with 3.2.3 rc3. Also the editor hotkey for copy property was blank by default. I tried setting to Command C and not getting any copy on theme parameters
image

@akien-mga
Copy link
Member

@HEAVYPOLY This hasn't been merged yet so it's not included in 3.2.3 RC 3. Or do you mean that you cherry-picked this PR manually and made your own build?

@rcorre rcorre force-pushed the proposal-106-copy-prop-path branch from d110aa4 to 6166f30 Compare August 11, 2020 11:57
@rcorre
Copy link
Contributor Author

rcorre commented Aug 11, 2020

I just backported to 3.2 here and it seems to work. I don't have a mac to test on though.

@HEAVYPOLY
Copy link
Contributor

@akien-mga Ah my mistake. Look forward to trying this in the next release!

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Needs rebase, but otherwise looks good.

EDIT:
Well, seems like there is issue with Ctrl+V changing pivot if compatible node is selected (e.g. Sprite). The inspector doesn't correctly grab focus. This wasn't important before, but now that we have this inspector-specific shortcut, this should be solved somehow. Not sure how though, it could be done in another PR.

EDIT2:
Also you need to squash the commits and update the commit message.

@rcorre rcorre force-pushed the proposal-106-copy-prop-path branch 2 times, most recently from 651f528 to 99e7124 Compare August 22, 2021 11:53
@rcorre
Copy link
Contributor Author

rcorre commented Aug 22, 2021

@KoBeWi I see what you're saying about the pivot shortcut, though it also conflicts with the existing copy/paste node shortcuts, so it seems like a more general problem that needs to be solved outside the context of this.

Is there a customizeable shortcut for changing the pivot? I couldn't find it (even typing ctrl+v into the editor shortcuts menu).

@KoBeWi
Copy link
Member

KoBeWi commented Aug 22, 2021

Is there a customizeable shortcut for changing the pivot? I couldn't find it (even typing ctrl+v into the editor shortcuts menu).

The shortcut is just V and it's not customizable. It wouldn't be a problem if clicking a property would focus the inspector.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Wait, actually there is one thing that needs to be changed. Inspector creates many EditorProperties at once and creating a menu for each of them is inefficient. They should be created on demand. See #50530 Just add a method _ensure_popup() and call it each time the popup is supposed to open.

Another thing which will be relevant after #51722 is that you need to block the popup and shortcuts when the property is readonly (by using is_read_only()).

@rcorre rcorre force-pushed the proposal-106-copy-prop-path branch 2 times, most recently from 02930b9 to 909628c Compare August 22, 2021 17:19
@rcorre
Copy link
Contributor Author

rcorre commented Aug 22, 2021

The popup is now created just-in-time. I'm assuming that if it is readonly, I should block the paste shortcut, but not the popup as a whole, since copy/copy_path should still be allowed, right?

editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Aug 22, 2021

You are right, only paste should be blocked.

btw I now noticed that shortcut checking in _unhandled_key_input() and switch cases in menu_option() are ordered differently than the enum. You should reorder the code.

@rcorre
Copy link
Contributor Author

rcorre commented Aug 22, 2021

Fixed the enum ordering and respected is_read_only:

menu

@rcorre rcorre force-pushed the proposal-106-copy-prop-path branch from 909628c to e4630a6 Compare August 22, 2021 20:39
Copy link
Member

@KoBeWi KoBeWi 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 now.

Will need update if #51983 gets merged first.

@YuriSizov
Copy link
Contributor

I don't see it mentioned in the OP, but from the last screenshot... does this implement godotengine/godot-proposals#3098?

@KoBeWi
Copy link
Member

KoBeWi commented Aug 22, 2021

Partially, Open in Help would be missing.

@YuriSizov
Copy link
Contributor

Yeah, and reset I guess? But since the proposal is based around the problem of copying the path, I think we can say that it closes it, and maybe do a follow-up PR that adds extra options.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 22, 2021

I wouldn't close the proposal, rather just leave a comment that it's halfway there.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 23, 2021

Aand #51983 was merged. _unhandled_key_input needs to be changed to virtual method instead of binding. You could keep the old code somewhere for 3.x backport though.

Resolves godotengine/godot-proposals#106.

Adds the following property menu options with default bindings:

- Copy Property (ctrl+c)
- Paste Property (ctrl+v)
- Copy Property Path (ctrl+shift+c)

If you hover over a property label in the inspector dock, you can copy
either the property value or the property path to the system clipboard
using the shortcuts above This is especially useful for the
`AnimationTree`, where code might reference properties like
"parameters/state/aim/move/blend_position".

One issue is that if you click a property, then click on the node you
currently have selected in the node tree, then press ctrl+shift+c, it
will still copy the selected property path rather than the node path. If
you click on a different node in the nodetree, however, ctrl+shift+c
will return to copying the nodepath.

The property value copy/paste was implemented by @KoBeWi at godotengine#39398 and
merged into this PR due to their similarity.
@rcorre rcorre force-pushed the proposal-106-copy-prop-path branch from e4630a6 to 0205fff Compare August 23, 2021 22:15
@YuriSizov YuriSizov requested a review from a team August 24, 2021 23:19
@KoBeWi KoBeWi merged commit f6f5e0f into godotengine:master Aug 26, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Aug 26, 2021

Thanks!

@akien-mga
Copy link
Member

This would require a dedicated PR for 3.x if a backport is wanted.

@rcorre
Copy link
Contributor Author

rcorre commented Sep 21, 2021

Pretty sure I still have the old 3.x branch around. I can work on a backport if we want one.

@Calinou
Copy link
Member

Calinou commented Nov 2, 2021

Pretty sure I still have the old 3.x branch around. I can work on a backport if we want one.

A backport to 3.x would be welcome (so that it can be merged for 3.5).

@KoBeWi KoBeWi removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 12, 2021
@Calinou
Copy link
Member

Calinou commented May 18, 2022

I can't find the 3.x version of this pull request. Can anyone link it here?

The feature does seem to be present in 3.5beta:

image

@KoBeWi
Copy link
Member

KoBeWi commented May 18, 2022

#54913

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.

Ability to copy parameters path from shaders and animation trees
6 participants