add Nextcloud 20 unified searching #344

dassio · 2020-10-09T20:59:38Z

nextcloud change the search mechanism , using an unified search . app has to provide an class that implements IProvider interface

matiasdelellis

Hi @dassio
Thank you very much for the contribution.

Most of the comments are due bad styles.. but first I would recommend that you try to divide the PR in two.

One PR to support NC20.

Please make sure it works. You tested?
You still have to enable the version!. https://github.com/matiasdelellis/facerecognition/blob/master/appinfo/info.xml#L40
Make sure they pass the tests. Add NC 20 https://github.com/matiasdelellis/facerecognition/blob/master/.travis.yml#L23-L25

Another PR for unified search.

I think it is a good feature of NC20, but I think it is not intended to return all photos of a person.
It should return the persons that you found, and if you click them, show the photos.
For this feature, you may want to wait that i finish the person view on PR Bye bye 'New person..' #336

lib/Search/PersonSearchProvider.php

lib/AppInfo/Application.php

lib/Search/PersonSearchProvider.php

matiasdelellis · 2020-10-10T13:07:33Z

Hi @dassio
There are comments that I received in the mail, but not here.

not really, you can see all the persons at the personal setting page, the unified search is to display the photos belonging to that person

No. The unified search, which allows is to unify the search framework among all the applications!. 😅
When you search within Files, Photos, Deck, Notes, or even within the Settings, all providers are consulted. So these queries should be fast, and since the results of all applications are displayed, they should show only the relevant information.

Can you imagine that happend if you rename your files to mom_***. Jpg, and files return 5000 files and facerecognition shows you another 5000?. It is completely insane. 🙈 😅

That said, I insist on only showing a link to the person within our application. 😬
This view doesn't exist yet (Depend on #336), but just opening our application is more than enough.

i don't see a way to dispaly certain photos after one click: nextcloud does not have this feature to filter photos . and it wil really difficult to change the behaviour of the unified search client js (probably server guys will not accept any PR specific to one app)

Example..
nextcloud/deck@2059e55

dassio · 2020-10-10T14:07:48Z

Hi @matiasdelellis
sorry for the confusion, before i did not check the PR you mentioned , then i saw it, that's why i delete the comment

yes, it would be better to have a link to the personal setting page, i will update on that

matiasdelellis

Wow.. Much better .. Thanks again. 😄

Tomorrow I'll merge #336 and you should rebase on it to reuse a lot of code.

Sorry if I'm annoying.. 🙈 😅

lib/AppInfo/Application.php

lib/AppInfo/Application20.php

lib/Db/ImageMapper.php

lib/Db/PersonMapper.php

lib/Search/PersonSearchProvider.php

matiasdelellis · 2020-10-14T15:07:40Z

@dassio
Just merge #336 . Please, rebase on master, and see last comments.

Signed-off-by: xiangbin.li <dassio@icloud.com>

matiasdelellis

Hi @dassio

Great!!!. 😄 😃 😬 😁 😀 🎉

Last change that I will ask you, and after testing we can merge it. 😀

matiasdelellis · 2020-10-14T21:29:24Z

lib/Search/PersonSearchProvider.php

+			array_map(function (Person $result) {
+				$personName = $result->getName();
+				return new SearchResultEntry(
+				    $this->urlGenerator->imagePath('facerecognition','avatar.webp'),


Suggested change

$this->urlGenerator->imagePath('facerecognition','avatar.webp'),

''

Don't add a new icon, just use the application icon at least in this stage..

p.s: The main reason is that bitmap icons don't adapt on dark themes. 😅

matiasdelellis · 2020-10-14T21:37:12Z

lib/Search/PersonSearchProvider.php

+				    $personName,
+				    '',
+				    $this->urlService->getRedirectToPersonUrl($personName),
+				    '',


Suggested change

'',

'icon-facerecognition',

Acording:
https://github.com/nextcloud/server/blob/3ffd09d3f191def63ad2766368aaef62b333fcd1/lib/public/Search/SearchResultEntry.php#L86-L107

This parameter is the class of the icon... 😉

MM.. You must include the icon as:

facerecognition/css/facerecognition.scss

Lines 5 to 7 in b26f39a

.icon-back {

@include icon-color('back', 'facerecognition', $color-black);

}

.icon-facrecognition { @include icon-color('app-dark', 'facerecognition', $color-black); }

matiasdelellis · 2020-10-14T21:37:57Z

lib/Service/UrlService.php

@@ -120,12 +120,12 @@ public function getRedirectToFileUrl(int $fileId) {
 	/**
 	 * Redirects to the facerecognition page to show photos of an person.
 	 *
-	 * @param int $personId person id to show
+	 * @param string $personName person id to show


Good catch.. 😬

matiasdelellis · 2020-10-14T22:30:47Z

Ok.. I'm already testing NC19, and it still seems to work fine .. 😄

matiasdelellis · 2020-10-21T14:59:32Z

Hi @dassio
Please, see #343 (comment) and the comments that follow

benjaminaigner · 2021-02-06T09:39:15Z

@matiasdelellis @dassio

Dear all, I'm really glad you worked out the unified search!
Is there progress in merging or do you need help in any way for this PR?
Or do you recommend just to check out this PR, apply your change requests and run it?

matiasdelellis · 2021-03-17T15:00:08Z

Hi @dassio

I take your work and I'm about to publish it. 😬

https://services.delellis.com.ar/data/facerecognition/facerecognition.tar.gz

Please, test. just replace them in the apps/facerecognition folder with this.

Thanks for all again. 😄

…344

matiasdelellis · 2021-03-17T18:45:13Z

Merged in last release.. Thanks again.. 😄

matiasdelellis requested changes Oct 9, 2020

View reviewed changes

matiasdelellis mentioned this pull request Oct 12, 2020

Bye bye 'New person..' #336

Merged

4 tasks

matiasdelellis requested changes Oct 13, 2020

View reviewed changes

dassio added 2 commits October 14, 2020 19:27

add Nextcloud 20 unified searching

9b2ec3a

Signed-off-by: xiangbin.li <dassio@icloud.com>

update according to #336

b33c447

Signed-off-by: xiangbin.li <dassio@icloud.com>

matiasdelellis requested changes Oct 14, 2020

View reviewed changes

matiasdelellis mentioned this pull request Oct 14, 2020

Testing and will it work on Ubuntu 20.04? #343

Closed

matiasdelellis mentioned this pull request Oct 18, 2020

Nextcloud compatibility #347

Closed

matiasdelellis mentioned this pull request Mar 17, 2021

Nextcloud 21 compatibility #429

Closed

matiasdelellis added a commit that referenced this pull request Mar 17, 2021

Use the bootstrap mechanism and introduce unified search. See old PR #…

67b2b65

…344

matiasdelellis closed this Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Nextcloud 20 unified searching #344

add Nextcloud 20 unified searching #344

dassio commented Oct 9, 2020

matiasdelellis left a comment

matiasdelellis commented Oct 10, 2020

dassio commented Oct 10, 2020

matiasdelellis left a comment

matiasdelellis commented Oct 14, 2020

matiasdelellis left a comment

matiasdelellis Oct 14, 2020

matiasdelellis Oct 14, 2020

matiasdelellis Oct 14, 2020

matiasdelellis Oct 14, 2020

matiasdelellis Oct 14, 2020

matiasdelellis commented Oct 14, 2020

matiasdelellis commented Oct 21, 2020

benjaminaigner commented Feb 6, 2021

matiasdelellis commented Mar 17, 2021

matiasdelellis commented Mar 17, 2021

	$this->urlGenerator->imagePath('facerecognition','avatar.webp'),
	''

	.icon-back {
	@include icon-color('back', 'facerecognition', $color-black);
	}

add Nextcloud 20 unified searching #344

add Nextcloud 20 unified searching #344

Conversation

dassio commented Oct 9, 2020

matiasdelellis left a comment

Choose a reason for hiding this comment

matiasdelellis commented Oct 10, 2020

dassio commented Oct 10, 2020

matiasdelellis left a comment

Choose a reason for hiding this comment

matiasdelellis commented Oct 14, 2020

matiasdelellis left a comment

Choose a reason for hiding this comment

matiasdelellis Oct 14, 2020

Choose a reason for hiding this comment

matiasdelellis Oct 14, 2020

Choose a reason for hiding this comment

matiasdelellis Oct 14, 2020

Choose a reason for hiding this comment

matiasdelellis Oct 14, 2020

Choose a reason for hiding this comment

matiasdelellis Oct 14, 2020

Choose a reason for hiding this comment

matiasdelellis commented Oct 14, 2020

matiasdelellis commented Oct 21, 2020

benjaminaigner commented Feb 6, 2021

matiasdelellis commented Mar 17, 2021

matiasdelellis commented Mar 17, 2021