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

Improve user experience for VCS metadata generator menu button to mitigate accidentally overriding #88609

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

Cass-dev-web
Copy link
Contributor

Simply added a check to see if Git is already initialized, and if so, it would change the user interface to display Override Version Control Metadata... instead of Create Version Control Metadata... to avoid confusion (just a polish, low priority).

Please review the code and provide feedback as I am new to push requests.

@Cass-dev-web
Copy link
Contributor Author

Cass-dev-web commented Feb 20, 2024

Commit 6416791: Added a wacky hack which checks that the class name does not conflict with Editor as it would cause #88601 to happen.

Needs work & needs to be tested for non-node extending scripts.

P.S.: I'm unsure if this needs to be a separate pull request, and if that's possible with one already pending approval

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@Cass-dev-web
Copy link
Contributor Author

Updated

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips changed the title Improved user experience for VCS metadata generator menu button to mitigate accidentally overriding Improve user experience for VCS metadata generator menu button to mitigate accidentally overriding Feb 21, 2024
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

This works, but the text is updated only after restarting the editor. This can be solved by checking the file on the EditorFileSystem.filesystem_changed signal or every time the popup is shown. In my opinion, this is an overkill, replacing "Create" with "Create/Override" or "Setup" will work too.

Also, please squash commits into one. To do this you need a local git client, GitHub web interface doesn't support it.

@Cass-dev-web
Copy link
Contributor Author

That's a better way of doing it, seems like it was a bit too ambitious in terms of performance, although it did lead to me learning new things, so this entire PR wasn't in vain :)

@Cass-dev-web
Copy link
Contributor Author

Cass-dev-web commented Feb 22, 2024

Also, please squash commits into one.

Squashed.

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

@dalexeev dalexeev 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. Please squash commits one more time (note that you can do this locally before force-push to save CI resources).

@Cass-dev-web
Copy link
Contributor Author

Squashed (not sure if I did it the right way however 🤔 )

@akien-mga
Copy link
Member

You squashed it correctly, but adding "(squashed x2)" in the commit message was unnecessary. Once merged, it shouldn't matter to a reader whether that commit is the result of squashing multiple earlier commits or not, what matters is what the commit does :)

If you can amend the commit with git commit --amend to remove that part, it should be perfect. But if not it's not a big deal, we can also merge as is.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 22, 2024
@akien-mga akien-mga merged commit 15bb860 into godotengine:master Feb 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants