search

LycheeOrg / Lychee

A great looking and easy-to-use photo-management-system you can run on your server, to manage and share photos.
https://lycheeorg.github.io/
MIT License
3.3k stars 294 forks source link

[Enhancement] Improve Deletion of Albums / Removing a Photo From an Album Should be Symmetric To Uploading a Photo #847

Open nagmat84 opened 3 years ago

nagmat84 commented 3 years ago

At the moment, it is impossible to see if a particular photo is assigned to more than one album. If one deletes an album (or a photo), Lychee shows a confirmation dialog which asks whether the photo shall be really deleted. However, there are actually two distinct cases:

  1. The photo is assigned to more than one album. In that case, the photo is not really deleted, but only removed from the current album.
  2. The current album is the last album with the particular photo. In that case, the photo is ultimately deleted.

IMHO it would be more intuitive, if the whole process was symmetric to the way how freshly uploaded photos are handled. It should be possible to link/unlink photos to albums. If an album gets deleted (or a photo gets removed from an album), the photo should only be unlinked from the current album, but never truly deleted. If the current album is the last one with this particular photo, the photo should re-appear in the smart album "unsorted". Then, a photo can be ultimately deleted from the unsorted album.

Option: Instead of putting a photo back into the "unsorted" album, there could optionally be another "orphaned" smart album such that the user is able to distinguish between new photo which have never been sorted into an album before and photos which have already been sorted previously but have been removed from any album.

Minor enhancement: The actions "move photo"/"copy photo"/"delete photo" should be renamed accordingly into "assign photo"/"unassign photo" or similar. Even at the moment, the term "copy" is misleading, because this action does not create a deep copy of the underlying photo file, but only puts the same photo into an additional album.

Under the hood

At the moment, there is only a single table photos, which works as follows:

  1. If a new photo is uploaded from the main page (i.e. not within a particular album), a new row is inserted into the table photos and the column photos.album_id is set to NULL. Photos with photos.album_id = NULL appear in the smart album "unsorted".
  2. If a photo is moved into an album the column photos.album_id is set to the album id. However, it is impossible to make it NULL again.
  3. If a photo is copied to an additional album, the entire row in the table photos is duplicated and the duplicated row gets the new album id. However, during this process a lot of photo-specific attributes (like creation time, focal, geo-position) becomes duplicated, too, and therefore creates redundant information.
  4. If a photo is removed, the row is deleted from the table. Additionally, if this row has been the last row which pointed to the underlying photo file, i.e. there is no other row with an equal value in the column url, then the photo file is deleted, too.

From a database perspective, I would recommend to split the table photos into a table photos and an association table, which I will call album_photo_associations. The latter table gets at least the columns album_id, photo_id and any other column from photos which is specific to a photo as part of an album. The table photos only keeps those columns which apply the photo as such. Linking/unlinking a photo from an album means inserting/deleting rows into/from the table album_photo_associations.

This approach has two advantages:

  1. If an photo is linked to an additional album, no photo-related informations is duplicated. At any time there is only a single row in photos.
  2. Erasing a photos is becomes easier. If a row from photos is deleted, the photo file can be removed, too. There is no need for a distinction of cases, i.e., if there is still another row pointing to the same file.
ildyria commented 3 years ago

Hi, first and foremost thank you for such a detailed suggestion. I really appreciate it as it show that you took the time to dig into Lychee.

However let me correct something:

However, it is impossible to make it NULL again.

This is wrong: you can just reassign the picture to the root and it will be visible in unsorted again.

Now, for more food for thought here is a situation:

+ root
+--+ Album A (visible)
|  |
|  +--+ SubAlbum A1 (hidden)
|     |
|     +-- Photo A
|
+--+ Album B (visible)
   |
   +--+ SubAlbum B1 (visible)
      |
      +-- Photo A

The expected behavior is:

However by using an additional table, it becomes more complex to implement due to the many-many relationship. No as we just got rid of recursive calls to optimize such queries, we are not going back at them again, see the heavy refactoring of the core using nested sets for the album proposed here: https://github.com/LycheeOrg/Lychee/pull/832

That being said I am quite mitigated on such change.

The actions "move photo"/"copy photo"/"delete photo" should be renamed accordingly into "assign photo"/"unassign photo" or similar. Even at the moment, the term "copy" is misleading, because this action does not create a deep copy of the underlying photo file, but only puts the same photo into an additional album.

I disagree. The term copy to while it does not create a "deep" copy still produces the same visual effect, thus it is explicit for the user no matter how misleading you may think of it. I would also argue that it is a copy because we do create a new record in the database for this picture. The file is just not duplicated for obvious space optimization.

nagmat84 commented 3 years ago

I apologize for my initial thoughts below the section "under the hood". I agree that this approach might cause too much work and other drawbacks I did not consider.

However, I still would vote for the enhancement from a user's perspective, but without altering the implementation too much. As a first step it would totally suffice to simply move the photo to the unsorted album, when it is deleted from the last album, i.e. set photo.album_id to NULL instead deleting it from disk. This way the user experiences the intended behaviour, but nothing needs to be changed with respect to the database layout.

ildyria commented 3 years ago

I apologize for my initial thoughts below the section "under the hood".

You don't have to. I really appreciated the fact that you actually did research it and what was going on in the code. That is not something often seen and to be honest quite refreshing. :)

d7415 commented 3 years ago

As a first step it would totally suffice to simply move the photo to the unsorted album, when it is deleted from the last album, i.e. set photo.album_id to NULL instead deleting it from disk. This way the user experiences the intended behaviour, but nothing needs to be changed with respect to the database layout.

If I delete a photo, I intend for it to be deleted, not moved.

ildyria commented 3 years ago

However, I still would vote for the enhancement from a user's perspective, but without altering the implementation too much. As a first step it would totally suffice to simply move the photo to the unsorted album, when it is deleted from the last album, i.e. set photo.album_id to NULL instead deleting it from disk. This way the user experiences the intended behaviour, but nothing needs to be changed with respect to the database layout.

Or even better: add a tick box "delete pictures contained inside the albums and sub albums".

The UI will need to be revamped, things like setting public permissions should have a tick box to propagate to the sub albums, when creating a sub album, there should be a tick box "initialize settings from parent"... There are still a lot of things to improve, what is missing is time and human resources. :|

nagmat84 commented 3 years ago

If I delete a photo, I intend for it to be deleted, not moved.

I have a main use case, why I opted for this enhancement. Often I want to remove a photo from a particular album, because I feel the photo does not fit right into the album any more, but I don't want to delete the photo entirely. In other words, it's fine to remove the photo from the album (and keep it in other albums, if there are any), but if this album is the last one, I want to keep the photo in a special place for later reconsideration.

Now, lets assume I use the trick as mentioned by @ildyria

This is wrong: you can just reassign the picture to the root and it will be visible in unsorted again.

Instead of deleting the photo, I move the photo into the unsorted album manually. However, if I do the same trick with the same photo in a different album again, I get a duplicate of the photo in the unsorted album. (I just tested it.) That's not optimal neither.

Timvrakas commented 3 years ago

If I delete a photo, I intend for it to be deleted, not moved.

This is perhaps slightly orthogonal, but I would prefer if photos that were "deleted" from within albums were moved to an archive folder, and files deleted from there were truly purged. My use case might be abnormal, but I have a number of users uploading/moving photos within a single user view. I can trust them to not intentionally nuke everything, but having to recover from backups whenever someone accidentally deletes something is a headache.

nagmat84 commented 3 years ago

@Timvrakas Exactly, this was my intention.

Timvrakas commented 3 years ago

I actually managed to get a setup I can live with just by patching ConfigFunctions.php to disable the trash button. I just made an archive album and every few months I can nuke that if it gets full.

kamil4 commented 3 years ago

Would it be accurate to say that you are advocating for a "Trash" functionality, as commonly implemented in desktop environments, i.e., a staged delete?

nagmat84 commented 3 years ago

Yes, it is some kind of trash functionality with the following stipulation:

kamil4 commented 3 years ago

This shouldn't be hard to implement. I'm mostly wondering what would be the best way to mark photos in the database as "in the trash". album id 0 is already taken (by Unsorted)... I feel that "trash" should a pure smart album, like the four one we have, not like the tagged albums which exist in the albums table. We could of course add an additional column to the photos table but that seems like an overkill. @ildyria, @d7415, what do you think?

d7415 commented 3 years ago

I kinda wonder from the requests above whether it would be easier to have a config flag where:

That way we can reuse the existing unsorted smart album.

ildyria commented 3 years ago

@kamil4 I would do it like that: https://medium.com/@chrissoemma/laravel-5-8-delete-and-soft-delete-practical-examples-b9b71c0a97f :smile:

Laravel already provide the code for Soft-delete :) (which basically add a few collumns) and a trait to the Photo. I'm pretty sure we can do a smart album that filters on those column. :)

PS: I am also in favor of adding an advanced setting and have that soft-delete functionality disabled by default.