Finish off bulk of metadata tracking in snapshot system

src/commands/build.rs

@@ -121,11 +121,8 @@ pub fn build(options: &BuildOptions) -> Result<(), BuildError> {
             // Place files don't contain an entry for the DataModel, but our
             // RbxTree representation does.
 
-            let top_level_ids = tree
+            let root_instance = tree.get_instance(root_id).unwrap();
-                .get_instance(root_id)
+            let top_level_ids = root_instance.children();
-                .unwrap()
-                .instance
-                .get_children_ids();
 
             rbx_xml::to_writer(&mut file, tree.inner(), top_level_ids, xml_encode_config())?;
         }
@@ -137,11 +134,8 @@ pub fn build(options: &BuildOptions) -> Result<(), BuildError> {
             log::warn!("Using the XML place format (rbxlx) is recommended instead.");
             log::warn!("For more info, see https://github.com/LPGhatguy/rojo/issues/180");
 
-            let top_level_ids = tree
+            let root_instance = tree.get_instance(root_id).unwrap();
-                .get_instance(root_id)
+            let top_level_ids = root_instance.children();
-                .unwrap()
-                .instance
-                .get_children_ids();
 
             rbx_binary::encode(tree.inner(), top_level_ids, &mut file)?;
         }

src/snapshot/metadata.rs

@@ -11,23 +11,31 @@ pub struct InstanceMetadata {
     /// manage.
     pub ignore_unknown_instances: bool,
 
-    /// A complete view of where this snapshot came from. It should contain
+    // TODO: Make source_path a SmallVec<PathBuf> in order to support meta
-    /// enough information, if not None, to recreate this snapshot
+    // files? Maybe we should use another member of the snapshot middleware to
-    /// deterministically assuming the source has not changed state.
+    // support damage-painting
-    pub source: Option<InstanceSource>,
+    /// The path that this file came from, if it's the top-level instance from a
+    /// model that came directly from disk.
+    ///
+    /// This path is used to make sure that file changes update all instances
+    /// that may need updates..
+    pub source_path: Option<PathBuf>,
+
+    /// If this instance was defined in a project file, this is the name from
+    /// the project file and the node under it.
+    ///
+    /// This information is used to make sure the instance has the correct name,
+    /// project-added children, and metadata when it's updated in response to a
+    /// file change.
+    pub project_node: Option<(String, ProjectNode)>,
 }
 
 impl Default for InstanceMetadata {
     fn default() -> Self {
         InstanceMetadata {
             ignore_unknown_instances: false,
-            source: None,
+            source_path: None,
+            project_node: None,
         }
     }
 }
-
-#[derive(Debug, Clone, PartialEq)]
-pub struct InstanceSource {
-    pub path: PathBuf,
-    pub project_node: Option<(String, ProjectNode)>,
-}

src/snapshot/patch_apply.rs

@@ -100,6 +100,10 @@ fn apply_update_child(
     tree: &mut RojoTree,
     patch: &PatchUpdateInstance,
 ) {
+    if let Some(metadata) = &patch.changed_metadata {
+        tree.update_metadata(patch.id, metadata.clone());
+    }
+
     let mut instance = tree
         .get_instance_mut(patch.id)
         .expect("Instance referred to by patch does not exist");
@@ -128,10 +132,7 @@ fn apply_update_child(
                 );
             }
             Some(value) => {
-                instance
+                instance.properties_mut().insert(key.clone(), value.clone());
-                    .instance
-                    .properties
-                    .insert(key.clone(), value.clone());
             }
             None => {
                 instance.properties_mut().remove(key);

src/snapshot/patch_compute.rs

@@ -3,11 +3,11 @@
 
 use std::collections::{HashMap, HashSet};
 
-use rbx_dom_weak::{RbxId, RbxInstance, RbxValue};
+use rbx_dom_weak::{RbxId, RbxValue};
 
 use super::{
     patch::{PatchAddInstance, PatchSet, PatchUpdateInstance},
-    InstanceSnapshot, RojoTree,
+    InstanceSnapshot, InstanceWithMeta, RojoTree,
 };
 
 pub fn compute_patch_set<'a>(
@@ -84,34 +84,40 @@ fn compute_patch_set_internal<'a>(
         .get_instance(id)
         .expect("Instance did not exist in tree");
 
-    compute_property_patches(snapshot, &instance.instance, patch_set);
+    compute_property_patches(snapshot, &instance, patch_set);
     compute_children_patches(context, snapshot, tree, id, patch_set);
 }
 
 fn compute_property_patches(
     snapshot: &InstanceSnapshot,
-    instance: &RbxInstance,
+    instance: &InstanceWithMeta,
     patch_set: &mut PatchSet,
 ) {
     let mut visited_properties = HashSet::new();
     let mut changed_properties = HashMap::new();
 
-    let changed_name = if snapshot.name == instance.name {
+    let changed_name = if snapshot.name == instance.name() {
         None
     } else {
         Some(snapshot.name.clone().into_owned())
     };
 
-    let changed_class_name = if snapshot.class_name == instance.class_name {
+    let changed_class_name = if snapshot.class_name == instance.class_name() {
         None
     } else {
         Some(snapshot.class_name.clone().into_owned())
     };
 
+    let changed_metadata = if &snapshot.metadata == instance.metadata() {
+        None
+    } else {
+        Some(snapshot.metadata.clone())
+    };
+
     for (name, snapshot_value) in &snapshot.properties {
         visited_properties.insert(name.as_str());
 
-        match instance.properties.get(name) {
+        match instance.properties().get(name) {
             Some(instance_value) => {
                 if snapshot_value != instance_value {
                     changed_properties.insert(name.clone(), Some(snapshot_value.clone()));
@@ -123,7 +129,7 @@ fn compute_property_patches(
         }
     }
 
-    for name in instance.properties.keys() {
+    for name in instance.properties().keys() {
         if visited_properties.contains(name.as_str()) {
             continue;
         }
@@ -131,16 +137,20 @@ fn compute_property_patches(
         changed_properties.insert(name.clone(), None);
     }
 
-    if changed_properties.is_empty() && changed_name.is_none() {
+    if changed_properties.is_empty()
+        && changed_name.is_none()
+        && changed_class_name.is_none()
+        && changed_metadata.is_none()
+    {
         return;
     }
 
     patch_set.updated_instances.push(PatchUpdateInstance {
-        id: instance.get_id(),
+        id: instance.id(),
         changed_name,
         changed_class_name,
         changed_properties,
-        changed_metadata: None,
+        changed_metadata,
     });
 }
 

src/snapshot/tree.rs

@@ -99,9 +99,38 @@ impl RojoTree {
         }
     }
 
+    /// Replaces the metadata associated with the given instance ID.
+    pub fn update_metadata(&mut self, id: RbxId, metadata: InstanceMetadata) {
+        use std::collections::hash_map::Entry;
+
+        match self.metadata_map.entry(id) {
+            Entry::Occupied(mut entry) => {
+                let existing_metadata = entry.get();
+
+                // If this instance's source path changed, we need to update our
+                // path associations so that file changes will trigger updates
+                // to this instance correctly.
+                if existing_metadata.source_path != metadata.source_path {
+                    if let Some(existing_path) = &existing_metadata.source_path {
+                        self.path_to_ids.remove(existing_path, id);
+                    }
+
+                    if let Some(new_path) = &metadata.source_path {
+                        self.path_to_ids.insert(new_path.clone(), id);
+                    }
+                }
+
+                entry.insert(metadata);
+            }
+            Entry::Vacant(entry) => {
+                entry.insert(metadata);
+            }
+        }
+    }
+
     fn insert_metadata(&mut self, id: RbxId, metadata: InstanceMetadata) {
-        if let Some(source) = &metadata.source {
+        if let Some(source_path) = &metadata.source_path {
-            self.path_to_ids.insert(source.path.clone(), id);
+            self.path_to_ids.insert(source_path.clone(), id);
         }
 
         self.metadata_map.insert(id, metadata);
@@ -117,15 +146,16 @@ impl RojoTree {
     ) {
         let metadata = self.metadata_map.remove(&id).unwrap();
 
-        if let Some(source) = &metadata.source {
+        if let Some(source_path) = &metadata.source_path {
-            self.path_to_ids.remove(&source.path, id);
+            self.path_to_ids.remove(source_path, id);
-            path_to_ids.insert(source.path.clone(), id);
+            path_to_ids.insert(source_path.clone(), id);
         }
 
         metadata_map.insert(id, metadata);
     }
 }
 
+/// RojoTree's equivalent of `RbxInstanceProperties`.
 #[derive(Debug, Clone)]
 pub struct InstancePropertiesWithMeta {
     pub properties: RbxInstanceProperties,
@@ -141,13 +171,22 @@ impl InstancePropertiesWithMeta {
     }
 }
 
-#[derive(Debug)]
+/// RojoTree's equivalent of `&'a RbxInstance`.
+///
+/// This has to be a value type for RojoTree because the instance and metadata
+/// are stored in different places. The mutable equivalent is
+/// `InstanceWithMetaMut`.
+#[derive(Debug, Clone, Copy)]
 pub struct InstanceWithMeta<'a> {
-    pub instance: &'a RbxInstance,
+    instance: &'a RbxInstance,
-    pub metadata: &'a InstanceMetadata,
+    metadata: &'a InstanceMetadata,
 }
 
 impl InstanceWithMeta<'_> {
+    pub fn id(&self) -> RbxId {
+        self.instance.get_id()
+    }
+
     pub fn name(&self) -> &str {
         &self.instance.name
     }
@@ -163,15 +202,28 @@ impl InstanceWithMeta<'_> {
     pub fn children(&self) -> &[RbxId] {
         self.instance.get_children_ids()
     }
+
+    pub fn metadata(&self) -> &InstanceMetadata {
+        &self.metadata
+    }
 }
 
+/// RojoTree's equivalent of `&'a mut RbxInstance`.
+///
+/// This has to be a value type for RojoTree because the instance and metadata
+/// are stored in different places. The immutable equivalent is
+/// `InstanceWithMeta`.
 #[derive(Debug)]
 pub struct InstanceWithMetaMut<'a> {
-    pub instance: &'a mut RbxInstance,
+    instance: &'a mut RbxInstance,
-    pub metadata: &'a mut InstanceMetadata,
+    metadata: &'a mut InstanceMetadata,
 }
 
 impl InstanceWithMetaMut<'_> {
+    pub fn id(&self) -> RbxId {
+        self.instance.get_id()
+    }
+
     pub fn name(&self) -> &str {
         &self.instance.name
     }
@@ -199,4 +251,8 @@ impl InstanceWithMetaMut<'_> {
     pub fn children(&self) -> &[RbxId] {
         self.instance.get_children_ids()
     }
+
+    pub fn metadata(&self) -> &InstanceMetadata {
+        &self.metadata
+    }
 }