From cb6de01059cea82f2d414e75645b3cdcf620bead Mon Sep 17 00:00:00 2001 From: Arjan Woltjer Date: Fri, 11 Sep 2026 16:17:52 +0200 Subject: [PATCH] fix: accept NULL line, font and foreground attribute references ISO 11783-6 allows the Line Attributes reference of the output shape objects, the Font Attributes reference of the string and number objects and the Foreground Colour reference of Input Boolean to be the NULL object ID, and real pools use it: a John Deere StarFire pool nulls line attributes on 213 shapes and foreground colour on all 38 of its Input Booleans. Reading them with try_into() failed with UnexpectedNullObjectId, and because from_iop stops at the first error the rest of the pool was silently dropped -- including the Working Set, which John Deere sends last. Make the nine fields NullableObjectId. The writer already accepts it, so NULL round-trips as 0xFFFF. Add ObjectPool::try_from_iop / try_extend_with_iop, strict counterparts that return the first ParseError; from_iop keeps its lenient behaviour. The existing read_working_set_test did not compile on this branch (Colour where u8 is expected); one-token fix so the test target builds. Co-Authored-By: Claude Fable 5.1 --- src/object_pool/object.rs | 36 ++++----- src/object_pool/object_pool.rs | 37 ++++++++- src/object_pool/reader.rs | 136 ++++++++++++++++++++++++++++++--- 3 files changed, 178 insertions(+), 31 deletions(-) diff --git a/src/object_pool/object.rs b/src/object_pool/object.rs index ab831d7..c4817a9 100644 --- a/src/object_pool/object.rs +++ b/src/object_pool/object.rs @@ -291,16 +291,16 @@ impl Object { Object::Key(o) => extend_object_refs(&mut refs, &o.object_refs), Object::Button(o) => extend_object_refs(&mut refs, &o.object_refs), Object::InputBoolean(o) => { - refs.push(o.foreground_colour); + push_nullable_id(&mut refs, &o.foreground_colour); push_nullable_id(&mut refs, &o.variable_reference); } Object::InputString(o) => { - refs.push(o.font_attributes); + push_nullable_id(&mut refs, &o.font_attributes); push_nullable_id(&mut refs, &o.input_attributes); push_nullable_id(&mut refs, &o.variable_reference); } Object::InputNumber(o) => { - refs.push(o.font_attributes); + push_nullable_id(&mut refs, &o.font_attributes); push_nullable_id(&mut refs, &o.variable_reference); } Object::InputList(o) => { @@ -308,28 +308,28 @@ impl Object { extend_nullable_ids(&mut refs, &o.list_items); } Object::OutputString(o) => { - refs.push(o.font_attributes); + push_nullable_id(&mut refs, &o.font_attributes); push_nullable_id(&mut refs, &o.variable_reference); } Object::OutputNumber(o) => { - refs.push(o.font_attributes); + push_nullable_id(&mut refs, &o.font_attributes); push_nullable_id(&mut refs, &o.variable_reference); } Object::OutputList(o) => { push_nullable_id(&mut refs, &o.variable_reference); extend_nullable_ids(&mut refs, &o.list_items); } - Object::OutputLine(o) => refs.push(o.line_attributes), + Object::OutputLine(o) => push_nullable_id(&mut refs, &o.line_attributes), Object::OutputRectangle(o) => { - refs.push(o.line_attributes); + push_nullable_id(&mut refs, &o.line_attributes); push_nullable_id(&mut refs, &o.fill_attributes); } Object::OutputEllipse(o) => { - refs.push(o.line_attributes); + push_nullable_id(&mut refs, &o.line_attributes); push_nullable_id(&mut refs, &o.fill_attributes); } Object::OutputPolygon(o) => { - refs.push(o.line_attributes); + push_nullable_id(&mut refs, &o.line_attributes); push_nullable_id(&mut refs, &o.fill_attributes); } Object::OutputMeter(o) => push_nullable_id(&mut refs, &o.variable_reference), @@ -527,7 +527,7 @@ pub struct InputBoolean { pub id: ObjectId, pub background_colour: u8, pub width: u16, - pub foreground_colour: ObjectId, + pub foreground_colour: NullableObjectId, pub variable_reference: NullableObjectId, pub value: bool, pub enabled: bool, @@ -550,7 +550,7 @@ pub struct InputString { pub width: u16, pub height: u16, pub background_colour: u8, - pub font_attributes: ObjectId, + pub font_attributes: NullableObjectId, pub input_attributes: NullableObjectId, pub options: InputStringOptions, pub variable_reference: NullableObjectId, @@ -576,7 +576,7 @@ pub struct InputNumber { pub width: u16, pub height: u16, pub background_colour: u8, - pub font_attributes: ObjectId, + pub font_attributes: NullableObjectId, pub options: NumberOptions, pub variable_reference: NullableObjectId, pub value: u32, @@ -629,7 +629,7 @@ pub struct OutputString { pub width: u16, pub height: u16, pub background_colour: u8, - pub font_attributes: ObjectId, + pub font_attributes: NullableObjectId, pub options: OutputStringOptions, pub variable_reference: NullableObjectId, pub justification: Alignment, @@ -653,7 +653,7 @@ pub struct OutputNumber { pub width: u16, pub height: u16, pub background_colour: u8, - pub font_attributes: ObjectId, + pub font_attributes: NullableObjectId, pub options: NumberOptions, pub variable_reference: NullableObjectId, pub value: u32, @@ -699,7 +699,7 @@ impl SizedObject for OutputList { #[derive(Debug, PartialEq, Clone)] pub struct OutputLine { pub id: ObjectId, - pub line_attributes: ObjectId, + pub line_attributes: NullableObjectId, pub width: u16, pub height: u16, pub line_direction: LineDirection, @@ -719,7 +719,7 @@ impl SizedObject for OutputLine { #[derive(Debug, PartialEq, Clone)] pub struct OutputRectangle { pub id: ObjectId, - pub line_attributes: ObjectId, + pub line_attributes: NullableObjectId, pub width: u16, pub height: u16, pub line_suppression: u8, @@ -740,7 +740,7 @@ impl SizedObject for OutputRectangle { #[derive(Debug, PartialEq, Clone)] pub struct OutputEllipse { pub id: ObjectId, - pub line_attributes: ObjectId, + pub line_attributes: NullableObjectId, pub width: u16, pub height: u16, pub ellipse_type: u8, @@ -765,7 +765,7 @@ pub struct OutputPolygon { pub id: ObjectId, pub width: u16, pub height: u16, - pub line_attributes: ObjectId, + pub line_attributes: NullableObjectId, pub fill_attributes: NullableObjectId, pub polygon_type: u8, pub points: Vec>, diff --git a/src/object_pool/object_pool.rs b/src/object_pool/object_pool.rs index b321d84..83441bb 100644 --- a/src/object_pool/object_pool.rs +++ b/src/object_pool/object_pool.rs @@ -7,6 +7,7 @@ use crate::object_pool::object::{ use crate::object_pool::object_id::ObjectId; use crate::object_pool::vt_version::VtVersion; use crate::object_pool::ObjectType; +use crate::object_pool::ParseError; #[derive(Debug, Clone, PartialEq)] pub struct ObjectPool { @@ -76,18 +77,50 @@ impl ObjectPool { op } + /// Like [`from_iop`](Self::from_iop), but returns the first parse error + /// instead of silently keeping only the objects read before it. + /// + /// `from_iop` cannot tell a pool that ended from a pool it could not + /// finish reading: both just stop. For a pool whose Working Set is not the + /// first object, an unreadable object anywhere before it makes the + /// lenient parse come back with no Working Set at all and no indication + /// why. Use this when that difference matters. + pub fn try_from_iop(data: I) -> Result + where + I: IntoIterator, + { + let mut op = Self::new(); + op.try_extend_with_iop(data)?; + Ok(op) + } + pub fn extend_with_iop(&mut self, data: I) where I: IntoIterator, { - let mut data = data.into_iter(); + // Lenient by design: keep whatever was readable, as this always has. + let _ = self.try_extend_with_iop(data); + } + + /// Strict counterpart of [`extend_with_iop`](Self::extend_with_iop): stops + /// at, and returns, the first object that cannot be read. Objects read + /// before the failure have already been added. + pub fn try_extend_with_iop(&mut self, data: I) -> Result<(), ParseError> + where + I: IntoIterator, + { + let mut data = data.into_iter().peekable(); - while let Ok(o) = Object::read(&mut data) { + // Running out of data *between* objects is the normal end of a pool; + // running out inside one is a truncated object and is reported. + while data.peek().is_some() { + let o = Object::read(&mut data)?; // By the standard, if there already is an object with the same ID, the new object // replaces the old one self.objects.retain(|x| x.id() != o.id()); self.objects.push(o); } + Ok(()) } pub fn as_iop(&self) -> Vec { diff --git a/src/object_pool/reader.rs b/src/object_pool/reader.rs index 84feec5..08194f1 100644 --- a/src/object_pool/reader.rs +++ b/src/object_pool/reader.rs @@ -497,7 +497,7 @@ impl Object { id, background_colour: Self::read_u8(data)?.into(), width: Self::read_u16(data)?, - foreground_colour: Self::read_u16(data)?.try_into()?, + foreground_colour: Self::read_u16(data)?.into(), variable_reference: Self::read_u16(data)?.into(), value: Self::read_bool(data)?, enabled: Self::read_bool(data)?, @@ -519,7 +519,7 @@ impl Object { width: Self::read_u16(data)?, height: Self::read_u16(data)?, background_colour: Self::read_u8(data)?.into(), - font_attributes: Self::read_u16(data)?.try_into()?, + font_attributes: Self::read_u16(data)?.into(), input_attributes: Self::read_u16(data)?.into(), options: Self::read_u8(data)?.into(), variable_reference: Self::read_u16(data)?.into(), @@ -544,7 +544,7 @@ impl Object { width: Self::read_u16(data)?, height: Self::read_u16(data)?, background_colour: Self::read_u8(data)?.into(), - font_attributes: Self::read_u16(data)?.try_into()?, + font_attributes: Self::read_u16(data)?.into(), options: Self::read_u8(data)?.into(), variable_reference: Self::read_u16(data)?.into(), value: Self::read_u32(data)?, @@ -599,7 +599,7 @@ impl Object { width: Self::read_u16(data)?, height: Self::read_u16(data)?, background_colour: Self::read_u8(data)?.into(), - font_attributes: Self::read_u16(data)?.try_into()?, + font_attributes: Self::read_u16(data)?.into(), options: Self::read_u8(data)?.into(), variable_reference: Self::read_u16(data)?.into(), justification: Self::read_u8(data)?.into(), @@ -622,7 +622,7 @@ impl Object { width: Self::read_u16(data)?, height: Self::read_u16(data)?, background_colour: Self::read_u8(data)?.into(), - font_attributes: Self::read_u16(data)?.try_into()?, + font_attributes: Self::read_u16(data)?.into(), options: Self::read_u8(data)?.into(), variable_reference: Self::read_u16(data)?.into(), value: Self::read_u32(data)?, @@ -646,7 +646,7 @@ impl Object { ) -> Result { let mut o = OutputLine { id, - line_attributes: Self::read_u16(data)?.try_into()?, + line_attributes: Self::read_u16(data)?.into(), width: Self::read_u16(data)?, height: Self::read_u16(data)?, line_direction: Self::read_u8(data)?.into(), @@ -665,7 +665,7 @@ impl Object { ) -> Result { let mut o = OutputRectangle { id, - line_attributes: Self::read_u16(data)?.try_into()?, + line_attributes: Self::read_u16(data)?.into(), width: Self::read_u16(data)?, height: Self::read_u16(data)?, line_suppression: Self::read_u8(data)?, @@ -685,7 +685,7 @@ impl Object { ) -> Result { let mut o = OutputEllipse { id, - line_attributes: Self::read_u16(data)?.try_into()?, + line_attributes: Self::read_u16(data)?.into(), width: Self::read_u16(data)?, height: Self::read_u16(data)?, ellipse_type: Self::read_u8(data)?, @@ -709,7 +709,7 @@ impl Object { id, width: Self::read_u16(data)?, height: Self::read_u16(data)?, - line_attributes: Self::read_u16(data)?.try_into()?, + line_attributes: Self::read_u16(data)?.into(), fill_attributes: Self::read_u16(data)?.into(), polygon_type: Self::read_u8(data)?, points: Vec::with_capacity(Self::read_u8(data)?.into()), @@ -1325,7 +1325,7 @@ mod tests { use crate::object_pool::object::{Object, WorkingSet}; use crate::object_pool::object_attributes::{ObjectRef, Point}; use crate::object_pool::object_id::ObjectId; - use crate::object_pool::{Colour, ObjectPool, ObjectType}; + use crate::object_pool::{Colour, NullableObjectId, ObjectPool, ObjectType, ParseError}; use std::vec::IntoIter; fn read_id_type(data: &mut dyn Iterator) -> ObjectId { @@ -1382,7 +1382,7 @@ mod tests { let _working_set_exp = WorkingSet { id, - background_colour: Colour::new_by_id(0xF0), + background_colour: Colour::new_by_id(0xF0).into(), selectable: true, active_mask: ObjectId::default(), object_refs: vec![ @@ -1401,4 +1401,118 @@ mod tests { assert_eq!(*_working_set_act, _working_set_exp); } + + // ISO 11783-6 allows the Line Attributes reference of the output shape + // objects, the Font Attributes reference of the string/number objects and + // the Foreground Colour reference of Input Boolean to be the NULL object + // ID (0xFFFF). Real pools use it: a John Deere StarFire receiver's pool + // nulls line attributes on 213 shapes and foreground colour on every one + // of its Input Booleans. Rejecting it made `Object::read` fail partway + // through such pools. + + #[test] + fn read_output_line_with_null_line_attributes_test() { + let mut data: IntoIter = vec![ + 0x01, 0x00, // Object ID + 0x0D, // Type: Output Line + 0xFF, 0xFF, // Line attributes: NULL + 0x70, 0x00, // Width + 0x01, 0x00, // Height + 0x00, // Line direction + 0x00, // Number of macro references + ] + .into_iter(); + + match Object::read(&mut data) { + Ok(Object::OutputLine(o)) => assert_eq!(o.line_attributes, NullableObjectId::NULL), + other => panic!("expected an Output Line, got {:?}", other), + } + } + + #[test] + fn read_output_string_with_null_font_attributes_test() { + let mut data: IntoIter = vec![ + 0x02, 0x00, // Object ID + 0x0B, // Type: Output String + 0x10, 0x00, // Width + 0x08, 0x00, // Height + 0x01, // Background colour + 0xFF, 0xFF, // Font attributes: NULL + 0x00, // Options + 0xFF, 0xFF, // Variable reference: NULL + 0x00, // Justification + 0x00, 0x00, // Length + 0x00, // Number of macro references + ] + .into_iter(); + + match Object::read(&mut data) { + Ok(Object::OutputString(o)) => assert_eq!(o.font_attributes, NullableObjectId::NULL), + other => panic!("expected an Output String, got {:?}", other), + } + } + + #[test] + fn read_input_boolean_with_null_foreground_colour_test() { + let mut data: IntoIter = vec![ + 0x03, 0x00, // Object ID + 0x07, // Type: Input Boolean + 0x01, // Background colour + 0x10, 0x00, // Width + 0xFF, 0xFF, // Foreground colour: NULL + 0xFF, 0xFF, // Variable reference: NULL + 0x00, // Value + 0x01, // Enabled + 0x00, // Number of macro references + ] + .into_iter(); + + match Object::read(&mut data) { + Ok(Object::InputBoolean(o)) => assert_eq!(o.foreground_colour, NullableObjectId::NULL), + other => panic!("expected an Input Boolean, got {:?}", other), + } + } + + // The pool-level consequence of the above: `from_iop` stops at the first + // object it cannot read and keeps what came before, so one NULL reference + // early in a pool used to drop every object after it -- including the + // Working Set when a vendor places it last, as John Deere does. + #[test] + fn from_iop_keeps_objects_after_null_line_attributes_test() { + let iop: Vec = vec![ + // Output Line, id 1, line attributes NULL + 0x01, 0x00, 0x0D, 0xFF, 0xFF, 0x70, 0x00, 0x01, 0x00, 0x00, 0x00, + // Working Set, id 0, active mask 2, one language "en" + 0x00, 0x00, 0x00, 0xF0, 0x01, 0x02, 0x00, 0x00, 0x00, 0x01, b'e', b'n', + // Data Mask, id 2, no soft key mask + 0x02, 0x00, 0x01, 0xF0, 0xFF, 0xFF, 0x00, 0x00, + ]; + + let pool = ObjectPool::from_iop(iop.clone()); + + assert_eq!(pool.objects().len(), 3); + assert!(pool.working_set_object().is_some()); + assert_eq!( + pool.as_iop(), + iop, + "NULL references must round-trip as 0xFFFF" + ); + + let strict = ObjectPool::try_from_iop(iop) + .unwrap_or_else(|why| panic!("strict parse should succeed: {:?}", why)); + assert_eq!(strict.objects().len(), 3); + } + + #[test] + fn try_from_iop_reports_truncated_object_test() { + // An Output Line cut off after its type byte. + let iop: Vec = vec![0x01, 0x00, 0x0D]; + + assert!(matches!( + ObjectPool::try_from_iop(iop.clone()), + Err(ParseError::DataEmpty) + )); + // The lenient reader keeps its existing behaviour: no objects, no error. + assert_eq!(ObjectPool::from_iop(iop).objects().len(), 0); + } }