Spine items rejecting property attributes

[ghost]

Related to: 9a9c8d8 where the 'rendition' prefix was disabled
The XML parsing runs fine, extracting the property string, but when creating the IRI, there's a whitelist for acceptable IRI prefixes it seems and the default has 'rendition' commented out. The proposed solution is to specifically enable this for SpineItem since it seems there are previous issues with enabling it in the default whitelist.

[danielweck]

Thank you for the PR.
I am trying to understand what problem this solves. Could you please provide an EPUB that exhibits a bug?

It has been a very long time since the commit you referenced, but ...
... if I remember correctly, the rendition prefix could not be included in the ReservedVocabularies (all GitHub permalinks reference the develop branch):

readium-sdk/ePub3/ePub/property_holder.cpp

Line 38 in a32d900

//{ "rendition", "http://www.idpf.org/vocab/rendition/#" }, CANNOT BE INCLUDED, BECAUSE OF OPF package@prefix attribute definition, see Package::InstallPrefixesFromAttributeValue

readium-sdk/ePub3/ePub/property_holder.cpp

Line 73 in a32d900

//__vmap_t("rendition", "http://www.idpf.org/vocab/rendition/#"), CANNOT BE INCLUDED, BECAUSE OF OPF package@prefix attribute definition, see Package::InstallPrefixesFromAttributeValue

... because of the InstallPrefixesFromAttributeValue() function potentially picking-up the rendition URI specified via the prefix attribute of the package OPF element:

readium-sdk/ePub3/ePub/package.cpp

Line 681 in a32d900

InstallPrefixesFromAttributeValue(val);

==>

readium-sdk/ePub3/ePub/package.cpp

Lines 1183 to 1215 in a32d900

	void Package::InstallPrefixesFromAttributeValue(const string& attrValue)
	{
	if ( attrValue.empty() )
	return;
	static REGEX_NS::regex::flag_type reflags(REGEX_NS::regex::ECMAScript|REGEX_NS::regex::optimize);
	static REGEX_NS::regex re("(\\w+):\\s*(.+?)(?:\\s+|$)", reflags);
	auto pos = REGEX_NS::sregex_iterator(attrValue.stl_str().begin(), attrValue.stl_str().end(), re);
	auto end = REGEX_NS::sregex_iterator();
	while ( pos != end )
	{
	if ( pos->size() == 3 ) // entire match plus two captures
	{
	if ( pos->str(1) == "_" )
	HandleError(EPUBError::OPFIllegalPrefixDefinition);
	if ( PropertyHolder::ReservedVocabularies.find(pos->str(1)) != PropertyHolder::ReservedVocabularies.end() )
	HandleError(EPUBError::OPFIllegalPrefixRedeclaration);
	for ( auto& pair : PropertyHolder::ReservedVocabularies )
	{
	if ( pair.second == pos->str(2) )
	{
	HandleError(EPUBError::OPFIllegalVocabularyIRIRedefinition);
	break;
	}
	}
	RegisterPrefixIRIStem(pos->str(1), pos->str(2));
	}
	++pos;
	}
	}

... which invokes RegisterPrefixIRIStem() (which in turn potentially registers the rendition URI):

readium-sdk/ePub3/ePub/property_holder.cpp

Lines 306 to 313 in a32d900

	void PropertyHolder::RegisterPrefixIRIStem(const string &prefix, const string &iriStem)
	{
	auto found = _vocabularyLookup.find(prefix);
	if ( found == _vocabularyLookup.end() )
	{
	_vocabularyLookup[prefix] = iriStem;
	}
	}

There are indeed direct calls to RegisterPrefixIRIStem() (special built-in cases) in order to force the rendition URI registration (when not explicit in package@prefix), so that MakePropertyIRI() can correctly handle the registered _vocabularyLookup:

readium-sdk/ePub3/ePub/package.cpp

Line 443 in a32d900

this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#");

readium-sdk/ePub3/ePub/package.cpp

Line 460 in a32d900

this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#");

==>

readium-sdk/ePub3/ePub/property_holder.cpp

Lines 314 to 326 in a32d900

	IRI PropertyHolder::MakePropertyIRI(const string &reference, const string& prefix) const
	{
	auto found = _vocabularyLookup.find(prefix);
	if ( found == _vocabularyLookup.end() )
	{
	auto parent = _parent.lock();
	if ( parent )
	return parent->MakePropertyIRI(reference, prefix);
	return IRI();
	}
	return IRI(found->second + reference);
	}

So, it seems you are trying to solve / have solved a similar issue for SpineItem:

https://github.com/kyles-ep/readium-sdk/blob/fac65be973beb86e179a7b9ab464150ce71dcece/ePub3/ePub/spine.cpp#L39-L42
( from your forked master branch https://github.com/kyles-ep/readium-sdk/blob/master/ePub3/ePub/spine.cpp )

Can you please explain why SpineItem in particular? Do you think the problem exists somewhere else? (bearing in mind, I am not sure what the "problem" is exactly :)

Thank you for clarifying, and sorry if I am missing an obvious point (it has been a long time since I looked at this C++ codebase)

PS: https://idpf.github.io/epub-vocabs/rendition/#sec-ref

[danielweck]

PS: is @evidentpoint using / planning to use a modified version of readium-sdk, with bug fixes, improvements, API changes, etc.?

[ghost]

Apologies for not elaborating on the problem.
I don't know if I can share the EPUB itself, so I'll provide the relevant bits (please let me know if it's sufficient).

The problem is that this document has spine items with "rendition" properties:

<itemref idref="asdf-1" properties="rendition:layout-pre-paginated rendition:spread-landscape rendition:page-spread-center"/>

and the package metadata element has the following children:

<meta property="rendition:layout">reflowable</meta>
<meta property="rendition:orientation">auto</meta>

From what I understand, spine items are allowed to override the rendition properties specified in the package metadata, and as it is, there's a bug that prevents this property from being picked up.
I admit I'm not the most familiar with EPUB, so perhaps there are other places where such a property needs to be picked up?

[jccr]

PS: is @evidentpoint using / planning to use a modified version of readium-sdk, with bug fixes, improvements, API changes, etc.?

A request came our way that asked us to look into fixing a bug with the readium-sdk. My colleague Kyle is investigating. Evident Point doesn't have plans that go beyond helping out with maintenance and bug fixing.

[ghost]

Sorry. I thought I could change both source and destination of the PR.
@jccr Suggests this should be merged into develop instead of master. Perhaps we should close this and open a new one with proper targeting?

[danielweck]

I took the liberty to change the PR target branch from master to develop.

[danielweck]

You explanation makes sense Kyle, thanks. I am surprised this bug hasn't surfaced before. <itemref properties="rendition:xxx" ... /> is not an uncommon construct in the EPUB test books typically used to validate Readium implementations.

Wouldn't it make more sense to invoke this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#"); immediately after InstallPrefixesFromAttributeValue() is called? (see my full description above)
This way, if the OPF prefix definition already contains "rendition", then RegisterPrefixIRIStem() simply overrides it with the default URI defined in the EPUB3 specification ( https://idpf.github.io/epub-vocabs/rendition/#sec-ref ).
What do you think? Could you please give this a try, by also removing the two other instances in
readium-sdk/ePub3/ePub/package.cpp where this->RegisterPrefixIRIStem("rendition", "http://www.idpf.org/vocab/rendition/#"); is invoked directly (again, details in my comment above).
Thanks!