Fix the Text node's Max Width/Height parameters with OptionalF64 losing the value when unticked (#3643)

* WIP

* Fix widget

* Fix migration

* Remove OptionalF64

* Custom attributes for optional f64 widget

* Code review

* Move comments to another PR

---------

Co-authored-by: Keavon Chambers <keavon@keavon.com>

Author: adamgerhant

editor/src/messages/portfolio/document/graph_operation/utility_types.rs

@@ -206,8 +206,10 @@ impl<'a> ModifyInputsContext<'a> {
 				Some(NodeInput::value(TaggedValue::F64(typesetting.font_size), false)),
 				Some(NodeInput::value(TaggedValue::F64(typesetting.line_height_ratio), false)),
 				Some(NodeInput::value(TaggedValue::F64(typesetting.character_spacing), false)),
-				Some(NodeInput::value(TaggedValue::OptionalF64(typesetting.max_width), false)),
-				Some(NodeInput::value(TaggedValue::OptionalF64(typesetting.max_height), false)),
+				Some(NodeInput::value(TaggedValue::Bool(typesetting.max_width.is_some()), false)),
+				Some(NodeInput::value(TaggedValue::F64(typesetting.max_width.unwrap_or(100.)), false)),
+				Some(NodeInput::value(TaggedValue::Bool(typesetting.max_width.is_some()), false)),
+				Some(NodeInput::value(TaggedValue::F64(typesetting.max_width.unwrap_or(100.)), false)),
 				Some(NodeInput::value(TaggedValue::F64(typesetting.tilt), false)),
 				Some(NodeInput::value(TaggedValue::TextAlign(typesetting.align), false)),
 			]);

editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs

@@ -21,7 +21,6 @@ use graphene_std::extract_xy::XY;
 use graphene_std::raster::{CellularDistanceFunction, CellularReturnType, Color, DomainWarpType, FractalType, NoiseType, RedGreenBlueAlpha};
 use graphene_std::raster_types::{CPU, Raster};
 use graphene_std::table::Table;
-use graphene_std::text::{Font, TypesettingConfig};
 #[allow(unused_imports)]
 use graphene_std::transform::Footprint;
 use graphene_std::vector::Vector;
@@ -1653,103 +1652,6 @@ fn document_node_definitions() -> HashMap<DefinitionIdentifier, DocumentNodeDefi
 			properties: None,
 		},
 		// TODO: Auto-generate this from its proto node macro
-		DocumentNodeDefinition {
-			identifier: "Text",
-			category: "Text",
-			node_template: NodeTemplate {
-				document_node: DocumentNode {
-					implementation: DocumentNodeImplementation::ProtoNode(text::text::IDENTIFIER),
-					inputs: vec![
-						NodeInput::scope("editor-api"),
-						NodeInput::value(TaggedValue::String("Lorem ipsum".to_string()), false),
-						NodeInput::value(
-							TaggedValue::Font(Font::new(graphene_std::consts::DEFAULT_FONT_FAMILY.into(), graphene_std::consts::DEFAULT_FONT_STYLE.into())),
-							false,
-						),
-						NodeInput::value(TaggedValue::F64(TypesettingConfig::default().font_size), false),
-						NodeInput::value(TaggedValue::F64(TypesettingConfig::default().line_height_ratio), false),
-						NodeInput::value(TaggedValue::F64(TypesettingConfig::default().character_spacing), false),
-						NodeInput::value(TaggedValue::OptionalF64(TypesettingConfig::default().max_width), false),
-						NodeInput::value(TaggedValue::OptionalF64(TypesettingConfig::default().max_height), false),
-						NodeInput::value(TaggedValue::F64(TypesettingConfig::default().tilt), false),
-						NodeInput::value(TaggedValue::TextAlign(text::TextAlign::default()), false),
-						NodeInput::value(TaggedValue::Bool(false), false),
-					],
-					..Default::default()
-				},
-				persistent_node_metadata: DocumentNodePersistentMetadata {
-					input_metadata: vec![
-						("Editor API", "TODO").into(),
-						InputMetadata::with_name_description_override("Text", "TODO", WidgetOverride::Custom("text_area".to_string())),
-						InputMetadata::with_name_description_override("Font", "TODO", WidgetOverride::Custom("text_font".to_string())),
-						InputMetadata::with_name_description_override(
-							"Size",
-							"TODO",
-							WidgetOverride::Number(NumberInputSettings {
-								unit: Some(" px".to_string()),
-								min: Some(1.),
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override(
-							"Line Height",
-							"TODO",
-							WidgetOverride::Number(NumberInputSettings {
-								unit: Some("x".to_string()),
-								min: Some(0.),
-								step: Some(0.1),
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override(
-							"Character Spacing",
-							"TODO",
-							WidgetOverride::Number(NumberInputSettings {
-								unit: Some(" px".to_string()),
-								step: Some(0.1),
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override(
-							"Max Width",
-							"TODO",
-							WidgetOverride::Number(NumberInputSettings {
-								unit: Some(" px".to_string()),
-								min: Some(1.),
-								blank_assist: false,
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override(
-							"Max Height",
-							"TODO",
-							WidgetOverride::Number(NumberInputSettings {
-								unit: Some(" px".to_string()),
-								min: Some(1.),
-								blank_assist: false,
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override(
-							"Tilt",
-							"Faux italic.",
-							WidgetOverride::Number(NumberInputSettings {
-								min: Some(-85.),
-								max: Some(85.),
-								unit: Some("°".to_string()),
-								..Default::default()
-							}),
-						),
-						InputMetadata::with_name_description_override("Align", "TODO", WidgetOverride::Custom("text_align".to_string())),
-						("Per-Glyph Instances", "Splits each text glyph into its own row in the table of vector geometry.").into(),
-					],
-					output_names: vec!["Vector".to_string()],
-					..Default::default()
-				},
-			},
-			description: Cow::Borrowed("TODO"),
-			properties: None,
-		},
 		DocumentNodeDefinition {
 			identifier: "Transform",
 			category: "Math: Transform",
@@ -2297,6 +2199,43 @@ fn static_input_properties() -> InputProperties {
 			}])
 		}),
 	);
+	map.insert(
+		// The custom number input settings are only available on proto nodes
+		"optional_f64".to_string(),
+		Box::new(|node_id, index, context| {
+			let node_metadata = registry::NODE_METADATA.lock().unwrap();
+			let mut number_input = NumberInput::default();
+			if let Some(field) = context
+				.network_interface
+				.implementation(&node_id, context.selection_network_path)
+				.and_then(|implementation| if let DocumentNodeImplementation::ProtoNode(id) = implementation { Some(id) } else { None })
+				.and_then(|proto_node_identifier| node_metadata.get(proto_node_identifier))
+				.and_then(|metadata| metadata.fields.get(index))
+			{
+				if let Some(unit) = field.unit {
+					number_input = number_input.unit(unit);
+				}
+				if let Some(number_min) = field.number_min {
+					number_input = number_input.min(number_min);
+				}
+				if let Some(number_max) = field.number_max {
+					number_input = number_input.max(number_max);
+				}
+				if let Some((range_min, range_max)) = field.number_mode_range {
+					number_input = number_input.range_min(Some(range_min));
+					number_input = number_input.range_max(Some(range_max));
+				}
+				number_input = number_input.is_integer(false);
+				if let Some(number_step) = field.number_step {
+					number_input = number_input.step(number_step);
+				}
+			};
+			Ok(vec![LayoutGroup::Row {
+				// NOTE: The bool input MUST be at the input index directly before the f64 input!
+				widgets: node_properties::optional_f64_widget(ParameterWidgetsInfo::new(node_id, index, false, context), index - 1, number_input),
+			}])
+		}),
+	);
 	map.insert(
 		"vec2".to_string(),
 		Box::new(|node_id, index, context| {

editor/src/messages/portfolio/document/node_graph/node_properties.rs

@@ -939,6 +939,49 @@ pub fn progression_widget(parameter_widgets_info: ParameterWidgetsInfo, number_p
 	widgets
 }
 
+/// `parameter_widgets_info` is for the f64 parameter. `bool_input_index` is the input index of the bool parameter for the checkbox.
+pub fn optional_f64_widget(parameter_widgets_info: ParameterWidgetsInfo, bool_input_index: usize, number_props: NumberInput) -> Vec<WidgetInstance> {
+	let ParameterWidgetsInfo {
+		document_node,
+		node_id,
+		index: number_input_index,
+		..
+	} = parameter_widgets_info;
+
+	let mut widgets = start_widgets(parameter_widgets_info);
+
+	let Some(document_node) = document_node else { return Vec::new() };
+	let Some(number_input) = document_node.inputs.get(number_input_index) else {
+		log::warn!("A widget failed to be built because its node's input index is invalid.");
+		return vec![];
+	};
+	let Some(bool_input) = document_node.inputs.get(bool_input_index) else {
+		log::warn!("A widget failed to be built because its node's input index is invalid.");
+		return vec![];
+	};
+	if let (Some(&TaggedValue::Bool(enabled)), Some(&TaggedValue::F64(number))) = (bool_input.as_non_exposed_value(), number_input.as_non_exposed_value()) {
+		widgets.extend_from_slice(&[
+			Separator::new(SeparatorStyle::Unrelated).widget_instance(),
+			Separator::new(SeparatorStyle::Related).widget_instance(),
+			// The checkbox toggles if the value is Some or None
+			CheckboxInput::new(enabled)
+				.on_update(update_value(|x: &CheckboxInput| TaggedValue::Bool(x.checked), node_id, bool_input_index))
+				.on_commit(commit_value)
+				.widget_instance(),
+			Separator::new(SeparatorStyle::Related).widget_instance(),
+			Separator::new(SeparatorStyle::Unrelated).widget_instance(),
+			number_props
+				.value(Some(number))
+				.on_update(update_value(move |x: &NumberInput| TaggedValue::F64(x.value.unwrap_or_default()), node_id, number_input_index))
+				.disabled(!enabled)
+				.on_commit(commit_value)
+				.widget_instance(),
+		]);
+	}
+
+	widgets
+}
+
 pub fn number_widget(parameter_widgets_info: ParameterWidgetsInfo, number_props: NumberInput) -> Vec<WidgetInstance> {
 	let ParameterWidgetsInfo { document_node, node_id, index, .. } = parameter_widgets_info;
 
@@ -982,27 +1025,6 @@ pub fn number_widget(parameter_widgets_info: ParameterWidgetsInfo, number_props:
 				.on_commit(commit_value)
 				.widget_instance(),
 		]),
-		Some(&TaggedValue::OptionalF64(x)) => {
-			// TODO: Don't wipe out the previously set value (setting it back to the default of 100) when reenabling this checkbox back to Some from None
-			let toggle_enabled = move |checkbox_input: &CheckboxInput| TaggedValue::OptionalF64(if checkbox_input.checked { Some(100.) } else { None });
-			widgets.extend_from_slice(&[
-				Separator::new(SeparatorStyle::Unrelated).widget_instance(),
-				Separator::new(SeparatorStyle::Related).widget_instance(),
-				// The checkbox toggles if the value is Some or None
-				CheckboxInput::new(x.is_some())
-					.on_update(update_value(toggle_enabled, node_id, index))
-					.on_commit(commit_value)
-					.widget_instance(),
-				Separator::new(SeparatorStyle::Related).widget_instance(),
-				Separator::new(SeparatorStyle::Unrelated).widget_instance(),
-				number_props
-					.value(x)
-					.on_update(update_value(move |x: &NumberInput| TaggedValue::OptionalF64(x.value), node_id, index))
-					.disabled(x.is_none())
-					.on_commit(commit_value)
-					.widget_instance(),
-			]);
-		}
 		Some(&TaggedValue::DVec2(dvec2)) => widgets.extend_from_slice(&[
 			Separator::new(SeparatorStyle::Unrelated).widget_instance(),
 			number_props
@@ -1087,14 +1109,6 @@ pub fn color_widget(parameter_widgets_info: ParameterWidgetsInfo, color_button:
 				.on_commit(commit_value)
 				.widget_instance(),
 		),
-		TaggedValue::OptionalColorNotInTable(color) => widgets.push(
-			color_button
-				.value(color.map_or(FillChoice::None, FillChoice::Solid))
-				.allow_none(true)
-				.on_update(update_value(|input: &ColorInput| TaggedValue::OptionalColorNotInTable(input.value.as_solid()), node_id, index))
-				.on_commit(commit_value)
-				.widget_instance(),
-		),
 		TaggedValue::Color(color_table) => widgets.push(
 			color_button
 				.value(match color_table.iter().next() {