diff --git a/src/change_processor.rs b/src/change_processor.rs index 2ce22bc8..77ae51d4 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -51,6 +51,9 @@ impl ChangeProcessor { imfs.change_receiver() }; + // Crossbeam's select macro generates code that Clippy doesn't like, and + // Clippy blames us for it. + #[allow(clippy::drop_copy)] loop { select! { recv(imfs_receiver) -> event => { diff --git a/src/snapshot/mod.rs b/src/snapshot/mod.rs index 47ef4794..de269d5b 100644 --- a/src/snapshot/mod.rs +++ b/src/snapshot/mod.rs @@ -13,6 +13,8 @@ //! usually contain data that hasn't actually changed, and how coarsely //! the snapshots happen is defined outside this part of the code. //! +//! See `src/snapshot_middleware` for implementation. +//! //! 2. Input snapshots are turned into `PatchSet` objects by Rojo's diffing //! algorithm via `compute_patch_set`. This operation doesn't mutate the //! instance tree, so work at this point can be thrown away. @@ -22,12 +24,16 @@ //! changes after the fact, since Rojo needs to assign IDs to created //! instances, which happens during patch application, not generation! //! +//! See `src/snapshot/patch_compute.rs` for implementation. +//! //! 3. Patch sets are applied to the tree with `apply_patch_set`, which //! mutates the relevant instances. `apply_patch_set` returns a new //! object, `AppliedPatchSet`. Applied patch sets describe the transform //! that was applied, and are suitable for cases where another tree needs //! to be synchronized with Rojo's, like the Rojo Studio plugin. //! +//! See `src/snapshot/patch_apply.rs` for implementation. +//! //! The aim with this approach is to reduce the number of bugs that arise from //! attempting to manually update instances in response to filesystem updates. //! Instead of surgically identifying what needs to change, we can do rough @@ -55,3 +61,6 @@ pub use patch::*; pub use patch_apply::apply_patch_set; pub use patch_compute::compute_patch_set; pub use tree::*; + +#[cfg(test)] +mod tests; diff --git a/src/snapshot/patch.rs b/src/snapshot/patch.rs index 7833a677..3908f78b 100644 --- a/src/snapshot/patch.rs +++ b/src/snapshot/patch.rs @@ -63,7 +63,7 @@ pub struct PatchUpdate { #[derive(Debug, Clone, Default)] pub struct AppliedPatchSet { pub removed: Vec, - pub added: Vec, + pub added: Vec, pub updated: Vec, } @@ -77,11 +77,6 @@ impl AppliedPatchSet { } } -#[derive(Debug, Clone)] -pub struct AppliedPatchAdd { - pub instance_id: RbxId, -} - #[derive(Debug, Clone)] pub struct AppliedPatchUpdate { pub id: RbxId, @@ -92,3 +87,15 @@ pub struct AppliedPatchUpdate { pub changed_properties: HashMap>, pub changed_metadata: Option, } + +impl AppliedPatchUpdate { + pub fn new(id: RbxId) -> Self { + Self { + id, + changed_name: None, + changed_class_name: None, + changed_properties: HashMap::new(), + changed_metadata: None, + } + } +} diff --git a/src/snapshot/patch_apply.rs b/src/snapshot/patch_apply.rs index 7463f3c6..ea28f302 100644 --- a/src/snapshot/patch_apply.rs +++ b/src/snapshot/patch_apply.rs @@ -5,10 +5,13 @@ use std::collections::HashMap; use rbx_dom_weak::{RbxId, RbxInstanceProperties, RbxValue}; use super::{ - patch::{AppliedPatchSet, PatchSet, PatchUpdate}, + patch::{AppliedPatchSet, AppliedPatchUpdate, PatchSet, PatchUpdate}, InstancePropertiesWithMeta, InstanceSnapshot, RojoTree, }; +/// Consumes the input `PatchSet`, applying all of its prescribed changes to the +/// tree and returns an `AppliedPatchSet`, which can be used to keep another +/// tree in sync with Rojo's. pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatchSet { let mut context = PatchApplyContext::default(); @@ -17,9 +20,11 @@ pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatch } for add_patch in patch_set.added_instances { - apply_add_child(&mut context, tree, add_patch.parent_id, &add_patch.instance); + apply_add_child(&mut context, tree, add_patch.parent_id, add_patch.instance); } + // Updates need to be applied after additions, which reduces the complexity + // of updates significantly. for update_patch in patch_set.updated_instances { apply_update_child(&mut context, tree, update_patch); } @@ -27,10 +32,45 @@ pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatch finalize_patch_application(context, tree) } +/// All of the ephemeral state needing during application of a patch. #[derive(Default)] struct PatchApplyContext { + /// A map from transient snapshot IDs (generated by snapshot middleware) to + /// instance IDs in the actual tree. These are both the same data type so + /// that they fit into the same `RbxValue::Ref` type. + /// + /// At this point in the patch process, IDs in instance properties have been + /// partially translated from 'snapshot space' into 'tree space' by the + /// patch computation process. An ID not existing in this map means either: + /// + /// 1. The ID is already in tree space and refers to an instance that + /// existed in the tree before this patch was applied. + /// + /// 2. The ID if in snapshot space, but points to an instance that was not + /// part of the snapshot that was put through the patch computation + /// function. + /// + /// #2 should not occur in well-formed projects, but is indistinguishable + /// from #1 right now. It could happen if two model files try to reference + /// eachother. snapshot_id_to_instance_id: HashMap, - properties_to_apply: HashMap>, + + /// The properties of instances added by the current `PatchSet`. + /// + /// Instances added to the tree can refer to eachother via Ref properties, + /// but we need to make sure they're correctly transformed from snapshot + /// space into tree space (via `snapshot_id_to_instance_id`). + /// + /// It's not possible to do that transformation for refs that refer to added + /// instances until all the instances have actually been inserted into the + /// tree. For simplicity, we defer application of _all_ properties on added + /// instances instead of just Refs. + /// + /// This doesn't affect updated instances, since they're always applied + /// after we've added all the instances from the patch. + added_instance_properties: HashMap>, + + /// The current applied patch result, describing changes made to the tree. applied_patch_set: AppliedPatchSet, } @@ -45,22 +85,24 @@ struct PatchApplyContext { /// where we build up a map of snapshot IDs to instance IDs as they're created, /// then apply properties all at once at the end. fn finalize_patch_application(context: PatchApplyContext, tree: &mut RojoTree) -> AppliedPatchSet { - for (id, mut properties) in context.properties_to_apply { + for (id, properties) in context.added_instance_properties { + // This should always succeed since instances marked as added in our + // patch should be added without fail. let mut instance = tree .get_instance_mut(id) .expect("Invalid instance ID in deferred property map"); - for property_value in properties.values_mut() { + for (key, mut property_value) in properties { if let RbxValue::Ref { value: Some(id) } = property_value { - if let Some(&instance_id) = context.snapshot_id_to_instance_id.get(id) { - *property_value = RbxValue::Ref { + if let Some(&instance_id) = context.snapshot_id_to_instance_id.get(&id) { + property_value = RbxValue::Ref { value: Some(instance_id), }; } } - } - *instance.properties_mut() = properties; + instance.properties_mut().insert(key, property_value); + } } context.applied_patch_set @@ -71,7 +113,7 @@ fn apply_remove_instance(context: &mut PatchApplyContext, tree: &mut RojoTree, r Some(_) => context.applied_patch_set.removed.push(removed_id), None => { log::warn!( - "Patch application error: Tried to remove instance {} but it did not exist.", + "Patch misapplication: Tried to remove instance {} but it did not exist.", removed_id ); } @@ -82,50 +124,64 @@ fn apply_add_child( context: &mut PatchApplyContext, tree: &mut RojoTree, parent_id: RbxId, - snapshot: &InstanceSnapshot, + snapshot: InstanceSnapshot, ) { let properties = InstancePropertiesWithMeta { properties: RbxInstanceProperties { - name: snapshot.name.clone().into_owned(), - class_name: snapshot.class_name.clone().into_owned(), + name: snapshot.name.into_owned(), + class_name: snapshot.class_name.into_owned(), // Property assignment is deferred until after we know about all - // instances in this patch. + // instances in this patch. See `PatchApplyContext` for details. properties: HashMap::new(), }, - metadata: Default::default(), // TODO + metadata: snapshot.metadata, }; let id = tree.insert_instance(properties, parent_id); + context.applied_patch_set.added.push(id); + context - .properties_to_apply - .insert(id, snapshot.properties.clone()); + .added_instance_properties + .insert(id, snapshot.properties); if let Some(snapshot_id) = snapshot.snapshot_id { context.snapshot_id_to_instance_id.insert(snapshot_id, id); } - for child_snapshot in &snapshot.children { + for child_snapshot in snapshot.children { apply_add_child(context, tree, id, child_snapshot); } } fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patch: PatchUpdate) { + let mut applied_patch = AppliedPatchUpdate::new(patch.id); + if let Some(metadata) = patch.changed_metadata { - tree.update_metadata(patch.id, metadata); + tree.update_metadata(patch.id, metadata.clone()); + applied_patch.changed_metadata = Some(metadata); } - let mut instance = tree - .get_instance_mut(patch.id) - .expect("Instance referred to by patch does not exist"); + let mut instance = match tree.get_instance_mut(patch.id) { + Some(instance) => instance, + None => { + log::warn!( + "Patch misapplication: Instance {}, referred to by update patch, did not exist.", + patch.id + ); + return; + } + }; if let Some(name) = patch.changed_name { - *instance.name_mut() = name; + *instance.name_mut() = name.clone(); + applied_patch.changed_name = Some(name); } if let Some(class_name) = patch.changed_class_name { - *instance.class_name_mut() = class_name; + *instance.class_name_mut() = class_name.clone(); + applied_patch.changed_class_name = Some(class_name); } for (key, property_entry) in patch.changed_properties { @@ -134,6 +190,10 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc // instance IDs if they referred to an instance that was created as // part of this patch. Some(RbxValue::Ref { value: Some(id) }) => { + // If our ID is not found in this map, then it either refers to + // an existing instance NOT added by this patch, or there was an + // error. See `PatchApplyContext::snapshot_id_to_instance_id` + // for more info. let new_id = context .snapshot_id_to_instance_id .get(&id) @@ -141,20 +201,24 @@ fn apply_update_child(context: &mut PatchApplyContext, tree: &mut RojoTree, patc .unwrap_or(id); instance.properties_mut().insert( - key, + key.clone(), RbxValue::Ref { value: Some(new_id), }, ); } - Some(value) => { - instance.properties_mut().insert(key, value); + Some(ref value) => { + instance.properties_mut().insert(key.clone(), value.clone()); } None => { instance.properties_mut().remove(&key); } } + + applied_patch.changed_properties.insert(key, property_entry); } + + context.applied_patch_set.updated.push(applied_patch) } #[cfg(test)] diff --git a/src/snapshot/tests/apply.rs b/src/snapshot/tests/apply.rs new file mode 100644 index 00000000..f06fa271 --- /dev/null +++ b/src/snapshot/tests/apply.rs @@ -0,0 +1,37 @@ +use rbx_dom_weak::RbxInstanceProperties; + +use crate::snapshot::{ + apply_patch_set, InstancePropertiesWithMeta, PatchSet, PatchUpdate, RojoTree, +}; + +#[test] +fn reify_folder() { + let mut tree = empty_tree(); + + let patch_set = PatchSet { + updated_instances: vec![PatchUpdate { + id: tree.get_root_id(), + changed_name: Some("Hello, world!".to_owned()), + changed_class_name: Some("Folder".to_owned()), + changed_properties: Default::default(), + changed_metadata: None, + }], + ..Default::default() + }; + + let _applied_patch_set = apply_patch_set(&mut tree, patch_set); + + // TODO: Make assertions about tree using snapshots + // TODO: make assertions about applied patch set using snapshots +} + +fn empty_tree() -> RojoTree { + RojoTree::new(InstancePropertiesWithMeta { + properties: RbxInstanceProperties { + name: "ROOT".to_owned(), + class_name: "ROOT".to_owned(), + properties: Default::default(), + }, + metadata: Default::default(), + }) +} diff --git a/src/snapshot/tests/compute.rs b/src/snapshot/tests/compute.rs new file mode 100644 index 00000000..c523c3d1 --- /dev/null +++ b/src/snapshot/tests/compute.rs @@ -0,0 +1,35 @@ +use std::borrow::Cow; + +use rbx_dom_weak::RbxInstanceProperties; + +use crate::snapshot::{compute_patch_set, InstancePropertiesWithMeta, InstanceSnapshot, RojoTree}; + +#[test] +fn reify_folder() { + let tree = empty_tree(); + + let folder = InstanceSnapshot { + snapshot_id: None, + metadata: Default::default(), + name: Cow::Borrowed("Some Folder"), + class_name: Cow::Borrowed("Folder"), + properties: Default::default(), + children: Vec::new(), + }; + + let _patch_set = compute_patch_set(&folder, &tree, tree.get_root_id()); + + // TODO: Make assertions about patch set using snapshots. This needs patches + // to be serializable and also to have ID redactions more readily available. +} + +fn empty_tree() -> RojoTree { + RojoTree::new(InstancePropertiesWithMeta { + properties: RbxInstanceProperties { + name: "ROOT".to_owned(), + class_name: "ROOT".to_owned(), + properties: Default::default(), + }, + metadata: Default::default(), + }) +} diff --git a/src/snapshot/tests/mod.rs b/src/snapshot/tests/mod.rs new file mode 100644 index 00000000..35edf7ae --- /dev/null +++ b/src/snapshot/tests/mod.rs @@ -0,0 +1,2 @@ +mod apply; +mod compute;