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

Added Rename Context Menu Option for SceneTreeDock #18786

Conversation

Eoin-ONeill-Yokai
Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai commented May 11, 2018

@atari-8bit pointed this issue out in the issue tracker ( #18560 ) and I agree that it is strange to not provide rename functionality in the context menu. This pull request adds that feature and also changes the organization or right click elements. I believe that Batch Rename (while useful + cool!) won't be used so much that it warrants being a top-level element in the context menu. I've moved all rename tools near the reload tool.

Rename should work when a single element is selected.

Bugsquad edit: Fixes #18560.

}
menu->add_icon_shortcut(get_icon("Rename", "EditorIcons"), ED_GET_SHORTCUT("scene_tree/batch_rename"), TOOL_BATCH_RENAME);
Copy link
Member

Choose a reason for hiding this comment

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

It should probably be moved to an else, otherwise we'll get two "Rename" options for single selections.

Copy link
Member

Choose a reason for hiding this comment

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

Or is Batch Rename useful for single selections too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey I commented down below, but to elaborate (sorry, not used to Github code review)

On batch rename, I'm not entirely sure there's any point to use it on a single node since I'm not the author of that functionality. I merely kept it outside of an else as that's how it was when I looked at the code.

Copy link
Member

Choose a reason for hiding this comment

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

I've had a look and Batch Rename is not useful for a single node, so you can put it in an else branch IMO.

@akien-mga
Copy link
Member

Unrelated to this PR, but note that the email you use to sign your commits (see header of https://github.com/godotengine/godot/commit/71211e7e2222f43b66138b68d794a3063e254a4a.patch ) is not linked to your GitHub account, so your commits don't appear as authored by "@THEYOKAI". It's not an issue per se, but if you want to fix it, you can add this email as secondary email in your GitHub profile.

@Eoin-ONeill-Yokai
Copy link
Contributor Author

Eoin-ONeill-Yokai commented May 11, 2018

Thanks @akien-mga, I'll fix that email issue. I use a different email for github and other git sites so I just need to manage that better.

Also, on batch rename, I'm not entirely sure there's any point to use it on a single node since I'm not the author of that functionality. I merely kept it outside of an else as that's how it was when I looked at the code.

@akien-mga
Copy link
Member

Merged manually as 1378ca1 with the else branch I wanted.

@reduz
Copy link
Member

reduz commented May 17, 2018

This is OK, but usability is terrible, an action not commonly accessed that should only appear when more than one node is selected should not be on top of the popup. I just changed this on my local branch so it appears more towards the bottom and only when needed.

@akien-mga
Copy link
Member

@reduz It's no longer on top of the popup since this PR was merged, so it's better if you don't change it and pull --rebase the master branch instead ;)

@Eoin-ONeill-Yokai
Copy link
Contributor Author

Eoin-ONeill-Yokai commented May 20, 2018

@reduz I agree about the batch rename tool, but this patch wasn't actually the one that introduced batch rename ( #15928 ). All I did was add a regular "rename" selection and moved batch rename a bit lower.

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.

3 participants