Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use physical pixel size for preview resolution #899

Merged
merged 1 commit into from
Jun 5, 2021
Merged

Use physical pixel size for preview resolution #899

merged 1 commit into from
Jun 5, 2021

Conversation

connection-reset
Copy link
Contributor

Fixes #898

MDN has an example for <canvas> that I basically copied.
The apparent image size stays the same, zoom still works like before.

Does not watch for devicePixelRatio changes, eg when moving the browser window from a low DPI screen to a high DPI screen.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working labels May 17, 2021
@skjnldsv
Copy link
Member

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label May 17, 2021
@skjnldsv
Copy link
Member

/backport to stable20

@skjnldsv
Copy link
Member

/backport to stable19

beardhatcode
beardhatcode requested changes May 23, 2021
View reviewed changes
Copy link
Contributor

@beardhatcode beardhatcode left a comment

Choose a reason for hiding this comment

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

Hi @connection-reset , thanks for your contribution 👍 .

I looked at your patch, I think the code could be improved by extracting the Math.floors into variables. (Or even better: extract everything behind the ? into a variable)

@connection-reset
Copy link
Contributor Author

I made width and height into variables. I tried out moving the whole URL Params into a variable, it is actually worse.

@beardhatcode beardhatcode reopened this May 23, 2021
@beardhatcode
Copy link
Contributor

beardhatcode commented May 23, 2021 •
edited
Loading

@connection-reset i changed It to what I think it should be. (and accidentally closed the PR for a sec, sorry)

Do you think this is OK @connection-reset?

@connection-reset
Copy link
Contributor Author

That is fine by me, I care more about how the preview looks than how the code does.

@beardhatcode
Copy link
Contributor

beardhatcode commented May 23, 2021 •
edited
Loading

@skjnldsv could you squash and merge this if this is OK for you? (The failing test is the sidebar test )

(devicePixelRatio has existed in all browsers since 2011 apparently, so this shouldn't break in any browser except IE)

        Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✔  audio.mpeg.spec.js                       00:15        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  audio.ogg.spec.js                        00:13        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  audios.spec.js                           00:15        9        9        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  delete.spec.js                           00:19        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  download-share-disabled.spec.js          00:22        9        9        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  download-share.spec.js                   00:20       11       11        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  download.spec.js                         00:19        5        5        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  files-shares.spec.js                     00:22       26       26        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  files.spec.js                            00:08        2        2        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image-small.png.spec.js                  00:11        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image.gif.spec.js                        00:12        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image.heic.spec.js                       00:15        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image.png.spec.js                        00:17        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image.svg.spec.js                        00:11        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  image.webp.spec.js                       00:13        7        7        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  images-custom-list-loadmore.spec.js      00:17       18       18        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  images-custom-list.spec.js               00:16       14       14        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  images.spec.js                           00:16       18       18        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  oddname/oddname-audio.spec.js            00:44       30       30        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  oddname/oddname-image.spec.js            01:33       60       60        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  oddname/oddname-video.spec.js            01:14       50       50        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  oddname/oddname.js                         3ms        -        -        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✖  sidebar.spec.js                          02:48       14        7        7        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  video.mkv.spec.js                        00:12        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  video.mp4.spec.js                        00:12        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  video.ogv.spec.js                        00:12        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  video.webm.spec.js                       00:13        6        6        -        -        - │
  ├────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ ✔  videos.spec.js                           00:17        9        9        -        -        - │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✖  1 of 28 failed (4%)                      12:18      358      351        7        -        -

@szaimen
Copy link
Contributor

szaimen commented May 23, 2021 •
edited
Loading

I am interested to know what happens if you limit the max preview size to below your screens resolution. Will it fail to request the preview?
Also it will be interesting to know if it still works with those values applied:
https://github.com/nextcloud/vm/blob/67c5a8814e396f99e12ef5319288d7704a0e52be/apps/previewgenerator.sh#L232-L236

@connection-reset
Copy link
Contributor Author

It will not fail, this patch makes no change to how previews work.

@beardhatcode
Copy link
Contributor

@szaimen, this PR only changes the "max preview width" and "max preview height" send to the server when requesting a preview. The Nextcloud server will then return the largest preview it can give. So with preview_max_y = 2048 you will never get a preview higher than 2048px even if you request one of height 100000000px.

@beardhatcode
Copy link
Contributor

@skjnldsv I rebased this on master

@beardhatcode beardhatcode requested a review from skjnldsv June 1, 2021 17:32
@skjnldsv
Copy link
Member

skjnldsv commented Jun 4, 2021

Also it will be interesting to know if it still works with those values applied:
nextcloud/vm@67c5a88/apps/previewgenerator.sh#L232-L236

Vm is not officially supported nor maintained by us. We strongly encourage to make sure any default config fits your needs :)
In any case, requesting a preview will always fallback to closest power of 4 (capped by the max config). 2048 here 🤷

@szaimen
Copy link
Contributor

szaimen commented Jun 4, 2021

Vm is not officially supported nor maintained by us.

I know :)

We strongly encourage to make sure any default config fits your needs :)
In any case, requesting a preview will always fallback to closest power of 4 (capped by the max config). 2048 here 🤷

Great, sounds good! :)

@beardhatcode
Copy link
Contributor

beardhatcode commented Jun 5, 2021 •
edited
Loading

@skjnldsv , I'll merge this if the tests pass. Works for me locally

@connection-reset @beardhatcode
getPreviewIfAny: Use physical pixel size for preview resolution
c7aceae 
Code was inspired by code in an MDN page (in the public domain)
https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio

getPreviewIfAny: extract queryParams to get preview

Signed-off-by: Robbert Gurdeep Singh <git@beardhatcode.be>
Signed-off-by: Axel Pirek <axel.pirek@protonmail.com>
@skjnldsv skjnldsv merged commit 46e17ee into nextcloud:master Jun 5, 2021
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 5, 2021
Merged
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
Merged
59 tasks
@szaimen szaimen mentioned this pull request Jun 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@beardhatcode beardhatcode beardhatcode approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request Pending backport by the backport-bot bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use physical pixel size instead of CSS pixel size for preview image resolution
4 participants
@connection-reset @skjnldsv @beardhatcode @szaimen