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

Bye bye 'New person..' #336

Merged
merged 25 commits into from
Oct 14, 2020
Merged

Bye bye 'New person..' #336

merged 25 commits into from
Oct 14, 2020

Conversation

matiasdelellis
Copy link
Owner

@matiasdelellis matiasdelellis commented Sep 8, 2020

Ok..
This time I am not going to change the infrastructure beyond this (Just allow null names.).

So, the main changes will be in the controllers, and the fronted. Then the persons will be all the clusters with the same name... And clusters, just that.

imagen

TODO:

  • Get the clusters without a name and suggest assigning one to them.
  • Rename all clusters on images of person view..
  • Change the sidebar panel considering in some way when it has a name and when it does not.
  • Fix and improve tests.

For the future we can continue with #262 and the comparisons would be between those who have names and those who do not.

@matiasdelellis
Copy link
Owner Author

p.s. The idea is basically to finish some discussions about join clustering and performance. 😅

@escoand
Copy link

escoand commented Sep 9, 2020

Good idea. And this is also the kind of view you want to integrate into images app?

@matiasdelellis
Copy link
Owner Author

Hi @escoand

Good idea. And this is also the kind of view you want to integrate into images app?

Yes. Hope to have something like this. However, there is no api for it yet. 😉

@matiasdelellis
Copy link
Owner Author

matiasdelellis commented Oct 7, 2020

Well, for the main view this would look more or less like this..

Main view: Persons
imagen

When selecting a person: Show all photos of all clusters with the same name. Note that reuses the nextcloud thumbnails and therefore the server load is less.
imagen

If the "person" has multiple clusters, it shows the button to see them and allows rename each clusters. This allows you to correct some errors.

Clusters view
imagen

Improve sidebar: Sorted by name, unknown last, and in a lighter gray. Still need to sort them by confidence
imagen

@escoand
Copy link

escoand commented Oct 7, 2020

OK, in the first layer you have added the "person abstraction" and show only distinct names, right? Thats effectively the often requested merge of multiple clusters with really less overhead and does not change the background work. Simple but nice solution.

But I don't understand why you show only one cluster on the second layer. Wouldn't make it more sense to show already here all clusters but allow to rename them?

But I would think the separation and rename button should be only small as this is in most cases just an onetime action.

@matiasdelellis
Copy link
Owner Author

HI @escoand

But I don't understand why you show only one cluster on the second layer.

They are also all clusters of one person. Count the pictures in the second and third pictures. 😉

Couldn't make it more sense to show already here all clusters but allow to rename them?

You are right, also still need to rename all the clusters of the same person in the second view. Soon... 😬

The third view is still necessary to fix grouping errors since the most common mistake is sometimes not using the last names, and 'join' two persons with the same name.. 😅

@matiasdelellis
Copy link
Owner Author

matiasdelellis commented Oct 11, 2020

EDIT: When the first clusters are created, suggests assigning them names. 😬

imagen

@matiasdelellis
Copy link
Owner Author

With last commit change url when you select a person.
e.g. https://nextcloud.com/settings/user/facerecognition?name=Sheldon%20Cooper

So, You can put as favorites in your browser and access the photos of your favorite people with it.. 😬

@dassio, this is especially for #344

@dassio
Copy link
Contributor

dassio commented Oct 13, 2020

e.g. https://nextcloud.com/settings/user/facerecognition?name=Sheldon%20Cooper

shouldn't it be https://nextcloud.com/index.php/settings/user/facerecognition?name=Sheldon%20Cooper ?

@matiasdelellis
Copy link
Owner Author

e.g. https://nextcloud.com/settings/user/facerecognition?name=Sheldon%20Cooper

shouldn't it be https://nextcloud.com/index.php/settings/user/facerecognition?name=Sheldon%20Cooper ?

Yes... The public url depends on the RewriteBase configuration, but the api itself must include the index.php, however don't worry about it. Just trust on OCP\IURLGenerator.

@matiasdelellis
Copy link
Owner Author

@dassio in last commit add an method to get this.

I will merge this patch tomorrow. Wait and reuse all this code. 😄

@matiasdelellis matiasdelellis merged commit b26f39a into master Oct 14, 2020
@DerOetzi
Copy link

I'm very excited about this feature. When will this be available for update. Or is there the possibility to manual update to version 0.6.4?

@matiasdelellis
Copy link
Owner Author

Hi @DerOetzi
Thanks, We hope it meets expectations.. 😅

Surely in a couple of days I'll release the update. I just hope to merge #344 and need testers on NC20 for that. 😉

@DerOetzi
Copy link

@matiasdelellis if you can give me a hint how to test I can give it a try on my instance.

@matiasdelellis
Copy link
Owner Author

Hi @DerOetzi
See #343 (comment)

@matiasdelellis matiasdelellis deleted the split-cluster-person branch October 27, 2020 14:13
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.

4 participants