Update rbx-dom #269
Update rbx-dom #269
Conversation
There was a problem hiding this comment.
Overall good, we should wait until rbx-dom cuts releases and use those instead of git dependencies before merging this though.
I'm not sure if Filip would agree with breaking changes to various methods and functions, but anywhere where impl AsRef<str> is used for a property or class name, we might prefer impl Into<Ustr> instead.
| @@ -75,7 +75,7 @@ impl Instance { | |||
|
|
|||
| Some(Self { | |||
| dom_ref, | |||
| class_name: instance.class.clone(), | |||
| class_name: instance.class.to_string(), | |||
There was a problem hiding this comment.
It's probably worth changing the type of Instance.class_name to Ustr
| pub fn get_property(&self, name: impl AsRef<str>) -> Option<DomValue> { | ||
| pub fn get_property(&self, name: &str) -> Option<DomValue> { | ||
| INTERNAL_DOM | ||
| .lock() | ||
| .expect("Failed to lock document") | ||
| .get_by_ref(self.dom_ref) | ||
| .expect("Failed to find instance in document") | ||
| .properties | ||
| .get(name.as_ref()) | ||
| .get(&name.into()) |
There was a problem hiding this comment.
If we don't want to change the type of name param to Ustr, then we can leave this method signature alone, and do &ustr(name.as_ref()) for the property get
| inst.properties.get(PROPERTY_NAME_ATTRIBUTES) | ||
| inst.properties.get(&PROPERTY_NAME_ATTRIBUTES.into()) |
There was a problem hiding this comment.
It might be more readable to do &ustr(PROPERTY_NAME_ATTRIBUTES) here, likewise for the other usages, and for usages of PROPERTY_NAME_TAGS as well
| @@ -41,7 +41,11 @@ where | |||
| } | |||
|
|
|||
| fn remove_matching_prop(inst: &mut DomInstance, ty: DomType, name: &'static str) { | |||
There was a problem hiding this comment.
Should probably change the type of the name param to Ustr
| if class_is_a(&inst.class, "LuaSourceContainer").unwrap_or(false) { | ||
| inst.properties.remove("ScriptGuid"); | ||
| if class_is_a(inst.class, "LuaSourceContainer").unwrap_or(false) { | ||
| inst.properties.remove(&"ScriptGuid".into()); |
There was a problem hiding this comment.
| inst.properties.remove(&"ScriptGuid".into()); | |
| inst.properties.remove(&ustr("ScriptGuid")); |
Updates with the breaking changes from the latest commits on master. Still a WIP.