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.

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

feat: Add username label on shared photos #1266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jovanbulck
Copy link
Contributor

Fixes #955

I find this feature really lacking when sharing large albums and/or timelines with multiple people, so I had a shot at implementing this the other day. Currently looks as follows:

Screenshot from 2024-08-10 21-26-00

The nextcloud username is shown in the bottom left of the picture frame for all pictures that are not owned by the logged-in user, both in the Timeline and Albums view.

I had to extend the database scheme with an additional "owner" column that stores the owner UUID for every memories-indexed photo. This would probably require a lib/Migration/ handler to amend existing rows; not sure how this is normally handled -- @pulsejet ?

Possible extensions include:

  • displaying full user info (ie not just the nextcloud username) in the "metadata" pane
  • filtered view for a dedicated "Shared with you" menu item cf Add Shared with you menu item #787 -- (one of the only features that Photos has that Memories doesn't..)

@pulsejet
Copy link
Owner

This is very nice. Populating the initial data should be easy, just need a loop to copy over during a migration. I'm not aware of any cases where file ownership can transfer so that should also be ok.

The only thing remaining is to make sure we show the display name of the user (not the username). This can be done with a client side cache (per-session)

Btw it might be a while before I can get to merging this. I'm not allowed to work on this project for a bit more time.

@jovanbulck
Copy link
Contributor Author

Great, thanks! No worries about having this in the queue for a while before you can get to it :)

When you get to it, feel free to let me know if there is something I can help with from my side to get this merged

@jovanbulck jovanbulck force-pushed the lbl-user branch 2 times, most recently from c3bd526 to 8b7c31b Compare October 27, 2024 12:01
@jovanbulck
Copy link
Contributor Author

This is very nice. Populating the initial data should be easy, just need a loop to copy over during a migration. I'm not aware of any cases where file ownership can transfer so that should also be ok.

Update (force-pushed in the above commit): I added a simple Migration handler to do this. It gets the UID from the oc_storages table, based on the storage ID found in the oc_filecache table for the respective file referenced in oc_memories.

I tested this on my end with Postgres and it ran very fast on a large DB. However, not sure about the exact syntax for SQLite and MySQL. Also not sure about edge cases like External Storage in Nextcloud (right now the queries rely on the home::uid syntax in oc_storages. FWIW, I made the new UID column notnulll=false in case some of these edge cases wouldn't work.

The only thing remaining is to make sure we show the display name of the user (not the username). This can be done with a client side cache (per-session)

This would still have to be implemented. Not sure how best to go about it, but maybe an initial query that loads all unique uids from oc_memories and then translates the displayname from oc_users and send it back to the client could work(?)

Fixes pulsejet#955

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
@jovanbulck
Copy link
Contributor Author

jovanbulck commented Nov 16, 2024

The only thing remaining is to make sure we show the display name of the user (not the username). This can be done with a client side cache (per-session)

This is now also implemented (as per the force-pushed latest commit above) using a new remote UidController API for the translation and local utils/cache to speed this up for repeated translations. This was new to me, so please double check :)

displaying full user info (ie not just the nextcloud username) in the "metadata" pane

is also implemented with a new "Shared By" view when uid != current_user

@pulsejet Marking this as "ready for review" as this PR should now be feature-complete and works stable on my end (both on a postgresql and a mysql setup).

@jovanbulck jovanbulck marked this pull request as ready for review November 16, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add username label in shared album for added photos
2 participants