feat: Selectable metadata in duplicates utility with diffing#26328
feat: Selectable metadata in duplicates utility with diffing#26328ollioddi wants to merge 13 commits intoimmich-app:mainfrom
Conversation
1c79b05 to
3a7bf6e
Compare
|
I'd like guidance on what i'm doing wrong with the translations :-) |
|
We've reworked the details shown in the duplicates viewer by now. This PR feels to me like too fine granular customization, I personally don't really see a big use case for this and I doubt many people would use this. @alextran1502 do you agree with me here or do you want that PR? |
|
Hi @danieldietzler, thanks for taking a look! I understand your concern about adding too much granularity for the average user, but my main motivation here is that deleting duplicates is a destructive action. When users are permanently deleting files, they need absolute confidence that they aren't losing valuable data, and the current UI doesn't always provide enough context for that. There are a few key reasons I believe showing more information, and adding labels is necessary: 1. Ambiguity and Cognitive Load 2. Flexibility for Power Users (Nielsen's Heuristics) 3. Preventing Hidden Metadata Loss The "diffing" feature in this PR specifically addresses your concern about UI bloat: it allows users to only see the fields that actually differ, keeping the interface as clean as possible while still surfacing critical discrepancies. To me, the current albeit revamped implementation is still lacking, which is why I decided to pick this up again. I'd love to hear what @alextran1502 thinks. I strongly believe that offering optional labels and the ability to expose hidden metadata differences makes the duplicates tool much safer and more robust for everyone. |
|
Hey again! We've discussed this and don't want this level of customization. We do however like the diffing, so what we'd prefer is
|
|
Thanks for continuing looking into it @danieldietzler. I can absolutely do that. Just to make sure I understand you correctly:
Did you also discuss the labels? They do take up quite a lot of space, but without them I also feel it's quite an increase in mental load deciphering what you are actually deciding on. Especially if theres many duplicates, which shows different fields on every page. For most things, its alright i guess, only one resolution. But still want to hear your thoughts.
EDIT: Since there will be no customisation, are there any metadata fields you always want shown, regardless if they are different or not? |
|
Adding label is fine, we can find the right text size so that it doesn't take up as much space |
|
@danieldietzler @alextran1502 i've implemented your feedback. Changes:
Tested by using it on a couple of groups. It even works nice with 28 photos in the same bucket...
Let me know if you want further modifications. |
web/src/lib/components/utilities-page/duplicates/duplicate-asset.svelte
Outdated
Show resolved
Hide resolved
|
I think adding tags as another metadata info and adding an expand button for the long texts might be good addition(i.e albums or long photo names etc) |
|
Excellent points @danieldietzler, that is much cleaner and duplicates less. I've also reverted the changes to translations. Sorry! I can also do tags as suggested by @multi-suggester if you feel it belongs in the PR? I'd have to populate the tags in the |
|
Nope, please disregard that comment. PRs generally should be focused on one thing; adding more fields would be an extra feature and should go in a new PR :) |
web/src/lib/components/utilities-page/duplicates/duplicate-asset.svelte
Outdated
Show resolved
Hide resolved
web/src/lib/components/utilities-page/duplicates/duplicate-asset.svelte
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @danieldietzler. You had great points. I've pushed the changes, and tested locally. Still functions as expected. Let me know if you need anything else from me. |
| location: isDifferent( | ||
| (a) => [a.exifInfo?.city, a.exifInfo?.state, a.exifInfo?.country].filter(Boolean).join(', ') || 'unknown', | ||
| // Ordered list of keys that differ, sliced based on show-more state | ||
| let visibleKeySet = $derived( |
There was a problem hiding this comment.
| let visibleKeySet = $derived( | |
| let visibleKeys = $derived( |
| keys: MetadataFieldKey[]; | ||
| }; | ||
|
|
||
| let allMetadataItems = $derived([ |
There was a problem hiding this comment.
| let allMetadataItems = $derived([ | |
| let allMetadataItems = $derived<MetadataItem[]>([ |
IMO is cleaner than the satisfies.
Also, this can be const right?
| import { t } from 'svelte-i18n'; | ||
| import { SvelteSet } from 'svelte/reactivity'; | ||
|
|
||
| const INITIAL_VISIBLE_COUNT = 5; |
There was a problem hiding this comment.
We don't use UPPER_SNAKE_CASE for constants. Just do UpperCamelCase.
There was a problem hiding this comment.
Also, as a personal preference I would put it on line 37 or something
| let differingMetadataFields: DifferingMetadataFields = $derived(computeDifferingMetadataFields(assets)); | ||
|
|
||
| let differingCount = $derived(Object.values(differingMetadataFields).filter(Boolean).length); | ||
| let hasMore = $derived(differingCount > INITIAL_VISIBLE_COUNT); |
| <Text size="tiny" class="text-immich-fg/40 dark:text-immich-dark-fg/40 self-center truncate px-1 pr-2 text-[10px]"> | ||
| {title} | ||
| </Text> | ||
| {/if} | ||
|
|
||
| <div class="overflow-hidden justify-self-end text-end rounded px-1 transition-colors"> | ||
| <Text size="tiny" class="break-all text-[10px]"> |
There was a problem hiding this comment.
You shouldn't be overwriting the font size. If you want a larger text, just use size="small" or something instead of size="tiny"
| return duplicateAssets.pop(); | ||
| }; | ||
|
|
||
| export const MetadataFieldKeys = [ |
There was a problem hiding this comment.
This is just allMetadataItems.flatMap(({ keys }) => keys), right?
I think what I would like to see is
- drop this array
- move
allMetadataItemshereexport const getAllMetadataItems = (asset: AssetResponseDto, $t: MessageFormatter) => [...]
- The type of
MetadataFieldKeyshould then be something likeReturnType<typeof getAllMetadataItems>[number]['keys'](if you even still need that type)
There was a problem hiding this comment.
I think getting those >100loc out of the svelte component helps a lot



Description
This PR introduces an enhancement to the duplicates utility. It gives the user full control of what metadata to show in the UI for a duplicate group. It also features diffing to only show relevant differences.
Superseedes #21342, as my branch had gotten 2300 commits behind. I re-created it again to respect changes made since.
Motivation
Deleting photos are destructive, and we want to preserve our best versions of our photos.
Keeping the wrong photo can result in a timeline which is out of order, or you simply lose metadata which you can't simply restore, such as GPS data or camera metadata.
How it works
You have full control over what metadata for a photo you want to see. Currently, a set of defaults are configured, but with most attributes hidden by default.
The controls are exposed via a Modal.
As a user, you have two options:
By allowing to only show different metadata, the UI is kept clean and comparison made easier. I don't want to bloat the UI, but these options gives the user full control over their preference.
Furthermore, the current UI relies entirely on icons for the metadata fields. An option has been added to show the labels, at the expense of a more dense layout (Good for 2 duplicates, a little tight for 4).
Changes made
How Has This Been Tested?
I've used it on many duplicates. Works like a charm.
Screenshots (if appropriate)
The new menu entry which opens the modal
The preferences modal. Choose display mode, and then individual properties you want to be shown.
A duplicate group with only differences selected to be shown
A duplicate group with ALL metadata set to be shown
Labels hidden, relying on icons
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
...
Just a little help coming from React, understanding Svelte concepts. Code is written by myself. Feedback on the Svelte part is welcome :-)
I made sure to use Immich/UI and tried to match conventions of existing code.