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

refactor(web): asset grid state #3513

Merged
merged 3 commits into from
Aug 3, 2023
Merged

Conversation

jrasm91
Copy link
Contributor

@jrasm91 jrasm91 commented Aug 2, 2023

In this PR:

  • Refactor asset grid state
  • Only create an abort controller before starting an http request
  • Track asset lookup map to bucket, bucket index, and asset index

Tested Scenarios:

  • View an asset, hit next until the next time bucket loads (observed in the network tab)
  • View an asset towards end of timeline, hit previous until a previous time bucket loads (observed in the network tab)
  • View first asset in the timeline, observe clicking previous does nothing
  • Scrub the timeline and observe relatively quick loading/rendering.
  • Add an artificial delay to get asset by time bucket to verify abort controller is firing and cancelling requests when the bucket is no longer intersecting
  • Delete photos, make sure they are removed from the grid
  • Archive photos, make sure they are removed from the grid
  • Select all photos, make sure the count matches the statistics for the user
  • Select the first asset, jump a long way down, shift select an asset, then scroll up and observe everything is selected

@vercel
Copy link

vercel bot commented Aug 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Aug 2, 2023 9:42pm

@jrasm91 jrasm91 changed the title refactor(web): time bucket abort logic refactor(web): asset grid state Aug 2, 2023
@jrasm91 jrasm91 requested a review from brighteyed August 2, 2023 05:16
Copy link
Contributor

@brighteyed brighteyed left a comment

Choose a reason for hiding this comment

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

Brilliant! I'm a big fan of your refactoring PRs.

Looks like handleSelectAssets doesn't work as expected:

  • Open photos page and select one asset
  • Click on the scrollbar somewhere near the bottom and Shift-select another asset

Assets got selected, but addGroupToMultiselectGroup was not called for intermediate date groups

@brighteyed
Copy link
Contributor

Brilliant! I'm a big fan of your refactoring PRs.

Looks like handleSelectAssets doesn't work as expected:

  • Open photos page and select one asset
  • Click on the scrollbar somewhere near the bottom and Shift-select another asset

Assets got selected, but addGroupToMultiselectGroup was not called for intermediate date groups

getBucketIndexByAssetId returns null instead of 0 in my case:
image

return nextBucket.assets[0]?.id || null;
}

private emit(recalculate: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for updating the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It updates the svelte store.

getAdjacentAsset,
updateAsset,
subscribe,
init: store.init.bind(store),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help explain what does the bind method do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The this in a method is "whatever object called the method". It's kind of complicated, but basically

on:click={obj.foo}
on:click={() => obj.foo()}

Have different values for this. Only the second one would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bind makes a method always use the same this value, which is passed into bind.

@alextran1502 alextran1502 merged commit 28ab1d4 into main Aug 3, 2023
20 checks passed
@alextran1502 alextran1502 deleted the refactor/asset-grid-state branch August 3, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants