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.

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

Performance: Full virtualization of the mosaic view #3436

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

heikomat
Copy link
Sponsor Contributor

@heikomat heikomat commented May 29, 2023

This updates the pseudo-virtualization of the mosaic view with a real virtualization.

This increases scroll-performance yet again (less blinking, faster loading), while decreasing memory usage.
The effect should be especially noticeable when scrolling through a lot of images on a small screen.

The virtualization is implemented by measuring the size of the first Element using a ResizeObserver and assuming, that all other elements have the same size and no spacing inbetween.

This has not yet been tested on iOS devices yet.

There is a known bug on Firefox, that the number of columns is off by 1 on certain resolutions. I highly suspect that that is because of the non-integer widths of the columns on most screen-sizes. Because the position-calculations are simple JS, it's probably the size reported by the ResizeObserver. Will fix that later
Fixed it

There is a known bug where often when opening a photo, and using the left/right arrows to switch between images, the wrong images are shown. This is strange, because the viewer opening logic isn't touched in this PR. Will try to find the cause and fix it soon.
Fixed it

Performance comparison
Scrolling through 1400 images on a smartphone-sized screen

before
performance-old

after
performance-new

Acceptance Criteria:

  • Features and enhancements must be fully implemented so that they can be released at any time without additional work
  • Automated unit and/or acceptance tests are mandatory to ensure the changes work as expected and to reduce repetitive manual work
  • Frontend components must be responsive to work and look properly on phones, tablets, and desktop computers; you must have tested them on all major browsers and different devices
  • Documentation and translation updates should be provided if needed

@lastzero
Copy link
Member

Looks amazing. We'll test it when are back in our office. Let us know when you think this is ready to be merged!

@heikomat
Copy link
Sponsor Contributor Author

heikomat commented May 29, 2023

@lastzero will do.
Fixed the Firefox bug.
but: using the left/right arrows in the photo viewer shows the wrong pictures? (which is interesting, because the viewer opening logic isn't touched in this PR) update: fixed

Now the only things missing are

  • fixing the viewer-prev/next-bug update: fixed
  • more testing on more devices
  • maybe some more benchmarking in different scenarios

@heikomat
Copy link
Sponsor Contributor Author

update: viewer-prev/next-bug is fixed

@lastzero lastzero added enhancement Refactoring, improvement or maintenance task work-in-progress Please don't merge just yet performance Performance Optimization labels Jun 12, 2023
@lastzero lastzero changed the title Mosaic real virtualization Performance: Full virtualization of the mosaic view Jun 12, 2023
@lastzero
Copy link
Member

My understanding is that this should first be tested on more devices, in particular iOS, before it's safe to be merged into develop?

@heikomat
Copy link
Sponsor Contributor Author

heikomat commented Jul 8, 2023

My understanding is that this should first be tested on more devices, in particular iOS, before it's safe to be merged into develop?

That is correct

@heikomat
Copy link
Sponsor Contributor Author

heikomat commented Jul 8, 2023

I found a small bug with these changes where when you scroll to the bottom of the currently loaded pictures super fast, the infinite-scroller seems to not always recognize that the end of the list was reached. If this happens, the next batches are only loaded once you scroll the tiniest but upwards.

This does not happen when not scrolling super fast, presumably because the infinite scroller isn't missing any events in that case

@lastzero
Copy link
Member

lastzero commented Aug 1, 2023

When you are ready, I'd be happy to merge this as well.

@heikomat
Copy link
Sponsor Contributor Author

heikomat commented Aug 1, 2023

Not yet. Let me fix this last bug mentioned here first, and let me do some performance tests on different devices first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Refactoring, improvement or maintenance task performance Performance Optimization work-in-progress Please don't merge just yet
Projects
Status: Development 🐝
Development

Successfully merging this pull request may close these issues.

None yet

2 participants