Skip to content

Conversation

@pulsejet
Copy link
Member

Fixes a regression where the viewer cannot show any photos from collaborative albums, since these previews are not accessible over the core API.

I unfortunately introduced this while fixing another regression in #1409 😩
Any better ideas to do this? Using the source image directly isn't right because it might be huge (plus no support for HEIC/TIFF), so this seems like the better option.

@pulsejet pulsejet added the regression Regression of a previous working feature label Oct 19, 2022
@pulsejet pulsejet requested review from skjnldsv and szaimen October 19, 2022 02:08
Fixes a regression where the viewer cannot show any photos
from collaborative albums, since these previews are not
accessible over the core API.

Signed-off-by: Varun Patil <[email protected]>
@pulsejet pulsejet added the 3. to review Waiting for reviews label Oct 19, 2022
@pulsejet pulsejet added this to the Nextcloud 26 milestone Oct 19, 2022
if (isPublic()) {
return generateUrl(`/apps/files_sharing/publicpreview/${getToken()}?file=${encodeFilePath(filename)}&${searchParams}`)
}
if (filename.startsWith('/photos/')) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filename is the full dav path btw, including the collection. So for a normal file it looks like /files/user/... and /photos/user/... for collaborative albums.

@pulsejet

This comment was marked as resolved.

@szaimen

This comment was marked as resolved.

@szaimen szaimen requested a review from artonge October 20, 2022 22:04
@pulsejet

This comment was marked as resolved.

@skjnldsv
Copy link
Member

@pulsejet don't ping random devs please :)
I'll review it later

@pulsejet
Copy link
Member Author

Apologies. He authored the commit that got broken (a05dbf3) so I thought he might know better.

@jkellerer
Copy link

Would it make sense to address this problem in a way that callers can decide how to provide preview urls for special cases instead of hard-coding the logic?

E.g.:

OCA.Viewer.open({
    ...
    previewUrl: ({ fileid, public, width, height }) => {
        return !public && generateUrl(`/apps/photos/api/v1/preview/${fileid}?x=${width}&y=${height}`)
    },
    ...
})

getPreviewIfAny({ fileid, filename, hasPreview, davPath }) {
    const width = Math.floor(screen.width * devicePixelRatio)
    const height = Math.floor(screen.height * devicePixelRatio)
    const searchParams = `fileId=${fileid}`
        + `&x=${width}`
        + `&y=${height}`
        + '&a=true'

    if (hasPreview) {
        const args = { fileid, filename, width, height, public: isPublic(), token: getToken() }
        const url = typeof this.previewUrl == 'function' && this.previewUrl(args)
        if (typeof url == 'string') {
            return url
        }

        // TODO: find a nicer standard way of doing this?
        if (isPublic()) {
            return generateUrl(`/apps/files_sharing/publicpreview/${getToken()}?file=${encodeFilePath(filename)}&${searchParams}`)
        }
...

@skjnldsv
Copy link
Member

skjnldsv commented Nov 8, 2022

Just an update here.
Currently nothing is broken, it's just not idea as we load bigger pictures in photos.

I'm still not ok with adding custom app-oriented conditions in Viewer.
I'm still thinking about how to better approach this.

@pulsejet
Copy link
Member Author

pulsejet commented Nov 8, 2022

Currently nothing is broken

Do you mean nextcloud/photos#1440 is fixed? That's what this PR was meant to address.

I'm still not ok with adding custom app-oriented conditions in Viewer. I'm still thinking about how to better approach this.

Yeah it's not ideal at all. I just submitted this patch as a quick fix.

@skjnldsv
Copy link
Member

skjnldsv commented Nov 9, 2022

@pulsejet if the source is provided, it should be rendered.
We fixed that in photos I think?

@artonge are we providing a source property for collaborative albums?

@pulsejet
Copy link
Member Author

pulsejet commented Dec 9, 2022

Currently nothing is broken, it's just not idea as we load bigger pictures in photos.

@skjnldsv @artonge I just want to point out that things are broken (this is latest stable25). Everyone doesn't use JPEG :)

ss1

Also closely related, with a lot of upvotes: nextcloud/photos#1452
There's no clean way to fix that one in viewer at all (had a go at it), since the token isn't exposed globally as a variable. So I agree this isn't the correct solution to the problem 👍🏻

ss2

@skjnldsv
Copy link
Member

Closing for #1525

@skjnldsv skjnldsv closed this Jan 24, 2023
@skjnldsv skjnldsv deleted the patch-collab branch September 14, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews regression Regression of a previous working feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants