feat: rework photo-viewer/asset-viewer - introduce adaptive-image.svelte#25234
feat: rework photo-viewer/asset-viewer - introduce adaptive-image.svelte#25234
Conversation
|
Preview environment has been removed. |
6f3f0af to
ec48504
Compare
|
When I stop at an asset, wait a few seconds and navigate to the next. I'd expect the full preview image is loaded right away, but instead I am still seeing the thumbhash flash in |
994bdef to
8a0f839
Compare
5d4c3ef to
584dc1e
Compare
2204308 to
0b05772
Compare
895202e to
f6862aa
Compare
6c672b8 to
009def8
Compare
cd15bed to
66bac74
Compare
0e6eaf2 to
7139757
Compare
66bac74 to
8dbcea4
Compare
7139757 to
70953a5
Compare
8dbcea4 to
c13b1e6
Compare
70953a5 to
49791dd
Compare
c13b1e6 to
a99a085
Compare
f58dcb2 to
a705be8
Compare
| if (params.alt !== undefined) { | ||
| img.alt = params.alt; | ||
| } | ||
| if (params.draggable !== undefined) { | ||
| img.draggable = params.draggable; | ||
| } | ||
| if (params.imgClass) { | ||
| img.className = classValueToString(params.imgClass); | ||
| } | ||
| if (params.role) { | ||
| img.role = params.role; | ||
| } | ||
| if (params.style !== undefined) { | ||
| img.setAttribute('style', params.style); | ||
| } | ||
| if (params.title !== undefined && params.title !== null) { | ||
| img.title = params.title; | ||
| } | ||
| if (params.loading !== undefined) { | ||
| img.loading = params.loading; | ||
| } |
There was a problem hiding this comment.
Is this just an Object.assign(img, params)?
There was a problem hiding this comment.
I reworked image-loader -
const createImageElement = (
src: string | undefined,
properties: ImageLoaderProperties,
onLoad: () => void,
onError: () => void,
onStart?: () => void,
) => {
if (!src) {
return undefined;
}
const img = document.createElement('img');
if (properties.alt !== undefined) {
img.alt = properties.alt;
}
if (properties.draggable !== undefined) {
img.draggable = properties.draggable;
}
if (properties.loading !== undefined) {
img.loading = properties.loading;
}
img.addEventListener('load', onLoad);
img.addEventListener('error', onError);
onStart?.();
img.src = src;
return img;
};
there are only 3 props now. Also - I can't really use object.assign, since that will assign 'undefined' to properties like img.alt which assigned the STRING 'undefined' to alt. I think this is cleanest for now.
There was a problem hiding this comment.
To be clear, what I meant is Object.assign(img, properties)
67a5d22 to
d4e7b1b
Compare
| let previousAssetId: string | undefined; | ||
| let previousSharedLinkId: string | undefined; | ||
|
|
||
| const adaptiveImageLoader = $derived.by(() => { |
There was a problem hiding this comment.
You should not assign state inside a $derived please change this to an $effect
There was a problem hiding this comment.
I looked into converting this to $effect and it doesn't work cleanly here. The core issue is timing: $derived.by evaluates synchronously before the template renders, so adaptiveImageLoader is always available. $effect runs after the render cycle, which means the loader would be undefined for the first frame (and on every asset change).
This matters because adaptiveImageLoader in the template. Converting to $effect would require ?. null guards at ~20 places. So, the code becomes less readable (IMHO)
The $derived.by + untrack() pattern here is basically memoization: "if asset/sharedLink haven't changed, return the cached loader; otherwise create a new one." The other "state"" is really just the previous generation of the same loader. The mutations inside untrack() are specifically to maintain that memoization without creating circular reactivity.
There was a problem hiding this comment.
If you want it to run before the DOM renders use $effect.pre?
There was a problem hiding this comment.
That may work at runtime, but typescript then need to use !. instead of ?. in about 12 places, though realistically, it can be condensed down to probably 4. I'll try it.
There was a problem hiding this comment.
Wouldn't a pattern like this work?
const createAdaptiveImageLoader = () =>
new AdaptiveImageLoader(asset, sharedLink, {
currentZoomFn: () => assetViewerManager.zoom,
onImageReady,
onError,
onUrlChange,
});
const loaderKey = $derived(`${asset.id}-${asset.thumbhash}-${sharedLink?.id}`);
let adaptiveImageLoader = $state(createAdaptiveImageLoader());
$effect.pre(() => {
void loaderKey;
untrack(() => {
assetViewerManager.resetZoomState();
adaptiveImageLoader = createAdaptiveImageLoader();
});
return () => {
adaptiveImageLoader.destroy();
};
});2bdcf0d to
15d3613
Compare
…higher quality images for responsiveness
danieldietzler
left a comment
There was a problem hiding this comment.
Ended up being busy with other stuff and have to run now, so only some superficial comments
| onStart?: () => void, | ||
| ) => { | ||
| if (!src) { | ||
| return undefined; |
There was a problem hiding this comment.
| return undefined; | |
| return; |
| if (properties.alt !== undefined) { | ||
| img.alt = properties.alt; | ||
| } | ||
| if (properties.draggable !== undefined) { | ||
| img.draggable = properties.draggable; | ||
| } | ||
| if (properties.loading !== undefined) { | ||
| img.loading = properties.loading; | ||
| } |
There was a problem hiding this comment.
Do you even need these guards? If, for instance, properties.alt is undefined, img.alt will be undefined too since you don't set it anywhere. So, might as well always set img.alt = properties.alt
| const listHeight = Math.min(MAX_LIST_HEIGHT, containerHeight - gap * 2 - chromeHeight); | ||
| const selectorHeight = listHeight + chromeHeight; | ||
|
|
||
| const clampTop = (top: number) => Math.max(gap, Math.min(top, containerHeight - selectorHeight - gap)); |
There was a problem hiding this comment.
Very minor nit; lodash already provides a clamp helper function https://lodash.com/docs/4.17.23#clamp. That might clean up things a bit here
| const overlapX = Math.max( | ||
| 0, | ||
| Math.min(position.left + selectorWidth, faceBox.left + faceBox.width) - Math.max(position.left, faceBox.left), | ||
| ); | ||
| const overlapY = Math.max( | ||
| 0, | ||
| Math.min(position.top + selectorHeight, faceBox.top + faceBox.height) - Math.max(position.top, faceBox.top), | ||
| ); |
There was a problem hiding this comment.
This is kind of hard to follow without taking notes 😅 Could you extract some of those intermediate calculations out into their own variables with descriptive names?
| if (params.alt !== undefined) { | ||
| img.alt = params.alt; | ||
| } | ||
| if (params.draggable !== undefined) { | ||
| img.draggable = params.draggable; | ||
| } | ||
| if (params.imgClass) { | ||
| img.className = classValueToString(params.imgClass); | ||
| } | ||
| if (params.role) { | ||
| img.role = params.role; | ||
| } | ||
| if (params.style !== undefined) { | ||
| img.setAttribute('style', params.style); | ||
| } | ||
| if (params.title !== undefined && params.title !== null) { | ||
| img.title = params.title; | ||
| } | ||
| if (params.loading !== undefined) { | ||
| img.loading = params.loading; | ||
| } |
There was a problem hiding this comment.
To be clear, what I meant is Object.assign(img, properties)
|
This PR has a lot of issues, but the biggest issue is that it is way too big. Please submit pull requests that are of a reasonable size and scope. |
Large rework photo-viewer/asset-viewer
Instead of spinner, use a new adaptive-image component. This component will display the thumbhash as the base layer - quickly followed up by the (highly likely) pre-cached low-res thumbnail, and then kick off the preview-size thumbnail. Mouse-wheel zoom is supported using all formats: thumbhash, preview. Zoom will automatically 'upgrade' the preview thumbnail to be 'fullsize' (or original) on zoom, like before, but now there will be no flicker when the full quality one loads.
All next/previous thumbnails are eagerly pre-loaded, and all pending images are canceled if navigation occurs before they finish loading. Preloads are lower priority, and will back off if a main-image is being loaded.