Allow uploading of images in statuses#3768
Allow uploading of images in statuses#3768iangreenleaf wants to merge 35 commits intobookwyrm-social:mainfrom
Conversation
markdown.py had parsing errors that were throwing off img srcsets.
It doesn't hugely matter one way or another, but the existing tests all expect no newline, so why rock the boat.
This is all that's needed to allow basic image insertion via markdown.
Also makes sure to save resized files with correct extension
markdown.py had parsing errors that were throwing off img srcsets.
1. File extension wasn't coming through to versions 2. Srcset errors prevented it from working correctly
ilkka-ollakka
left a comment
ilkka-ollakka
left a comment
There was a problem hiding this comment.
I think overall it looks ok, few comments here and there.
Biggest issue I see, is that uploads are not linked to statuses where they are uploaded to, so I don't see clear flow how those are cleaned up if nothing links to those uploads anymore.
| }); | ||
| } | ||
|
|
||
| uploadFile(file, target) { |
There was a problem hiding this comment.
I think this could also check filesize. API/nginx will give error if file is too big, but not sure this codeflow gives reasonable explanation to the user in that case?
One example can be found in https://github.com/bookwyrm-social/bookwyrm/pull/3627/files how bookwyrm.js uses form dataset to check upload file-sizes.
There was a problem hiding this comment.
@ilkka-ollakka I think I've addressed all your feedback except this item. I got stuck here trying to figure out how to pass DATA_UPLOAD_MAX_MEMORY_SIZE through to the front end. To replicate how it's done in that other PR, I think I'd have to pass it through the status form, and I couldn't figure out how to do that - too many layers of stuff I'm unfamiliar with.
There was a problem hiding this comment.
I'll take look if I can provide a way to solve that.
There was a problem hiding this comment.
I think you could solve the issue by adding the upload-limits to text-area as dataset attributes, then you can access that data via event.currentTarget.dataset ?
So no need to go via the example PR on widgets as there is not really upload form like that. Instead you could for example add simple_tag to templatetags/utilities.py for adding data-set attributes with defined limits and use that tag in text-area where you defined the droppable-textfield ?
Not sure if it is the best way, but atleast it could be straightforward way to do it.
There was a problem hiding this comment.
Works for me! Thanks for the help.
|
Thanks @ilkka-ollakka, I will go through your comments in detail once the holidays are over and I'm back to my normal schedule, most likely early January. Regarding not tying the uploads to statuses, yes, that's a design decision I made that's worth discussing. I've seen two schools of thought regarding how to treat uploads. One is to tie them closely to a database record, as you're suggesting, and manage them as a sort of additional piece of data on that record. The other approach is to treat them as independent and sort of idempotent: every uploaded file goes in a big bucket and gets a unique identifier that's the only link it has to anything in the database. Typically in that second way of doing things, every uploaded file lives forever, even if not needed—I suspect this approach really only came into practice once storage was cheap enough to not worry about a bit of "waste". To align with the first approach instead of the second, I certainly could set up a join table to tie uploads to statuses. I think doing that might significantly increase the complexity of the front-end code, since we would need to figure out how to upload files at the same time as the status form is submitted, and tie each image to the correct place in the text of the status with some sort of placeholder for rendering. It's all a bit easier to handle when uploads happen prior to the creation of the status. Another option that's in between the two poles would be to create an optional join table to statuses. That would allow me to keep the current flow for uploading immediately, but would give server admins the option to do some sort of clean-up action that deletes any images older than some threshold that aren't tied to a status. |
|
Thanks for joining us @iangreenleaf! This is a cool idea. I'm wondering how attached you are to images uploads being a separate model, and if so can you explain a little about why? The usual way we'd do this would be to add a file field to an existing model (in this case, probably BookStatus). Then you'd add that field to ReviewForm, That partially solves the issue @ilkka-ollakka has noted with orphaned images (though Django has a few issues with cleaning up files generally). The downside of that is that they're not "embedded" in the post with markdown, and would have to be added like we do with book covers which I suppose one might argue is a little clunky. But the upside is that the image/s are understood by the Django backend as being part of that status, and the code is easier to understand and maintain because it works in a "Django-like" way. When posts are pushed out as ActivityPub the images will be attachments rather than inline images. |
|
@hughrun I'm quite attached to the idea of the images being embedded in the markdown and thus being able to be located at specific points in the text. That is necessary to what I want out of this feature and fits with the perspective of treating book statuses more like a rich-text "article" and less like a microblogging "status". I'm not terribly attached to any of the particulars of how the code makes that functionality possible. If we can accomplish it while managing the files as part of the Django model, that would be fine with me - though as I mentioned in my previous comment I was a little apprehensive about complicating the code by going that route. |
|
Ok there are a few options here. I think we can probably get the best of both worlds with some care and not a huge amount of complication. The biggest issue I see at this point is that currently only @mouse-reeve do you have any thoughts on any of this? |
|
I would be okay with having "inline" images limited to Reviews - that's the only place that it really feels like it makes sense to me. And maybe the other statuses actually would make sense with attached images; you could post a "started reading" status with a photo of you with your book and mug of hot cocoa or something. I would want some advice on architecting it if we went that route, as I'm still new to the Bookwyrm codebase. I plugged it into all statuses just because that was the obvious and straightforward place to do it. |
|
No problem at all @iangreenleaf. My own first PR for BookWyrm was my first time working with Django and ended up being a bit more complicated than I anticipated, so I empathise with the feeling of not quite knowing how everything fits together (also, BookWyrm is kinda complex because we have to consider ActivityPub as well). If you give me a few days I'll think about what might be the most straightforward way to do this. I don't think you'll need to change too much, it's mostly how we handle saving images, and the ActivityPub aspect might need some additional changes. As @ilkka-ollakka noted, there are some methods already available for image handling that we can probably plug into as well. |
hughrun
left a comment
hughrun
left a comment
There was a problem hiding this comment.
@iangreenleaf Now that I've had a proper look I can see what you're doing here. I haven't had time to do an actual test run of this yet but it generally looks good.
Uploads are tied to the status when the status is created. The uploads have been created while the message was drafted, without the foreign key.
This started failing in a test - unsure why TBH, but the new version is better documented and should work well.
|
@iangreenleaf let us know when you're ready for this to be looked at again :) |
|
@hughrun @ilkka-ollakka I believe I've addressed all the feedback and this is ready for another look! |
3 participants
Description
Allows images to be uploaded and displayed inline in reviews. I wanted the ability to do this (I'm reviewing picture books) so I set out to make it happen. I tried to solve this in a clean and self-contained way while also making it flexible enough to be powerful, so this is what I came up with:
<img>elements and a few attributes through the HTML sanitizer. With this change alone, it's technically possible for people to insert images uploaded elsewhere into their statuses.!imagemarkdown helper that integrates with theUserUploadclass to display responsive images by making use ofsrcsetto provide all the available sizes of image.!imagehelper at the cursor.End result:
A couple notes:
What type of Pull Request is this?
Does this PR change settings or dependencies, or break something?
Details of breaking or configuration changes (if any of above checked)
New
UPLOAD_IMAGE_SIZES.env value that controls what sizes of images will be generated.Documentation
I'm not sure if any documentation is needed here, let me know.
Tests