Clarify what id means in the VerificationNote.

5 files changed, 52 insertions, 33 deletions

diff --git a/src/dcp.cc b/src/dcp.cc
index a4ac3eb3..570ccfcc 100644
--- a/src/dcp.cc
+++ b/src/dcp.cc
@@ -268,7 +268,7 @@ DCP::read (vector<dcp::VerificationNote>* notes, bool ignore_incorrect_picture_m
 			bool found_threed_marked_as_twod = false;
 			auto asset = asset_factory(path, ignore_incorrect_picture_mxf_type, &found_threed_marked_as_twod);
 			if (asset->id() != id) {
-				notes->push_back(VerificationNote(VerificationNote::Type::ERROR, VerificationNote::Code::MISMATCHED_ASSET_MAP_ID).set_id(id).set_other_id(asset->id()));
+				notes->push_back(VerificationNote(VerificationNote::Type::ERROR, VerificationNote::Code::MISMATCHED_ASSET_MAP_ID).set_asset_id(id).set_other_asset_id(asset->id()));
 			}
 			other_assets.push_back(asset);
 			if (found_threed_marked_as_twod && notes) {
diff --git a/src/verify.cc b/src/verify.cc
index 0d542de3..fcea8ac4 100644
--- a/src/verify.cc
+++ b/src/verify.cc
@@ -895,7 +895,7 @@ verify_text_details (
 	std::function<bool (shared_ptr<Reel>)> check,
 	std::function<optional<string> (shared_ptr<Reel>)> xml,
 	std::function<int64_t (shared_ptr<Reel>)> duration,
-	std::function<std::string (shared_ptr<Reel>)> id
+	std::function<std::string (shared_ptr<Reel>)> asset_id
 	)
 {
 	/* end of last subtitle (in editable units) */
@@ -1026,7 +1026,7 @@ verify_text_details (
 		reel_offset = end;
 
 		if (context.dcp->standard() && *context.dcp->standard() == dcp::Standard::SMPTE && has_text && font_ids.empty()) {
-			context.add_note(dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, VerificationNote::Code::MISSING_LOAD_FONT).set_id(id(reels[i])));
+			context.add_note(dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, VerificationNote::Code::MISSING_LOAD_FONT).set_asset_id(asset_id(reels[i])));
 		}
 	}
 
@@ -1059,7 +1059,11 @@ verify_text_details (
 	}
 
 	if (missing_load_font_id) {
-		context.add_note(dcp::VerificationNote(VerificationNote::Type::ERROR, VerificationNote::Code::MISSING_LOAD_FONT_FOR_FONT).set_id(*missing_load_font_id));
+		context.add_note(
+			dcp::VerificationNote(
+				VerificationNote::Type::ERROR,
+				VerificationNote::Code::MISSING_LOAD_FONT_FOR_FONT
+			).set_load_font_id(*missing_load_font_id));
 	}
 }
 
@@ -2183,11 +2187,11 @@ dcp::note_to_string(VerificationNote note, function<string (string)> process_str
 	case VerificationNote::Code::INCORRECT_SUBTITLE_NAMESPACE_COUNT:
 		return compose("The XML in the subtitle asset %1 has more than one namespace declaration.", note.note().get());
 	case VerificationNote::Code::MISSING_LOAD_FONT_FOR_FONT:
-		return compose("A subtitle or closed caption refers to a font with ID %1 that does not have a corresponding <LoadFont> node", note.id().get());
+		return compose("A subtitle or closed caption refers to a font with ID %1 that does not have a corresponding <LoadFont> node", note.load_font_id().get());
 	case VerificationNote::Code::MISSING_LOAD_FONT:
-		return compose("The SMPTE subtitle asset %1 has <Text> nodes but no <LoadFont> node", note.id().get());
+		return compose("The SMPTE subtitle asset %1 has <Text> nodes but no <LoadFont> node", note.asset_id().get());
 	case VerificationNote::Code::MISMATCHED_ASSET_MAP_ID:
-		return compose("The asset with ID %1 in the asset map actually has an id of %2", note.id().get(), note.other_id().get());
+		return compose("The asset with ID %1 in the asset map actually has an id of %2", note.asset_id().get(), note.other_asset_id().get());
 	case VerificationNote::Code::EMPTY_CONTENT_VERSION_LABEL_TEXT:
 		return compose("The <LabelText> in a <ContentVersion> in CPL %1 is empty", note.cpl_id().get());
 	case VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT:
@@ -2215,8 +2219,9 @@ dcp::operator== (dcp::VerificationNote const& a, dcp::VerificationNote const& b)
 		a.frame() == b.frame() &&
 		a.component() == b.component() &&
 		a.size() == b.size() &&
-		a.id() == b.id() &&
-		a.other_id() == b.other_id() &&
+		a.load_font_id() == b.load_font_id() &&
+		a.asset_id() == b.asset_id() &&
+		a.other_asset_id() == b.other_asset_id() &&
 		a.frame_rate() == b.frame_rate() &&
 		a.cpl_id() == b.cpl_id() &&
 		a.reference_hash() == b.reference_hash() &&
@@ -2267,12 +2272,16 @@ dcp::operator< (dcp::VerificationNote const& a, dcp::VerificationNote const& b)
 		return a.size().get_value_or(0) < b.size().get_value_or(0);
 	}
 
-	if (a.id() != b.id()) {
-		return a.id().get_value_or("") < b.id().get_value_or("");
+	if (a.load_font_id() < b.load_font_id()) {
+		return a.load_font_id().get_value_or("") < b.load_font_id().get_value_or("");
 	}
 
-	if (a.other_id() != b.other_id()) {
-		return a.other_id().get_value_or("") < b.other_id().get_value_or("");
+	if (a.asset_id() != b.asset_id()) {
+		return a.asset_id().get_value_or("") < b.asset_id().get_value_or("");
+	}
+
+	if (a.other_asset_id() != b.other_asset_id()) {
+		return a.other_asset_id().get_value_or("") < b.other_asset_id().get_value_or("");
 	}
 
 	if (a.cpl_id() != b.cpl_id()) {
diff --git a/src/verify.h b/src/verify.h
index 6867721b..57d23ddb 100644
--- a/src/verify.h
+++ b/src/verify.h
@@ -521,7 +521,7 @@ public:
 		INCORRECT_SUBTITLE_NAMESPACE_COUNT,
 		/** A subtitle or closed caption file has a _<Font>_ tag which refers to a font that is not
 		 *  first introduced with a <LoadFont>.
-		 *  id contains the ID of the <Font> tag.
+		 *  load_font_id contains the ID of the <Font> tag.
 		 */
 		MISSING_LOAD_FONT_FOR_FONT,
 		/** A SMPTE subtitle asset has at least one _<Text>_ element but no <LoadFont>
@@ -608,8 +608,9 @@ private:
 		FRAME,
 		COMPONENT,
 		SIZE,
-		ID,
-		OTHER_ID,
+		LOAD_FONT_ID,
+		ASSET_ID,
+		OTHER_ASSET_ID,
 		FRAME_RATE,
 		CPL_ID,
 		CALCULATED_HASH,
@@ -667,22 +668,31 @@ public:
 		return data<int>(Data::SIZE);
 	}
 
-	VerificationNote& set_id(std::string id) {
-		_data[Data::ID] = id;
+	VerificationNote& set_load_font_id(std::string id) {
+		_data[Data::LOAD_FONT_ID] = id;
 		return *this;
 	}
 
-	boost::optional<std::string> id() const {
-		return data<std::string>(Data::ID);
+	boost::optional<std::string> load_font_id() const {
+		return data<std::string>(Data::LOAD_FONT_ID);
 	}
 
-	VerificationNote& set_other_id(std::string other_id) {
-		_data[Data::OTHER_ID] = other_id;
+	VerificationNote& set_asset_id(std::string id) {
+		_data[Data::ASSET_ID] = id;
 		return *this;
 	}
 
-	boost::optional<std::string> other_id() const {
-		return data<std::string>(Data::OTHER_ID);
+	boost::optional<std::string> asset_id() const {
+		return data<std::string>(Data::ASSET_ID);
+	}
+
+	VerificationNote& set_other_asset_id(std::string other_asset_id) {
+		_data[Data::OTHER_ASSET_ID] = other_asset_id;
+		return *this;
+	}
+
+	boost::optional<std::string> other_asset_id() const {
+		return data<std::string>(Data::OTHER_ASSET_ID);
 	}
 
 	VerificationNote& set_frame_rate(int frame_rate) {
diff --git a/src/verify_internal.h b/src/verify_internal.h
index 33ab873d..3e16937e 100644
--- a/src/verify_internal.h
+++ b/src/verify_internal.h
@@ -126,7 +126,7 @@ public:
 			note.set_reel_index(*reel_index);
 		}
 		if (asset_id) {
-			note.set_id(*asset_id);
+			note.set_asset_id(*asset_id);
 		}
 		notes.push_back(std::move(note));
 	}
diff --git a/test/verify_test.cc b/test/verify_test.cc
index eaf93cc3..aac245b2 100644
--- a/test/verify_test.cc
+++ b/test/verify_test.cc
@@ -253,8 +253,8 @@ to_string(dcp::VerificationNote const& note)
 
 	s += dcp::String::compose(
 		"%1 %2 %3 %4 %5 %6]\n",
-		note.id().get_value_or("<none>"),
-		note.other_id().get_value_or("<none>"),
+		note.asset_id().get_value_or("<none>"),
+		note.other_asset_id().get_value_or("<none>"),
 		note.cpl_id().get_value_or("<none>"),
 		note.reference_hash().get_value_or("<none>"),
 		note.calculated_hash().get_value_or("<none>"),
@@ -5246,7 +5246,7 @@ BOOST_AUTO_TEST_CASE(verify_invalid_main_picture_active_area_1)
 				).set_cpl_id(cpl->id()),
 			dcp::VerificationNote(
 				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_MAIN_PICTURE_ACTIVE_AREA, "height 4080 is bigger than the asset height 1080", canonical(find_cpl(dir))
-				).set_cpl_id(cpl->id()).set_id(picture_id).set_reel_index(0),
+				).set_cpl_id(cpl->id()).set_asset_id(picture_id).set_reel_index(0),
 		});
 }
 
@@ -5292,10 +5292,10 @@ BOOST_AUTO_TEST_CASE(verify_invalid_main_picture_active_area_2)
 				).set_cpl_id(cpl->id()),
 			dcp::VerificationNote(
 				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_MAIN_PICTURE_ACTIVE_AREA, "width 9900 is bigger than the asset width 1998", canonical(find_cpl(dir))
-				).set_cpl_id(cpl->id()).set_id(picture_id).set_reel_index(0),
+				).set_cpl_id(cpl->id()).set_asset_id(picture_id).set_reel_index(0),
 			dcp::VerificationNote(
 				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_MAIN_PICTURE_ACTIVE_AREA, "height 5125 is bigger than the asset height 1080", canonical(find_cpl(dir))
-				).set_cpl_id(cpl->id()).set_id(picture_id).set_reel_index(0),
+				).set_cpl_id(cpl->id()).set_asset_id(picture_id).set_reel_index(0),
 		});
 }
 
@@ -5707,7 +5707,7 @@ BOOST_AUTO_TEST_CASE(verify_missing_load_font_for_font)
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_KIND, string{"trailer"}, cpl),
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
 			{ dcp::VerificationNote::Type::BV21_ERROR, dcp::VerificationNote::Code::INVALID_STANDARD },
-			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISSING_LOAD_FONT_FOR_FONT).set_id("theFontId").set_cpl_id(cpl->id())
+			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISSING_LOAD_FONT_FOR_FONT).set_load_font_id("theFontId").set_cpl_id(cpl->id())
 		});
 
 }
@@ -5770,7 +5770,7 @@ BOOST_AUTO_TEST_CASE(verify_missing_load_font)
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
 			ok(dcp::VerificationNote::Code::CORRECT_PICTURE_HASH, canonical(dir / "video.mxf"), cpl).set_reel_index(0),
 			ok(dcp::VerificationNote::Code::VALID_PICTURE_FRAME_SIZES_IN_BYTES, canonical(dir / "video.mxf"), cpl).set_reel_index(0),
-			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISSING_LOAD_FONT).set_id(reel_subs->id()).set_cpl_id(cpl->id()).set_reel_index(0)
+			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISSING_LOAD_FONT).set_asset_id(reel_subs->id()).set_cpl_id(cpl->id()).set_reel_index(0)
 		});
 }
 
@@ -5809,7 +5809,7 @@ BOOST_AUTO_TEST_CASE(verify_spots_wrong_asset)
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_KIND, string{"trailer"}, cpl),
 			ok(dcp::VerificationNote::Code::VALID_CPL_ANNOTATION_TEXT, string{"A Test DCP"}, cpl),
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
-			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISMATCHED_ASSET_MAP_ID).set_id(asset_1).set_other_id(asset_2)
+			dcp::VerificationNote(dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::MISMATCHED_ASSET_MAP_ID).set_asset_id(asset_1).set_other_asset_id(asset_2)
 		});
 }