immich icon indicating copy to clipboard operation
immich copied to clipboard

fix(web/mobile): sort order wrong in asset viewer

Open Sophtli opened this issue 2 years ago • 11 comments

This fixes #2061 I don't really understand why it has to be sorted this way, but I went with the way it is implemented in the asset grid: https://github.com/immich-app/immich/blob/4e526dfaae577336ab5a5e4aea451e722eeabdd7/web/src/lib/components/photos-page/asset-date-group.svelte#L33-L37

Sophtli avatar Mar 29 '23 18:03 Sophtli

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

Name Status Preview Comments Updated (UTC)
immich ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 19, 2023 4:47pm

vercel[bot] avatar Mar 29 '23 18:03 vercel[bot]

Hello, I have a set of photos taken on the same date, the navigation on the web seems to work correctly. Is there a set of photos that you can share to reproduce the issue that this PR trying to fix?

alextran1502 avatar Apr 01 '23 20:04 alextran1502

Hello, I was able to use some of these images to reproduce the issue (iirc they not only have to be taken on the same day but also in the same minute probably): immich.zip

Sophtli avatar Apr 01 '23 22:04 Sophtli

test-images.zip I did some testing with these test images that show the same number as the filename. Everytime I reimport the images the order changes in the timeline on the web. The android app always shows them in the order 5, 2, 4, 3, 1

natedawg avatar Apr 02 '23 00:04 natedawg

I can see that the order is incorrect comparing the mobile app vs the web, however, the PR doesn't seem to fix this?

alextran1502 avatar Apr 02 '23 17:04 alextran1502

I actually haven't looked at the differences between web and mobile before you mentioned it.

I added additional sorting by filename (originalPath should imply filename) because that seems like the only useful criteria that should work in every scenario when the date is completely identical. Without that the sorting seems to be very arbitrary, probably just based on the way the sorting algorithms are implemented, which is hard to track down.

Sophtli avatar Apr 02 '23 20:04 Sophtli

I've put some more thoughts into this and decided that using the the id (remoteId and additionally localId on mobile) would probably be the more intuitive way to handle this. The result is essentially the same but it seems easier and less error-prone to me. Honestly, I don't know why I didn't use this already.

Sophtli avatar Jun 02 '23 23:06 Sophtli

Thank you for working on this.

I do not see why sorting by remoteId makes sense - it's a random UUID. Sorting by it is probably just like taking the algorithms way of handling ties. filename makes a lot more sense to me in this regard.

For me the names of most if not all of my files are pretty random anyways and since there is no way to rename them after uploading them it feels just as random to me as sorting by ID but I can definitely understand why you think so. I'll simply revert the latest commit as soon as I have access to my PC again. Thank you for the feedback!

Sophtli avatar Jun 03 '23 13:06 Sophtli

Sorting by a unique value has the benefit of ensuring the same order across different platforms and when an unstable sorting algorithm is used. I agree that sorting by filename makes more sense than ID, but we could include the ID as a third field.

michelheusschen avatar Jun 03 '23 13:06 michelheusschen

@michelheusschen does adding the id as a third field give any additional value? As far as I can see originalPath should also be unique and thus there should be no case where the id would be used for sorting or am I missing something?

Sophtli avatar Jun 03 '23 13:06 Sophtli

You're right, originalPath is unique. Then reverting the last commit should be good👍

michelheusschen avatar Jun 03 '23 13:06 michelheusschen

I believe this was fixed in #3092.

uhthomas avatar Jul 09 '23 20:07 uhthomas

This PR attempts to fix the order of assets in the main timeline for assets with the same timestamp. So it was not fixed by #3092

brighteyed avatar Jul 10 '23 02:07 brighteyed

This PR attempts to fix the order of assets in the main timeline for assets with the same timestamp. So it was not fixed by #3092

Hmm, okay. There have been some changes in this area so I wonder if this is still present? I also wonder if the server should just do a better job or sorting instead to avoid duplicating this logic across clients?

uhthomas avatar Jul 10 '23 08:07 uhthomas