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

Ensure special characters are escaped in TSCN connections and editable hint #86417

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

LimestaX
Copy link
Contributor

@LimestaX LimestaX commented Dec 22, 2023

Fixes issue regarding #86347

@LimestaX
Copy link
Contributor Author

LimestaX commented Dec 22, 2023

Originally was under #86348 , but I properly bricked my branch, this is a fresh build properly following guidelines, this is my first PR, and I tried using the web editor with Github, I didn't realize how bad that would make things, I couldn't properly rebase locally with the CLI nor VSCode.

@Calinou Calinou added this to the 4.3 milestone Dec 22, 2023
@Calinou Calinou added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 22, 2023
@AThousandShips AThousandShips requested a review from a team December 22, 2023 14:37
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 correct the commit message, it must start with a capital letter, see CONTRIBUTING.md.

@LimestaX LimestaX force-pushed the tscn-escape-char-fix branch from fc0e276 to 84fd03e Compare December 23, 2023 03:03
@LimestaX
Copy link
Contributor Author

All set! Thank you all for being patient.

@akien-mga

This comment was marked as outdated.

@akien-mga
Copy link
Member

I confirm the bug and the fix seems correct.

I skimmed through the file and found another occurrence that can have the same bug:

diff --git a/scene/resources/resource_format_text.cpp b/scene/resources/resource_format_text.cpp
index 29cd9f648d..b75b0280a0 100644
--- a/scene/resources/resource_format_text.cpp
+++ b/scene/resources/resource_format_text.cpp
@@ -2319,7 +2319,7 @@ Error ResourceFormatSaverTextInstance::save(const String &p_path, const Ref<Reso
 			if (i == 0) {
 				f->store_line("");
 			}
-			f->store_line("[editable path=\"" + editable_instances[i].operator String() + "\"]");
+			f->store_line("[editable path=\"" + editable_instances[i].operator String().c_escape() + "\"]");
 		}
 	}
 

Could you add it to this commit?

Overall I wonder if:

  • We should prevent using \ in Node names/paths
  • We should ensure NodePaths are always encoded with VariantWriter (i.e. as NodePath("Node\\"))

But both changes would be compat breaking, so this approach is the simplest solution for now.

@akien-mga akien-mga force-pushed the tscn-escape-char-fix branch from 84fd03e to 0a32c16 Compare January 8, 2024 10:42
@akien-mga akien-mga changed the title Ensure special characters are escaped in TSCN connections Ensure special characters are escaped in TSCN connections and editable hint Jan 8, 2024
@akien-mga
Copy link
Member

I amended the commit with the further fix I suggested, so it should be ready to merge.

@akien-mga akien-mga merged commit 48b726d into godotengine:master Jan 8, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

Escape characters not implemented on connections
4 participants