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

fix(web): slideshow ascending order when video has ended #8024

Closed
wants to merge 5 commits into from
Closed

fix(web): slideshow ascending order when video has ended #8024

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
273f683
fix: slideshow ascending order when video has ended
martabal Mar 17, 2024
a3164b8
fix: tests
martabal Mar 17, 2024
9f22a06
pr feedback
martabal Mar 17, 2024
1feb877
pr feedback
martabal Mar 17, 2024
b96a07d
pr feedback
martabal Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions web/src/lib/components/asset-viewer/asset-viewer.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,9 @@

const handleVideoEnded = async () => {
if ($slideshowState === SlideshowState.PlaySlideshow) {
await navigateAsset('next');
$slideshowNavigation === SlideshowNavigation.AscendingOrder
? await navigateAsset('previous')
: await navigateAsset('next');
Copy link
Contributor

Choose a reason for hiding this comment

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

Man, I really dislike this. We're adding more duplicate code everywhere and the asset-viewer is even more coupled to the slideshow functionality than it was before. The component should only be in charge of rendering an asset, not managing all this unrelated state.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved everything to navigateAsset

}
};

Expand Down Expand Up @@ -576,9 +578,8 @@
{:else}
<VideoViewer
assetId={previewStackedAsset.id}
on:close={closeViewer}
on:onVideoEnded={handleVideoEnded}
on:onVideoStarted={handleVideoStarted}
onVideoEnd={handleVideoEnded}
onVideoStart={handleVideoStarted}
/>
{/if}
{/key}
Expand All @@ -594,11 +595,7 @@
</div>
{:else if asset.type === AssetTypeEnum.Image}
{#if shouldPlayMotionPhoto && asset.livePhotoVideoId}
<VideoViewer
assetId={asset.livePhotoVideoId}
on:close={closeViewer}
on:onVideoEnded={() => (shouldPlayMotionPhoto = false)}
/>
<VideoViewer assetId={asset.livePhotoVideoId} onVideoEnd={() => (shouldPlayMotionPhoto = false)} />
{:else if asset.exifInfo?.projectionType === ProjectionType.EQUIRECTANGULAR || (asset.originalPath && asset.originalPath
.toLowerCase()
.endsWith('.insp'))}
Expand All @@ -607,12 +604,7 @@
<PhotoViewer {asset} {preloadAssets} on:close={closeViewer} />
{/if}
{:else}
<VideoViewer
assetId={asset.id}
on:close={closeViewer}
on:onVideoEnded={handleVideoEnded}
on:onVideoStarted={handleVideoStarted}
/>
<VideoViewer assetId={asset.id} onVideoEnd={handleVideoEnded} onVideoStart={handleVideoStarted} />
{/if}
{#if $slideshowState === SlideshowState.None && isShared && ((album && album.isActivityEnabled) || numberOfComments > 0)}
<div class="z-[9999] absolute bottom-0 right-0 mb-6 mr-6 justify-self-end">
Expand Down
10 changes: 6 additions & 4 deletions web/src/lib/components/asset-viewer/video-viewer.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@
import { getAssetFileUrl, getAssetThumbnailUrl } from '$lib/utils';
import { handleError } from '$lib/utils/handle-error';
import { ThumbnailFormat } from '@immich/sdk';
import { createEventDispatcher } from 'svelte';
import { fade } from 'svelte/transition';
import LoadingSpinner from '../shared-components/loading-spinner.svelte';

export let assetId: string;
export let onVideoStart: (() => void) | undefined = undefined;
export let onVideoEnd: () => void;

let isVideoLoading = true;
const dispatch = createEventDispatcher<{ onVideoEnded: void; onVideoStarted: void }>();

const handleCanPlay = async (event: Event) => {
try {
const video = event.currentTarget as HTMLVideoElement;
video.muted = true;
await video.play();
video.muted = false;
dispatch('onVideoStarted');
if (onVideoStart) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, does onVideoStart ?? onVideoStart() work?

Copy link
Member

Choose a reason for hiding this comment

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

You would want to do onVideoStart && onVideoStart() and that should work, yea

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice !

Copy link
Contributor

Choose a reason for hiding this comment

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

Really it should just be onVideoStart?.()

onVideoStart();
}
} catch (error) {
handleError(error, 'Unable to play video');
} finally {
Expand All @@ -34,7 +36,7 @@
controls
class="h-full object-contain"
on:canplay={handleCanPlay}
on:ended={() => dispatch('onVideoEnded')}
on:ended={onVideoEnd}
bind:volume={$videoViewerVolume}
poster={getAssetThumbnailUrl(assetId, ThumbnailFormat.Jpeg)}
>
Expand Down
Loading