feat: downscaled image previews for task attachments #2541

Merged
konrad merged 34 commits from Elscrux/vikunja:feature/downscaled-image-previews into main 2024-07-19 09:30:09 +00:00
Contributor

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?

As discussed in [https://community.vikunja.io/t/add-scaled-down-images-for-image-previews](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?
Elscrux requested review from konrad 2024-07-09 10:59:03 +00:00
Member

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!

Beep boop, I'm a bot.

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! > Beep boop, I'm a bot.
konrad requested changes 2024-07-10 09:15:41 +00:00
Dismissed
@ -188,0 +201,4 @@
return err
}
if !exists {
return caches.ErrCacheMiss
Owner

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.

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.
Author
Contributor

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.

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 https://kolaente.dev/Elscrux/vikunja/src/commit/25f4b9c7cf8ca311597a0448758d98a0bd636343/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)
Owner

This should be debug

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 {
Owner

Can you put this in a seperate function which also handles the caching?

Can you put this in a seperate function which also handles the caching?
konrad marked this conversation as resolved
@ -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)
Owner

Please check if the attachment we're dealing with is actually an image and not something totally different.

Please check if the attachment we're dealing with is actually an image and not something totally different.
Author
Contributor

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?

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?
Owner

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.

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.
Author
Contributor

Done

Done
konrad marked this conversation as resolved
@ -168,0 +197,4 @@
}
// Scale down the image to a minimum size
const minimumSize = 200
Owner

can we have different sizes? I wouldn't allow arbitrary sizing, but a scale like sm, md, lg, xl.

can we have different sizes? I wouldn't allow arbitrary sizing, but a scale like `sm`, `md`, `lg`, `xl`.
Author
Contributor

Sure, I'll add an extra query parameter for this. Any suggestions for the pixels behind the sizes?

Sure, I'll add an extra query parameter for this. Any suggestions for the pixels behind the sizes?
Owner

sm should roughly (round to nearest 100) be the size of the attachment preview in the task detail view, then double each iteration.

sm should roughly (round to nearest 100) be the size of the attachment preview in the task detail view, then double each iteration.
Author
Contributor

Good, that's how I did it

Good, that's how I did it
konrad marked this conversation as resolved
@ -168,0 +219,4 @@
return handler.HandleHTTPError(err, c)
}
// Write the bytes to a new file in memory
Owner

Why write the file again?

Why write the file again?
Author
Contributor

We need to put a file into taskAttachment.File.File. The previous file was just the original scale one.

We need to put a file into taskAttachment.File.File. The previous file was just the original scale one.
Owner

Why don't you just return the bytes from cache?

Why don't you just return the bytes from cache?
Author
Contributor

Unless I'm understanding it incorrectly, http.ServeContent doesn't accept bytes

Unless I'm understanding it incorrectly, `http.ServeContent` doesn't accept bytes
Owner

It does not, but you can use c.Blob instead.

See

return c.Blob(http.StatusOK, mimeType, a)

It does not, but you can use `c.Blob` instead. See https://kolaente.dev/vikunja/vikunja/src/commit/f2d90f31bcdb9ac0db446b7397c667794c0a0181/pkg/routes/api/v1/avatar.go#L112
Author
Contributor

Good to know. This is changed now.

Good to know. This is changed now.
Author
Contributor

@konrad I've pushed some updates, most notably the new size options.

@konrad I've pushed some updates, most notably the new size options.
konrad requested changes 2024-07-10 21:37:58 +00:00
Dismissed
@ -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 {
Owner

This should be private.

This should be private.
Author
Contributor

Done

Done
konrad marked this conversation as resolved
@ -186,2 +197,4 @@
}
func CacheKeyForTaskAttachmentPreview(id int64) string {
return "task_attachment_preview_" + strconv.Itoa(int(id))
Owner

The Key should contain the size.

The Key should contain the size.
Author
Contributor

I based this on the avatar upload, I guess this should also be changed?

I based this on the avatar upload, I guess this should also be changed?
Author
Contributor

Done now

Done now
konrad marked this conversation as resolved
@ -188,0 +211,4 @@
return err
}
if !exists || cached[previewSize] == nil {
return caches.ErrCacheMiss
Owner

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.

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`.
Author
Contributor

Done

Done
konrad marked this conversation as resolved
@ -188,0 +234,4 @@
return nil
}
func GetSize(previewSize string) int {
Owner

Please make the size a custom Type with constants.

Please make the size a custom Type with constants.
Author
Contributor

I'm not sure what you mean. A map?

I'm not sure what you mean. A map?
Owner
Like this: https://kolaente.dev/vikunja/vikunja/src/branch/main/pkg/models/favorites.go#L27-L33
konrad marked this conversation as resolved
@ -188,0 +264,4 @@
return resizedImg
}
func (ta *TaskAttachment) GeneratePreview(previewSize string) error {
Owner

Please call this function SavePreviewToCache.

Please call this function `SavePreviewToCache`.
Author
Contributor

Done

Done
konrad marked this conversation as resolved
konrad requested changes 2024-07-14 21:01:26 +00:00
Dismissed
konrad left a comment
Owner

Please also update the swagger docs comment at the start of the web handler.

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') }}
Owner

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)

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)
Author
Contributor

done

done
konrad requested changes 2024-07-15 10:32:22 +00:00
Dismissed
konrad left a comment
Owner

Now this is mostly small things, already looking quite good!

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) {
Owner

Can you make the size accept undefined and then get rid of the preview boolean instead?

Like this:

getBlobUrl(model: IAttachment, previewSize: PREVIEW_SIZE | undefined = undefined) {

Then only check if previewSize is not undefined, then we'll only need one parameter.

Can you make the size accept `undefined` and then get rid of the `preview` boolean instead? Like this: ``` getBlobUrl(model: IAttachment, previewSize: PREVIEW_SIZE | undefined = undefined) { ``` 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"
Owner

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.

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.
Author
Contributor

I used the goimports command. Is there any way for this to be done manually or any reference of this convention?

I used the goimports command. Is there any way for this to be done manually or any reference of this convention?
Owner

The linter in the Pipeline enforces the convention, I wonder why it let this one through.

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)
Owner

Please use strconv.FormatInt.

Please use `strconv.FormatInt`.
Author
Contributor

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?

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?
Owner

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.

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.
Author
Contributor

Oh I see, thanks for the explanation!

Oh I see, thanks for the explanation!
Elscrux marked this conversation as resolved
@ -188,0 +196,4 @@
return "task_attachment_preview_" + strconv.Itoa(int(id)) + "_size_" + string(size)
}
func (ta *TaskAttachment) ReadPreview(previewSize PreviewSize) []byte {
Owner

If this will always only read a cached preview, that should be reflected in the function name. Something like GetPreviewFromCache.

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"
Owner

Please prefix these preview constants with Preview so that it's clear what they are about, since they can be used globally.

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 {
Owner

Please make this a type method

func (previewSize PreviewSize) GetNumericSize() int {

That way, it's immediately clear, what size we're talking about just from the function name.

Please make this a type method ```go func (previewSize PreviewSize) GetNumericSize() int { ``` That way, it's immediately clear, what size we're talking about just from the function name.
Author
Contributor

That's useful!

That's useful!
Elscrux marked this conversation as resolved
@ -188,0 +235,4 @@
}
func Resize(img image.Image, size int) *image.NRGBA {
x := img.Bounds().Size().X
Owner

Does this function have to be public?

Does this function have to be public?
Author
Contributor

Probably not - I'm just not used to naming convention based access modifiers yet

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 {
Owner

This function not only saves the preview, it also creates and returns it. Please reflect that in the function name.

This function not only saves the preview, it also creates and returns it. Please reflect that in the function name.
Author
Contributor

That name was your suggestion but I can change it 😜

That name was your suggestion but I can change it 😜
Owner

I did? Wow.

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."
Owner

This should mention the default and what the sizes mean.

This should mention the default and what the sizes mean.
Elscrux marked this conversation as resolved
@ -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") {
Owner

This should be strings.HasPrefix(taskAttachment.File.Mime, "image")

This should be `strings.HasPrefix(taskAttachment.File.Mime, "image")`
Elscrux marked this conversation as resolved
@ -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)
Owner

Doesn't the mime type change when the file is resized?

Doesn't the mime type change when the file is resized?
Author
Contributor

I changed it to the png mime type specifically which is the format the downscaled images use

I changed it to the png mime type specifically which is the format the downscaled images use
konrad reviewed 2024-07-15 21:49:33 +00:00
@ -14,6 +22,8 @@ export default class AttachmentService extends AbstractService<IAttachment> {
})
}
authStore = useAuthStore()
Owner

What is the auth store used for?

What is the auth store used for?
Author
Contributor

It's a leftover from the settings implementation. I'll remove it.

It's a leftover from the settings implementation. I'll remove it.
konrad marked this conversation as resolved
@ -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) {
Owner

Please add a default of undefined, right now it is required to set a value for size.

Please add a default of `undefined`, right now it is required to set a value for `size`.
Author
Contributor

This is using an optional parameter. It is not required to set size. https://www.typescripttutorial.net/typescript-tutorial/typescript-optional-parameters/

This is using an optional parameter. It is not required to set size. https://www.typescripttutorial.net/typescript-tutorial/typescript-optional-parameters/
Owner

TIL, thanks!

TIL, thanks!
konrad marked this conversation as resolved
konrad approved these changes 2024-07-19 07:03:06 +00:00
konrad left a comment
Owner

Looks good! Please resolve the conflicts and then we're ready to merge.

Looks good! Please resolve the conflicts and then we're ready to merge.
Elscrux force-pushed feature/downscaled-image-previews from 075196f7de to accf7c4db4 2024-07-19 08:52:15 +00:00 Compare
konrad changed title from Downscaled Image Previews to feat: downscaled image previews for task attachments 2024-07-19 09:10:10 +00:00
konrad scheduled this pull request to auto merge when all checks succeed 2024-07-19 09:11:19 +00:00
konrad canceled auto merging this pull request when all checks succeed 2024-07-19 09:11:55 +00:00
Owner

Lint failed:

/go/src/code.vikunja.io/api/frontend/src/components/tasks/partials/FilePreview.vue

  22:14  error  'PropType' is defined but never used  @typescript-eslint/no-unused-vars
Lint failed: ``` /go/src/code.vikunja.io/api/frontend/src/components/tasks/partials/FilePreview.vue 22:14 error 'PropType' is defined but never used @typescript-eslint/no-unused-vars
Elscrux added 1 commit 2024-07-19 09:13:12 +00:00
fix: lint
All checks were successful
continuous-integration/drone/pr Build is passing
9aff6c62de
konrad scheduled this pull request to auto merge when all checks succeed 2024-07-19 09:16:01 +00:00
konrad merged commit 75ce261f74 into main 2024-07-19 09:30:09 +00:00
Sign in to join this conversation.
No description provided.