Audiobook play time #2551
Audiobook play time #2551
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
@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:mmandminutesformats.humanize. Considering thatdjango.contrib.humanizedoesn'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!