Skip to content

Conversation

@mickael-menu
Copy link
Member

@mickael-menu mickael-menu commented Jan 21, 2022

Deprecated

Shared

  • Link.toLocator() is deprecated as it may create an incorrect Locator if the link type is missing.
    • Use publication.locatorFromLink() instead.

See Slack discussion for context: https://readium.slack.com/archives/C703MSTQU/p1642520174001200

@mickael-menu mickael-menu requested a review from qnga January 21, 2022 15:01
override val currentLocator: StateFlow<Locator> get() = _currentLocator
private val _currentLocator = MutableStateFlow(initialLocator ?: publication.readingOrder.first().toLocator())
private val _currentLocator = MutableStateFlow(initialLocator
?: requireNotNull(publication.locatorFromLink(publication.readingOrder.first()))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link in the reading order must have a type, so locatorFromLink() will not return null.

* If there's no match, try again after removing any query parameter and anchor from the
* given [href].
*/
fun linkWithHref(href: String): Link? {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this from Publication, as it makes sense to have it in Manifest as well.

Comment on lines -87 to -91
// progression is mandatory in some contexts
if (it.locations.fragments.isEmpty())
it.copyWithLocations(progression = 0.0)
else
it
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made sure that locatorFromLink() returns a Locator with either progression or fragments set.

@mickael-menu mickael-menu requested a review from qnga January 24, 2022 09:54
Copy link
Member

@qnga qnga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants