Code review

Author: Keavon

demo-artwork/isometric-fountain.graphite

@@ -67,5 +67,7 @@ futures = { workspace = true }
 tokio = { workspace = true, features = ["rt", "macros"] }
 
 [lints.rust]
-# ToDo: figure out why we check these features when they do not exist
-unexpected_cfgs = { level = "warn", check-cfg = ['cfg(feature, values("resvg", "vello"))'] }
+# TODO: figure out why we check these features when they do not exist
+unexpected_cfgs = { level = "warn", check-cfg = [
+	'cfg(feature, values("resvg", "vello"))',
+] }

demo-artwork/just-a-potted-cactus.graphite

@@ -433,19 +433,18 @@ mod test {
 
 		for (document_name, _, file_name) in crate::messages::dialog::simple_dialogs::ARTWORK {
 			let document_serialized_content = std::fs::read_to_string(format!("../demo-artwork/{file_name}")).unwrap();
-			let document_serialized_content = document_serialized_content.replace("\"ManipulatorGroupIds\"", "\"PointIds\"");
 
-			// assert_eq!(
-			// 	document_serialized_content.lines().count(),
-			// 	1,
-			// 	"Demo artwork '{document_name}' has more than 1 line (remember to open and re-save it in Graphite)",
-			// );
+			assert_eq!(
+				document_serialized_content.lines().count(),
+				1,
+				"Demo artwork '{document_name}' has more than 1 line (remember to open and re-save it in Graphite)",
+			);
 
 			let responses = editor.handle_message(PortfolioMessage::OpenDocumentFile {
 				document_name: document_name.into(),
 				document_serialized_content,
 			});
-			println!("Res {responses:#?}");
+			println!("Responses:\n{responses:#?}");
 
 			// Check if the graph renders
 			let portfolio = &mut editor.dispatcher.message_handlers.portfolio_message_handler;
@@ -454,7 +453,7 @@ mod test {
 				.submit_node_graph_evaluation(portfolio.documents.get_mut(&portfolio.active_document_id.unwrap()).unwrap(), glam::UVec2::ONE);
 			crate::node_graph_executor::run_node_graph().await;
 			let mut messages = VecDeque::new();
-			editor.poll_node_graph_evaluation(&mut messages).expect("graph should render");
+			editor.poll_node_graph_evaluation(&mut messages).expect("Graph should render");
 
 			for response in responses {
 				// Check for the existence of the file format incompatibility warning dialog after opening the test file
@@ -476,9 +475,11 @@ mod test {
 	async fn migrate() {
 		use crate::messages::portfolio::document::graph_operation::transform_utils::*;
 		use crate::messages::portfolio::document::graph_operation::utility_types::*;
+
 		use graph_craft::document::NodeInput;
 		use graph_craft::document::{value::TaggedValue, DocumentNodeImplementation};
 		use graphene_core::vector::*;
+
 		init_logger();
 		let mut editor = Editor::create();
 
@@ -508,8 +509,9 @@ mod test {
 				.executor
 				.submit_node_graph_evaluation(portfolio.documents.get_mut(&portfolio.active_document_id.unwrap()).unwrap(), glam::UVec2::ONE);
 			crate::node_graph_executor::run_node_graph().await;
+
 			let mut messages = VecDeque::new();
-			editor.poll_node_graph_evaluation(&mut messages).expect("graph should render");
+			editor.poll_node_graph_evaluation(&mut messages).expect("Graph should render");
 
 			let document = editor.dispatcher.message_handlers.portfolio_message_handler.active_document_mut().unwrap();
 			let mut updated_nodes = HashSet::new();
@@ -519,25 +521,28 @@ mod test {
 				if document.metadata.is_folder(layer) {
 					continue;
 				}
-				println!("Layer {layer:?}");
 				let bounds = LayerBounds::new(&document.metadata, layer);
+
 				let mut responses = VecDeque::new();
 				let mut shape = None;
+
 				if let Some(mut modify_inputs) = ModifyInputsContext::new_with_layer(layer.to_node(), &mut document.network, &mut document.metadata, &mut document.node_graph_handler, &mut responses) {
 					modify_inputs.modify_existing_inputs("Transform", |inputs, node_id, metadata| {
 						if !updated_nodes.insert(node_id) {
 							return;
 						}
+
 						let transform = get_current_transform(&inputs);
 						let upstream_transform = metadata.upstream_transform(node_id);
 						let pivot_transform = glam::DAffine2::from_translation(upstream_transform.transform_point2(bounds.local_pivot(get_current_normalized_pivot(&inputs))));
-						println!("pivot {pivot_transform}");
+
 						update_transform(inputs, pivot_transform * transform * pivot_transform.inverse());
 					});
 					modify_inputs.modify_existing_inputs("Shape", |inputs, node_id, metadata| {
 						if !updated_nodes.insert(node_id) {
 							return;
 						}
+
 						let path_data = if let NodeInput::Value {
 							tagged_value: TaggedValue::Subpaths(translation),
 							..
@@ -547,6 +552,7 @@ mod test {
 						} else {
 							&Vec::new()
 						};
+
 						let colinear_manipulators = if let NodeInput::Value {
 							tagged_value: TaggedValue::PointIds(translation),
 							..
@@ -556,11 +562,13 @@ mod test {
 						} else {
 							&Vec::new()
 						};
+
 						let mut vector_data = VectorData::from_subpaths(path_data, false);
 						vector_data.colinear_manipulators = colinear_manipulators
 							.iter()
 							.filter_map(|&point| ManipulatorPointId::Anchor(point).get_handle_pair(&vector_data))
 							.collect();
+
 						shape = Some((node_id, VectorModification::create_from_vector(&vector_data)));
 					});
 				}

demo-artwork/procedural-string-lights.graphite

@@ -1,7 +1,6 @@
 use super::utility_types::TransformIn;
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
 use crate::messages::prelude::*;
-use graphene_core::vector::VectorModificationType;
 
 use bezier_rs::Subpath;
 use graph_craft::document::{DocumentNode, NodeId, NodeInput};
@@ -10,6 +9,7 @@ use graphene_core::text::Font;
 use graphene_core::vector::brush_stroke::BrushStroke;
 use graphene_core::vector::style::{Fill, Stroke};
 use graphene_core::vector::PointId;
+use graphene_core::vector::VectorModificationType;
 use graphene_core::{Artboard, Color};
 use graphene_std::vector::misc::BooleanOperation;
 

demo-artwork/red-dress.graphite

@@ -1,4 +1,4 @@
-use super::transform_utils::{self};
+use super::transform_utils;
 use super::utility_types::ModifyInputsContext;
 use crate::messages::portfolio::document::node_graph::document_node_types::resolve_document_node_type;
 use crate::messages::portfolio::document::utility_types::document_metadata::{DocumentMetadata, LayerNodeIdentifier};

demo-artwork/valley-of-spires.graphite

@@ -1,8 +1,9 @@
 use bezier_rs::Subpath;
-use glam::{DAffine2, DVec2};
 use graph_craft::document::{value::TaggedValue, NodeInput};
 use graphene_core::vector::PointId;
 
+use glam::{DAffine2, DVec2};
+
 /// Convert an affine transform into the tuple `(scale, angle, translation, shear)` assuming `shear.y = 0`.
 pub fn compute_scale_angle_translation_shear(transform: DAffine2) -> (DVec2, f64, DVec2, DVec2) {
 	let x_axis = transform.matrix2.x_axis;
@@ -46,13 +47,13 @@ pub struct LayerBounds {
 	pub layer_transform: DAffine2,
 }
 
-#[cfg(test)]
-use crate::messages::portfolio::document::utility_types::document_metadata::{DocumentMetadata, LayerNodeIdentifier};
-
 #[cfg(test)]
 impl LayerBounds {
 	/// Extract the layer bounds and their transform for a layer.
-	pub fn new(metadata: &DocumentMetadata, layer: LayerNodeIdentifier) -> Self {
+	pub fn new(
+		metadata: &crate::messages::portfolio::document::utility_types::document_metadata::DocumentMetadata,
+		layer: crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier,
+	) -> Self {
 		Self {
 			bounds: metadata.nonzero_bounding_box(layer),
 			bounds_transform: DAffine2::IDENTITY,