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

Jump to bottom

Feature/improve date selection #9160

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Feature/improve date selection #9160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfd3583
feat(web): set the initial date in the change date dialog
sebbi08 Apr 29, 2024
2d1592f
feat(web): allow onyl date change
sebbi08 Apr 29, 2024
6d454a9
chore: use unsused variable
sebbi08 Apr 29, 2024
c665ec2
chore: remove unneeded ternary operator
sebbi08 Apr 30, 2024
c6fc22b
chore: fix missing variable
sebbi08 Apr 30, 2024
9ef8335
chore: fix missing import after rebase
sebbi08 May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions mobile/openapi/doc/AssetBulkUpdateDto.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions mobile/openapi/doc/UpdateAssetDto.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 18 additions & 1 deletion mobile/openapi/lib/model/asset_bulk_update_dto.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 18 additions & 1 deletion mobile/openapi/lib/model/update_asset_dto.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions mobile/openapi/test/asset_bulk_update_dto_test.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions mobile/openapi/test/update_asset_dto_test.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions open-api/immich-openapi-specs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6883,6 +6883,9 @@
"isFavorite": {
"type": "boolean"
},
"keepTimeUnchanged": {
"type": "boolean"
},
"latitude": {
"type": "number"
},
Expand Down Expand Up @@ -10624,6 +10627,9 @@
"isFavorite": {
"type": "boolean"
},
"keepTimeUnchanged": {
"type": "boolean"
},
"latitude": {
"type": "number"
},
Expand Down
2 changes: 2 additions & 0 deletions open-api/typescript-sdk/src/fetch-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ export type AssetBulkUpdateDto = {
ids: string[];
isArchived?: boolean;
isFavorite?: boolean;
keepTimeUnchanged?: boolean;
latitude?: number;
longitude?: number;
removeParent?: boolean;
Expand Down Expand Up @@ -311,6 +312,7 @@ export type UpdateAssetDto = {
description?: string;
isArchived?: boolean;
isFavorite?: boolean;
keepTimeUnchanged?: boolean;
latitude?: number;
longitude?: number;
};
Expand Down
3 changes: 3 additions & 0 deletions server/src/dtos/asset.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ export class UpdateAssetBase {
@IsDateString()
dateTimeOriginal?: string;

@Optional()
keepTimeUnchanged?: boolean;

@ValidateGPS()
@IsLatitude()
@IsNotEmpty()
Expand Down
18 changes: 16 additions & 2 deletions server/src/services/asset.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ export class AssetService {
}

async updateAll(auth: AuthDto, dto: AssetBulkUpdateDto): Promise<void> {
const { ids, removeParent, dateTimeOriginal, latitude, longitude, ...options } = dto;
const { ids, keepTimeUnchanged, removeParent, dateTimeOriginal, latitude, longitude, ...options } = dto;
await this.access.requirePermission(auth, Permission.ASSET_UPDATE, ids);

// TODO: refactor this logic into separate API calls POST /stack, PUT /stack, etc.
Expand Down Expand Up @@ -323,7 +323,21 @@ export class AssetService {
}

for (const id of ids) {
await this.updateMetadata({ id, dateTimeOriginal, latitude, longitude });
const asset = await this.assetRepository.getById(id);
const oldCreatedAtDate = asset?.fileCreatedAt && DateTime.fromJSDate(asset.fileCreatedAt);
let newDateTimeString = dateTimeOriginal;
if (dateTimeOriginal && keepTimeUnchanged && oldCreatedAtDate) {
let newDateTime = DateTime.fromISO(dateTimeOriginal);

newDateTime = newDateTime.set({
hour: oldCreatedAtDate.hour,
minute: oldCreatedAtDate?.minute,
second: oldCreatedAtDate.second,
});
const newDateTimeStringWithNull = newDateTime?.toISO();
newDateTimeString = newDateTimeStringWithNull === null ? undefined : newDateTimeStringWithNull;
}
Comment on lines +326 to +339
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inside a for loop, which might be called with 10k assets. Loading an asset from the database one at a time is going to be a problem. In hindsight, calling updateMetadata one at a time, which eventually calls upsertExif might not be the best either. I would probably recommend finding a more optimized solution. Ideally we move this logic into updateMetadata itself, and change that method to accept a list of ids to update. Only if the keepTimeUnchanged property is set, we can use the assetRepo.getByIds methods to load them in a single query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also move the "keepTimeUnchanged" logic inside the SIDECAR_WRITE job or create a new one for that case.

The problem I see with changing multiple assets in one Job is the error handling.
What should happen if one of the metadata updates fail?

And maybe one side question:
is await this.jobRepository.queue waiting for the job to complete, or is it fire and forget. And what would happen if the SIDECAR_WRITE fails and the

await this.assetRepository.updateAll(ids, options); // asset.services.ts L 343

still is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

.queue is waiting for the job to be added to the queue. It is fine if the job takes longer to execute since it happens in the background.

Currently the bulk update (and single update) method optimistically update the database so the UI can refresh immediately and see the changes, but then persist the changes to a sidecar file so future metadata extraction doesn't erase/override the changes.

It is important that the initial implementation is fast, since the caller is waiting. Updating every asset individually makes that a little bit more difficult, especially if the asset needs to be read ahead of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The options are to not do any update immediately, but defer it to the job. The user might be confused as the time isn't updated and the assets still appear in the wrong place for some time.

Or, update each asset one at a time, but optimize the loading to be in a single database request.

await this.updateMetadata({ id, dateTimeOriginal: newDateTimeString, latitude, longitude });
}

await this.assetRepository.updateAll(ids, options);
Expand Down
12 changes: 6 additions & 6 deletions web/src/lib/components/asset-viewer/detail-panel.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
import { featureFlags } from '$lib/stores/server-config.store';
import { user } from '$lib/stores/user.store';
import { websocketEvents } from '$lib/stores/websocket';
import { getAssetThumbnailUrl, getPeopleThumbnailUrl, isSharedLink, handlePromiseError } from '$lib/utils';
import { getAssetThumbnailUrl, getPeopleThumbnailUrl, handlePromiseError, isSharedLink } from '$lib/utils';
import { delay } from '$lib/utils/asset-utils';
import { autoGrowHeight } from '$lib/utils/autogrow';
import { clickOutside } from '$lib/utils/click-outside';
import { shortcut } from '$lib/utils/shortcut';
import {
ThumbnailFormat,
getAssetInfo,
Expand Down Expand Up @@ -38,10 +39,9 @@
import CircleIconButton from '../elements/buttons/circle-icon-button.svelte';
import PersonSidePanel from '../faces-page/person-side-panel.svelte';
import ChangeLocation from '../shared-components/change-location.svelte';
import UserAvatar from '../shared-components/user-avatar.svelte';
import LoadingSpinner from '../shared-components/loading-spinner.svelte';
import { NotificationType, notificationController } from '../shared-components/notification/notification';
import { shortcut } from '$lib/utils/shortcut';
import UserAvatar from '../shared-components/user-avatar.svelte';

export let asset: AssetResponseDto;
export let albums: AlbumResponseDto[] = [];
Expand Down Expand Up @@ -145,10 +145,10 @@

let isShowChangeDate = false;

async function handleConfirmChangeDate(dateTimeOriginal: string) {
async function handleConfirmChangeDate(dateTimeOriginal: string, keepTimeUnchanged: boolean) {
isShowChangeDate = false;
try {
await updateAsset({ id: asset.id, updateAssetDto: { dateTimeOriginal } });
await updateAsset({ id: asset.id, updateAssetDto: { dateTimeOriginal, keepTimeUnchanged } });
} catch (error) {
handleError(error, 'Unable to change date');
}
Expand Down Expand Up @@ -433,7 +433,7 @@
: DateTime.now()}
<ChangeDate
initialDate={assetDateTimeOriginal}
on:confirm={({ detail: date }) => handleConfirmChangeDate(date)}
on:confirm={({ detail: data }) => handleConfirmChangeDate(data.newDateValue, data.keepTimeUnchanged)}
on:cancel={() => (isShowChangeDate = false)}
/>
{/if}
Expand Down
16 changes: 11 additions & 5 deletions web/src/lib/components/photos-page/actions/change-date-action.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,41 @@
import { getSelectedAssets } from '$lib/utils/asset-utils';
import { handleError } from '$lib/utils/handle-error';
import { updateAssets } from '@immich/sdk';
import { mdiCalendarEditOutline } from '@mdi/js';
import { DateTime } from 'luxon';
import MenuOption from '../../shared-components/context-menu/menu-option.svelte';
import { getAssetControlContext } from '../asset-select-control-bar.svelte';
import { mdiCalendarEditOutline } from '@mdi/js';
export let menuItem = false;
const { clearSelect, getOwnedAssets } = getAssetControlContext();

let isShowChangeDate = false;

const handleConfirm = async (dateTimeOriginal: string) => {
const handleConfirm = async (dateTimeOriginal: string, keepTimeUnchanged: boolean) => {
isShowChangeDate = false;
const ids = getSelectedAssets(getOwnedAssets(), $user);

try {
await updateAssets({ assetBulkUpdateDto: { ids, dateTimeOriginal } });
await updateAssets({ assetBulkUpdateDto: { ids, dateTimeOriginal, keepTimeUnchanged } });
} catch (error) {
handleError(error, 'Unable to change date');
}
clearSelect();
};
const getInitialDate = () => {
const ids = getSelectedAssets(getOwnedAssets(), $user);
const selectedAsset = Array.from(getOwnedAssets()).find((asset) => ids.includes(asset.id));
const initialDate = selectedAsset ? DateTime.fromISO(selectedAsset.fileCreatedAt) : DateTime.now();
return initialDate;
};
</script>

{#if menuItem}
<MenuOption text="Change date" icon={mdiCalendarEditOutline} on:click={() => (isShowChangeDate = true)} />
{/if}
{#if isShowChangeDate}
<ChangeDate
initialDate={DateTime.now()}
on:confirm={({ detail: date }) => handleConfirm(date)}
initialDate={getInitialDate()}
on:confirm={({ detail: date }) => handleConfirm(date.newDateValue, date.keepTimeUnchanged)}
on:cancel={() => (isShowChangeDate = false)}
/>
{/if}
16 changes: 10 additions & 6 deletions web/src/lib/components/shared-components/change-date.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<script lang="ts">
import { createEventDispatcher } from 'svelte';
import Checkbox from '$lib/components/elements/checkbox.svelte';
import { DateTime } from 'luxon';
import ConfirmDialogue from './confirm-dialogue.svelte';
import Combobox from './combobox.svelte';
import { createEventDispatcher } from 'svelte';
import DateInput from '../elements/date-input.svelte';
import Combobox from './combobox.svelte';
import ConfirmDialogue from './confirm-dialogue.svelte';

export let initialDate: DateTime = DateTime.now();

Expand All @@ -29,7 +30,6 @@
}));

const initialOption = timezones.find((item) => item.value === 'UTC' + initialDate.toFormat('ZZ'));

let selectedOption = initialOption && {
label: initialOption?.label || '',
value: initialOption?.value || '',
Expand All @@ -40,17 +40,18 @@
// Keep local time if not it's really confusing
$: date = DateTime.fromISO(selectedDate).setZone(selectedOption?.value, { keepLocalTime: true });

let keepTimeUnchanged = false;
const dispatch = createEventDispatcher<{
cancel: void;
confirm: string;
confirm: { newDateValue: string; keepTimeUnchanged: boolean };
}>();

const handleCancel = () => dispatch('cancel');

const handleConfirm = () => {
const value = date.toISO();
if (value) {
dispatch('confirm', value);
dispatch('confirm', { newDateValue: value, keepTimeUnchanged });
}
};
</script>
Expand Down Expand Up @@ -83,5 +84,8 @@
placeholder="Search timezone..."
/>
</div>
<div class="flex flex-col w-full mt-8">
<Checkbox label="Keep Time unchanged?" id="keeptime" bind:checked={keepTimeUnchanged} />
</div>
</div>
</ConfirmDialogue>