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(server, web): partner share persons/faces #7045

Closed
wants to merge 10 commits into from

Conversation

ttyridal
Copy link
Contributor

@ttyridal ttyridal commented Feb 12, 2024

This PR will bring the partner's persons/faces into the sharedTo-user's explorer UI
ref discussions: #7038 #5089 #6339 #5457 and probably more

The /person endpoint gains a withPartners flag and web UI is updated to set it.

Remains to include withPartners flag when requesting timeBuckets when personId is set, but thought it would be nice to get some review going :)
Opportunistically including all parter ids when building time-bucket with the personId option set could potentially be an overkill?

The shared persons will still behave a bit "strange" (ref #7038) - they are read only!

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code actually looks really good for now!

server/src/domain/person/person.service.ts Outdated Show resolved Hide resolved
Copy link
Member

@martabal martabal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, I don't know if it's that easy. How do you handle duplicates ? I mean, if you have pics of your son and your partner too, will it appear twice in the people list ?

Can you re-assign a face to a person owned by your partner ?

@ttyridal
Copy link
Contributor Author

Mmmh, I don't know if it's that easy. How do you handle duplicates ? I mean, if you have pics of your son and your partner too, will it appear twice in the people list ?

I specifically don't. As stated in the PR message:

The shared persons will still behave a bit "strange" (ref #7038) - they are read only!

Yes, they will appear twice and no, it's not possible to merge own and partner's persons. I made this pull request to get the basic functionality going (and that will cover a lot of the referenced feature requests I think). Meanwhile, I started that discussion thread in the hope to get some discussion going on where to go next.

In my opinion, a partner share should be near equivalent to your own libraries.. Though some might very likely disagree.

@martabal
Copy link
Member

Right, I think it should be clear in the WebUI what you can modify and what you can't.

In my opinion, a partner share should be near equivalent to your own libraries.. Though some might very likely disagree.

I agree but that's a technical challenge as soon as you have multiple partners : Let's say you have users A, B and C. If users A-B and A-C are partners, B and C must not see each other's faces / people.
Overall, it's a security and performance challenge

@ttyridal
Copy link
Contributor Author

You are right, that will be a challenge. Suggest moving that discussion to #7038 as it's not really related to the PR (as it stand at least)

@ttyridal
Copy link
Contributor Author

ttyridal commented Feb 14, 2024

Right, I think it should be clear in the WebUI what you can modify and what you can't.

Totally agree by the way! A little icon in the corner or something, indicating that this person object is shared to you would be awesome. (think I saw several request for various variants of that on other areas too in the discussion section).. Not sure if I'm up to that task though.

add the withPartners flag to /person endpoint, should allow
requesting person objects shared by the partner in a similar
way that the assets endpoint is doing

feature request immich-app#6339 immich-app#5089 immich-app#5457 immich-app#7038
make it possible for frontend to visualize the fact that this is a shared person
f.ex with a small icon or hiding actions like merge, change name etc that will
fail due to share beeing read-only.
@ttyridal ttyridal force-pushed the feat/partnershare_persons branch from 69f58d8 to fa04b02 Compare February 21, 2024 00:41
@Yetangitu
Copy link

Does this PR have a future or is this functionality planned to be implemented in some other way?

@alextran1502
Copy link
Contributor

@Yetangitu I was debating to merge the PR but what is stopping me is that this might cause confusion because the facial data isn't technically shared and synced between shared accounts. I think it would be better for us to wait and implement it the proper way

@ttyridal
Copy link
Contributor Author

ttyridal commented Apr 2, 2024

@alextran1502 Unfortunately I missed that debate. Could you give a short summary of the objections? What's meant by "isn't technically shared and synced between shared accounts" f.ex? or thoughts on the "proper way"..

Really want the functionality, so I would probably be willing to invest some time on this :)

@xttlegendapi
Copy link

this is a really important missing feature. anyone a update on this?

@sergey-litvinov
Copy link

I'm not sure it would help, but wanted to give another use case here. i have tons of photos made in the past and just want to host them and allow partner to see them as well. So in that case it's not about use it to add new photos, but more one person that would add fresh photos of both me and partner, and then second person would be viewing them. I agree that seeing duplicated faces might be confusing, but for this specific use case - it won't matter that much.

@popy2k14
Copy link

+1 for this feature

@derkling
Copy link

@Yetangitu I was debating to merge the PR but what is stopping me is that this might cause confusion because the facial data isn't technically shared and synced between shared accounts. I think it would be better for us to wait and implement it the proper way

Any update on this topi? Could you please share a view on how this feature should be better implemented if the current PR is not good enough as a starting point?

@popy2k14
Copy link

popy2k14 commented Jun 21, 2024

Just my thoughts:
If i share my library with another person, i have assumed that also the faces data is shared.
I think it's an point of view.

Maybe just an checkbox during sharing would be enough, to give the user who is sharing the library an choice.

PS.: It would be so nice if this get's merged.

@Mirio
Copy link

Mirio commented Jul 5, 2024

+1 for this feature

@Apbooks
Copy link

Apbooks commented Aug 4, 2024

+1 for this
Want to use Immich for family to upload old photos and identify people in the photos. Would like to have a shared album that users can discuss and change the names and dates and locations. Would be really cool to be able to collaborate with all users and have comments about the photos. Sharing faces would make this really awesome!

@alextran1502
Copy link
Contributor

As we are working toward separating the asset relationship, which will ultimately help to solve this use case, I will be closing this PR since it is stale and not the direction we would take to handle this sharing mechanism

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.