feat: downscaled image previews for task attachments #2541
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "Elscrux/vikunja:feature/downscaled-image-previews"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
As discussed in https://community.vikunja.io/t/add-scaled-down-images-for-image-previews this adds a query parameter in the task attachment request which returns a scaled down image for preview purposes to reduce network load and improve responsiveness.
Currently the minimum size for the smallest side (width/height) is hardcoded to 200px. Not sure this should be a config option or if this should just stay this way?
Hi Elscrux!
Thank you for creating a PR!
I've deployed the frontend changes of this PR on a preview environment under this URL: https://2541-feature-downscaled-image-preview--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.
This preview does not contain any changes made to the api, only the frontend.
Have a nice day!
@ -188,0 +201,4 @@
return err
}
if !exists {
return caches.ErrCacheMiss
Shouldn't this create and save the preview in that case? This looks like it will create the preview when it exists and returns an error when it does not.
This only reads it from the cache when it already exists. Creating and saving I've delayed until after the image was loaded regularly in the main part of the code
25f4b9c7cf/pkg/routes/api/v1/task_attachment.go (L192)
It's not perfect to have it separated like this, but without refactoring a lot and extracting reading the original file as a function this was the only way to provide an early exit when the preview is cached and generate the preview and cache it after loading the original file.
@ -156,0 +170,4 @@
if preview {
err := taskAttachment.ReadPreview()
if err == nil {
log.Infof("Cached attachment image preview found for task attachment %v", taskAttachment.ID)
This should be debug
@ -166,2 +190,4 @@
}
// If a preview is requested and the preview was not cached, we create the preview and cache it
if preview {
Can you put this in a seperate function which also handles the caching?
@ -167,1 +191,4 @@
// If a preview is requested and the preview was not cached, we create the preview and cache it
if preview {
img, _, err := image.Decode(taskAttachment.File.File)
Please check if the attachment we're dealing with is actually an image and not something totally different.
Currently that's inferred by the usage via the frontend. If you want to I can see how to check this on the backend too, or do you have an idea?
The api is not only used by the frontend.
The file has the mimetype saved in the database, please use that to check if the file is an image.
Done
@ -168,0 +197,4 @@
}
// Scale down the image to a minimum size
const minimumSize = 200
can we have different sizes? I wouldn't allow arbitrary sizing, but a scale like
sm
,md
,lg
,xl
.Sure, I'll add an extra query parameter for this. Any suggestions for the pixels behind the sizes?
sm should roughly (round to nearest 100) be the size of the attachment preview in the task detail view, then double each iteration.
Good, that's how I did it
@ -168,0 +219,4 @@
return handler.HandleHTTPError(err, c)
}
// Write the bytes to a new file in memory
Why write the file again?
We need to put a file into taskAttachment.File.File. The previous file was just the original scale one.
Why don't you just return the bytes from cache?
Unless I'm understanding it incorrectly,
http.ServeContent
doesn't accept bytesIt does not, but you can use
c.Blob
instead.See
Good to know. This is changed now.
@konrad I've pushed some updates, most notably the new size options.
@ -185,6 +196,133 @@ func (ta *TaskAttachment) ReadAll(s *xorm.Session, _ web.Auth, _ string, page in
return attachments, len(attachments), numberOfTotalItems, err
}
func CacheKeyForTaskAttachmentPreview(id int64) string {
This should be private.
Done
@ -186,2 +197,4 @@
}
func CacheKeyForTaskAttachmentPreview(id int64) string {
return "task_attachment_preview_" + strconv.Itoa(int(id))
The Key should contain the size.
I based this on the avatar upload, I guess this should also be changed?
Done now
@ -188,0 +211,4 @@
return err
}
if !exists || cached[previewSize] == nil {
return caches.ErrCacheMiss
This should be a custom error, not one from xorm. Or better, return the bytes from cache and if there is nothing in the cache, return
nil
.Done
@ -188,0 +234,4 @@
return nil
}
func GetSize(previewSize string) int {
Please make the size a custom Type with constants.
I'm not sure what you mean. A map?
Like this: https://kolaente.dev/vikunja/vikunja/src/branch/main/pkg/models/favorites.go#L27-L33
@ -188,0 +264,4 @@
return resizedImg
}
func (ta *TaskAttachment) GeneratePreview(previewSize string) error {
Please call this function
SavePreviewToCache
.Done
Please also update the swagger docs comment at the start of the web handler.
@ -44,1 +44,4 @@
</div>
<div class="field">
<label class="label">
{{ $t('user.settings.general.previewSize') }}
Please don't make this a setting. The user won't know what to choose and as such, the setting will just be confusing. Instead, load sm on attachment previews and md on the kanban card.
In the long run, this should be using srcset, but that's a bunch more complicated (pinging @dpschen for a better explanation)
done
Now this is mostly small things, already looking quite good!
@ -39,3 +49,2 @@
getBlobUrl(model: IAttachment) {
return AbstractService.prototype.getBlobUrl.call(this, '/tasks/' + model.taskId + '/attachments/' + model.id)
getBlobUrl(model: IAttachment, preview: boolean = false, size: PREVIEW_SIZE = PREVIEW_SIZE.SM) {
Can you make the size accept
undefined
and then get rid of thepreview
boolean instead?Like this:
Then only check if
previewSize
is not undefined, then we'll only need one parameter.@ -24,1 +27,4 @@
"code.vikunja.io/api/pkg/log"
"github.com/disintegration/imaging"
"code.vikunja.io/api/pkg/events"
Please sort the imports. First should be standard library, then Vikunja code (Everything starting with
code.vikunja.io/api/
, then all third-party libraries. Separated with a blank line.I used the goimports command. Is there any way for this to be done manually or any reference of this convention?
The linter in the Pipeline enforces the convention, I wonder why it let this one through.
@ -186,2 +193,4 @@
}
func cacheKeyForTaskAttachmentPreview(id int64, size PreviewSize) string {
return "task_attachment_preview_" + strconv.Itoa(int(id)) + "_size_" + string(size)
Please use
strconv.FormatInt
.How do you want to format it? It requires a base. I put in 10 for now although I'm not sure if there is any difference to the current way of doing it?
Base 10 is what you need.
You're casting a 64 bit int down to a 32 bit int which throws away half of the bytes. That will lead to an overflow if the number is large enough.
This is unlikely to be a problem here in this specific use case, but it's also the kind of tech debt that comes back to bite you hard when it becomes a problem.
Oh I see, thanks for the explanation!
@ -188,0 +196,4 @@
return "task_attachment_preview_" + strconv.Itoa(int(id)) + "_size_" + string(size)
}
func (ta *TaskAttachment) ReadPreview(previewSize PreviewSize) []byte {
If this will always only read a cached preview, that should be reflected in the function name. Something like
GetPreviewFromCache
.@ -188,0 +213,4 @@
type PreviewSize string
const (
Small PreviewSize = "sm"
Please prefix these preview constants with
Preview
so that it's clear what they are about, since they can be used globally.@ -188,0 +219,4 @@
ExtraLarge PreviewSize = "xl"
)
func GetSize(previewSize PreviewSize) int {
Please make this a type method
That way, it's immediately clear, what size we're talking about just from the function name.
That's useful!
@ -188,0 +235,4 @@
}
func Resize(img image.Image, size int) *image.NRGBA {
x := img.Bounds().Size().X
Does this function have to be public?
Probably not - I'm just not used to naming convention based access modifiers yet
@ -188,0 +249,4 @@
return resizedImg
}
func (ta *TaskAttachment) SavePreviewToCache(previewSize PreviewSize) []byte {
This function not only saves the preview, it also creates and returns it. Please reflect that in the function name.
That name was your suggestion but I can change it 😜
I did? Wow.
@ -116,2 +118,4 @@
// @Param id path int true "Task ID"
// @Param attachmentID path int true "Attachment ID"
// @Param preview query string false "If set to true, a preview image will be returned if the attachment is an image."
// @Param size query string false "The size of the preview image. Can be sm, md, lg or xl."
This should mention the default and what the sizes mean.
@ -156,0 +165,4 @@
}
// If the preview query parameter is set and the preview was already generated and cached, return the cached preview image
if preview && strings.Contains(taskAttachment.File.Mime, "image") {
This should be
strings.HasPrefix(taskAttachment.File.Mime, "image")
@ -156,0 +170,4 @@
if previewFileBytes != nil {
log.Debugf("Cached attachment image preview found for task attachment %v", taskAttachment.ID)
return c.Blob(http.StatusOK, taskAttachment.File.Mime, previewFileBytes)
Doesn't the mime type change when the file is resized?
I changed it to the png mime type specifically which is the format the downscaled images use
@ -14,6 +22,8 @@ export default class AttachmentService extends AbstractService<IAttachment> {
})
}
authStore = useAuthStore()
What is the auth store used for?
It's a leftover from the settings implementation. I'll remove it.
@ -39,3 +49,2 @@
getBlobUrl(model: IAttachment) {
return AbstractService.prototype.getBlobUrl.call(this, '/tasks/' + model.taskId + '/attachments/' + model.id)
getBlobUrl(model: IAttachment, size?: PREVIEW_SIZE) {
Please add a default of
undefined
, right now it is required to set a value forsize
.This is using an optional parameter. It is not required to set size. https://www.typescripttutorial.net/typescript-tutorial/typescript-optional-parameters/
TIL, thanks!
Looks good! Please resolve the conflicts and then we're ready to merge.
075196f7de
toaccf7c4db4
Downscaled Image Previewsto feat: downscaled image previews for task attachmentsLint failed: