Skip to content

Conversation

@emberfiend
Copy link
Contributor

@emberfiend emberfiend commented May 30, 2025

📝 Summary

This introduces a cross-app "dependency" on metadata currently provided by image uploads when Photos is enabled (although it fails gracefully without it). When present in oc_files_metadata, this uses photos-size.value.width and height (and blurhash.value) to show a placeholder of correct size before the image itself loads.

Notes:

  1. While it's slick on Firefox, Chrome (or Chromium anyway) has a noticeable gap between removing the placeholder and adding the loaded image. Any ideas? (I'd like to fix this before merge ideally.) Fixed, the preload wasn't working because of some cache invalidation, so it had to download the image twice. Thanks Julius :)
  2. This adds some brittle-ness with the hardcoded values in the CSS and size calculations. Now whenever we change the ImageView we need to make sure the placeholder is the same size. Open to ideas about ways to mitigate this.
  3. Many ResizeObservers running on a long document mean that this might make window resizing laggy on slower devices. You could make the case that we just size placeholders once, for the initial load's element width, or pass one in from the parent (Editor.vue already watches width).
  4. The blurhash logic could be abstracted out since it's similar to what's in photos, but I feel one repetition does not justify it yet. As always, happy to accommodate if anyone feels differently. Now uses NcBlurHash.
  5. Julius pointed out that this does nothing for resources AttachmentResolver doesn't provide metadata for, such as DAV-sourced bare URLs. Something to add in the future.
  6. On the todo front, I started this PR moving SizeMetadataProvider out of Photos to server, but after discussing with Julius we decided to split the task and do that separately & in conversation with the involved parties. Just flagging it as a follow-up here.

General pointers about Vue best practices or local conventions also appreciated.

🖼️ Videos

🏚️ Before
resize before.webm

🏡 After (firefox)
resize after.webm

🏡 After (chromium)
resize after chrome.webm
resize after chrome.webm

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • [😇] Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • [N/A] Documentation (README or documentation) has been updated or is not required

@codecov
Copy link

codecov bot commented May 30, 2025

Codecov Report

Attention: Patch coverage is 4.83871% with 59 lines in your changes missing coverage. Please review.

Project coverage is 59.00%. Comparing base (72e3453) to head (c1de72b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/nodes/ImageView.vue 4.83% 59 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7275      +/-   ##
==========================================
+ Coverage   52.18%   59.00%   +6.81%     
==========================================
  Files         483      482       -1     
  Lines       41918    37081    -4837     
  Branches     1047     1046       -1     
==========================================
+ Hits        21875    21878       +3     
+ Misses      19940    15101    -4839     
+ Partials      103      102       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@emberfiend emberfiend changed the title use Photos metadata for pre-sized, blurhashed image placeholders feat(ImageView): use Photos metadata for pre-sized, blurhashed image placeholders May 30, 2025
@emberfiend emberfiend force-pushed the feat/image-size-metadata branch 6 times, most recently from 9a3f26e to 73743d6 Compare June 12, 2025 07:35
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Approving already so it can get in after the @nextcloud/vue bump is in

if (this.imageWidth > 0 && this.imageHeight > 0) {
const ratio = this.imageWidth / this.imageHeight
const newWidth =
this.wrapperWidth - 12 > this.imageWidth
Copy link
Member

Choose a reason for hiding this comment

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

As discussed not a blocker but would be nice to base this and the paddings on the image view on the css variables so they adjust if designers change their minds about spacing.

@juliusknorr juliusknorr added enhancement New feature or request 4. to release labels Jun 12, 2025
@emberfiend emberfiend force-pushed the feat/image-size-metadata branch 2 times, most recently from 62c8c11 to a0a3e46 Compare June 13, 2025 10:20
@emberfiend emberfiend force-pushed the feat/image-size-metadata branch from a0a3e46 to c1de72b Compare June 16, 2025 09:44
@emberfiend emberfiend merged commit 9852d4b into main Jun 16, 2025
68 of 70 checks passed
@emberfiend emberfiend deleted the feat/image-size-metadata branch June 16, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image loading moves text

3 participants