New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audiobook play time #2551
base: main
Are you sure you want to change the base?
Audiobook play time #2551
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great overall, and is an excellent addition. I left some comments, most importantly that I think we should avoid storing loosely-formatted dates directly in the database, and interpret those at input time instead. I also had a minor observation around breakage for users without Javascript. The remaining feedback is mostly style preferences and is not crucial.
Thanks for making this happen!
bookwyrm/activitypub/book.py
Outdated
@@ -59,6 +59,7 @@ class Edition(Book): | |||
isbn13: str = "" | |||
oclcNumber: str = "" | |||
pages: int = None | |||
audiobookPlayTime: str = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of encoding audiobookPlayTime
as an integer (seconds or minutes), or even better a time interval and interpreting it accordingly on the frontend? As a general principle, I find it's best if the data stored in the database is as unambiguous and as close to "ground truth" as possible, with any interpretation being done clientside.
More briefly: I find it leads to less headaches if you are permissive on input, and strict on what you store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big question is: how will it federate? My transform (* 60 to the value at clean() time) would multiply an already correct value, coming from another server, and so on. Do you know how I can prevent that? Is it by making an activitypub-aware field with field_to_activity
and field_from_activity
transforms? That's a part I really don't know in the codebase.
Thank you for the review @cincodenada :) I agree with you about storing badly formatted data in the database, and had first tried with an ArrayField, which didn’t work too well. And somehow I had overlooked the time interval field type, made exactly for this type of data 🤦 As for the JS: you’re right, I forgot about non-JS users! What would you think of a CSS utility that’s like |
Does this need anything besides an update from |
@joachimesque Are you still able and willing to work on this? Please let us know - thank you! |
@mouse-reeve @jaschaurbach I updated from From what I can remember, my question about federation/ActivityPub serialization has not been tested. As I don't know this part of the logic very well, I'll ask for your help :) |
{% if format_property %} | ||
<meta itemprop="bookFormat" content="{{ format_property }}"> | ||
{% if book.physical_format == "AudiobookFormat" and book.audiobook_play_time %} | ||
<meta itemprop="timeRequired" content="{{ book.audiobook_play_time|iso_duration }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested this, it looks like this line was passing in a string-ified version of the duration (for the arbitrary numbers I tried, 85 days, 17:36:00
), which was throwing an error in the filter. I think that this is because I entered a big enough number that it went over hours and into days
@@ -299,6 +299,10 @@ class Edition(Book): | |||
max_length=255, choices=FormatChoices, null=True, blank=True | |||
) | |||
physical_format_detail = fields.CharField(max_length=255, blank=True, null=True) | |||
audiobook_play_time = models.DurationField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is calling a standard Django model field, rather than a custom BookWyrm activitypub model field, it's not going to be serialized in ActivityPub. To fix this, there needs to be a subclass of DurationField
in https://github.com/bookwyrm-social/bookwyrm/blob/main/bookwyrm/models/fields.py#L528 -- that code will handle parsing the string version of the play time into a DurationField.
I know this is kind of murky depths of how the activitypub implementation works --is that something you want to do, and if so does that give you enough of an idea of how to proceed? If you'd rather not, I can go ahead and make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joachimesque Have you seen this?
I believe the tests are just failing because this needs a merge migration |
Closes #2536
Closes #2941
This PR adds an audiobook duration field whenever the "Audiobook" choice is selected as a format
hh:mm
andminutes
formats.humanize
. Considering thatdjango.contrib.humanize
doesn't provide the support for durations, I chose to add it to the requirements, to avoid relying on a lot of localization code and translation keys.Happy holidays!