immich icon indicating copy to clipboard operation
immich copied to clipboard

chore(server): simplified library scans

Open etnoy opened this issue 1 year ago • 13 comments
trafficstars

Merge all scan options into one image

Library assets that are offline are moved to trash, with a little warning message attached to each asset image

If the asset file is restored, the asset will be restored on next scan. If not, it'll be removed by the trash after the usual 30 days.

We remove isOffline and add a trashReason field to the asset entity

etnoy avatar Sep 01 '24 23:09 etnoy

I think it is time to consolidate the library scanning jobs. I don't think we need 4 separate jobs in a context menu like this.

jrasm91 avatar Sep 02 '24 12:09 jrasm91

Can we have a single scan button/menu item that's opens a modal to submit a new scan job. It can use a drop-down to pick between a few options:

  • new/added
  • updated
  • removed
  • all

I think we can have a single library scanning job that takes in a body of options, that can use the selected type. Something like { type: 'new' }

jrasm91 avatar Sep 02 '24 12:09 jrasm91

Can we have a single scan button/menu item that's opens a modal to submit a new scan job. It can use a drop-down to pick between a few options:

  • new/added
  • updated
  • removed
  • all

I think we can have a single library scanning job that takes in a body of options, that can use the selected type. Something like { type: 'new' }

The pr description is already outdated, I've cleaned it up somewhat. But I agree 100% and will try to incorporate this as well

etnoy avatar Sep 02 '24 13:09 etnoy

I'd rather keep isOffline and use it for all assets instead of just library assets. It can be useful for the repair page, which is basically inaccessible for a lot of instances right now due to the amount of IO it does on each load.

Edit: also, isn't it a little dangerous if someone happens to make a mistake when changing their mounts or something goes offline and a scan wipes out all their asset info?

mertalev avatar Sep 02 '24 22:09 mertalev

I'd rather keep isOffline and use it for all assets instead of just library assets. It can be useful for the repair page, which is basically inaccessible for a lot of instances right now due to the amount of IO it does on each load.

Edit: also, isn't it a little dangerous if someone happens to make a mistake when changing their mounts or something goes offline and a scan wipes out all their asset info?

Regarding the danger part, I've been thinking about this for a while. With isOffline, we aren't really helping the user at the moment. Only when viewing individual assets could you potentially spot that things are offline due to the red box on the right hand side, and this is only on web, not mobile. If someone (including myself) removes assets, they'll just run scan immediately followed by remove offline without any hesitation between the two steps.

This change ensures that the removal of deleted assets only happens manually, it's never automatically scheduled in any way. I'll add a simple check to each import path to make sure it's still there. This way we can exclude the simple cases of a network share being unavailable and make sure it doesn't cause the whole library to be deleted.

etnoy avatar Sep 03 '24 08:09 etnoy

I'd rather keep isOffline and use it for all assets instead of just library assets. It can be useful for the repair page, which is basically inaccessible for a lot of instances right now due to the amount of IO it does on each load.

Edit: also, isn't it a little dangerous if someone happens to make a mistake when changing their mounts or something goes offline and a scan wipes out all their asset info?

For the isOffline being used for other things, I really think we instead should move the repair info to a separate table and have a job that checks repair things in the background. Having this info in the asset table doesn't really make sense to me.

etnoy avatar Sep 03 '24 08:09 etnoy

cc @zackpollard I think you were working on the repair page stuff right?

bo0tzz avatar Sep 03 '24 08:09 bo0tzz

cc @zackpollard I think you were working on the repair page stuff right?

Yea, I put it on hold when I saw this rework was happening, I will continue after this gets merged :smile: But yes, the plan is for things to move to another table, realistically with the new files table it might end up living there, as the file itself is the thing that's really offline.

zackpollard avatar Sep 03 '24 08:09 zackpollard

cc @zackpollard I think you were working on the repair page stuff right?

Yea, I put it on hold when I saw this rework was happening, I will continue after this gets merged 😄

You guys are talking about repair and I have no idea why. Are we using isOffline outside of external libraries? There's nothing in the code to suggest that

etnoy avatar Sep 03 '24 08:09 etnoy

cc @zackpollard I think you were working on the repair page stuff right?

Yea, I put it on hold when I saw this rework was happening, I will continue after this gets merged 😄

You guys are talking about repair and I have no idea why. Are we using isOffline outside of external libraries? There's nothing in the code to suggest that

The plan was to change the repair page to run in the background, and given we already have a lot of logic for scanning "external" libraries, it'd make sense to extend this to all libraries and use that logic for the repair page. Otherwise we are maintaining two implementations of scanning for missing files.

zackpollard avatar Sep 03 '24 08:09 zackpollard

This PR has taken another turn, now it moves offline files to trash instead of deleting them. Four scan options have been merged into one

etnoy avatar Sep 04 '24 10:09 etnoy

Some notes, but overall this looks like a great change and vastly simplifies the external libraries scanning code. Once comments are resolved I am happy for this to be merged in 😄

Thanks! Happy that you like the concepts in this PR. I'll do some modifications based on your suggestions.

etnoy avatar Sep 13 '24 20:09 etnoy