Feat: Create filter-plugin architecture for unified search

[nfebe]

Description

This commit introduces the mechanism for apps out of the call, to add search filters to the unified search "Places" filter selector. Resolves : https://github.com/nextcloud/server/issues/42914

TODO

• [x] Ensure nothing regarding this feature is triggered if talk is not installed
• [x] Document how to add filters to unified search, somewhere in developer notes? (Transferred to : https://github.com/nextcloud/documentation/issues/11544)

Manual Testing

Set spreed/talk branch to : https://github.com/nextcloud/spreed/pull/11575

[nfebe]

As @marcoambrosini said: Rename the current "Conversations" filter to "Talk" for better clarity

The reason this was not done straight way is because the providers (come from the backend) but it's possible to loop and find specific filters and rename them, don't know if this route would be okay.

[marcoambrosini]

The reason this was not done straight way is because the providers

I think it's better to do it properly in the backend no?

[nfebe]

The context of the PR has changed and actual UI changes that his PR initially contained are now in : https://github.com/nextcloud/spreed/pull/11575

[nfebe]

Maybe you need to use 'talk-message-current' provider?

I asked and got confirmation from @nickvergessen that the existing provider is correct.

So mentioning here for further alignment, sending extra parameters to that provider, does nothing.

cc: @Altahrim

[nickvergessen]

talk-message is the search provider that accepts the filter. There is also an integration test that the API if called like specified, does the filtering correctly.

[nfebe]

Looking at

https://github.com/nextcloud/spreed/blob/978922ee73e74f15c2ffe3fd83800edb6d2b01af/tests/integration/features/bootstrap/FeatureContext.php#L2452-L2459 it does look like we expect the user to type the search query to include the room? according to regex '/conversation:ROOM\((?P<name>\w+)\)?

Or at least that we need the request like that? In the chats what @Altahrim explained was that we simple need to pass ?term=hello&conversation=<token> for the filtering to work.

Please, am I missing something?

Otherwise, If the user is expected to type strings that match '/conversation:ROOM\((?P<name>\w+)\) I think it should be parsed at the front-end at still sent simply using the query structure above?

Looking at same snippet it seems to suggest @Antreesy is right that we should use talk-message-current' instead as talk-message has descriptive text "search in other rooms"

cc: @nickvergessen

[nickvergessen]

it does look like we expect the user to type the search query to include the room? according to regex /conversation:ROOM\((?P<name>\w+)\)?

This is just because the integration test has only 1 field. But the code you linked to is exactly the important one. It removes the conversation bit from the query and adds &conversation=<token>

So yeah:

?term=hello&conversation=<token>

This is exactly what is expected. /search/providers/talk-message/search?term=hello&conversation=<token> to be specific

talk-message-current only searches in the conversation that is the from=/call/<token2>, so no. talk-message must be used for the filter plugin.

[nfebe]

The URL's being sent look like /search/providers/talk-message/search?term=uni&from=/apps/spreed/&conversation=qbhbdutu @nickvergessen kindly test and review at your earliest convenience.

Thank you.

[nickvergessen]

Seems to be working after compiling locally (because you didn't commit that yet).

The "In conversation" should be moved further up so it is grouped with the other Talk-related filters

This however is currently still missing and impossible for Talk to do. Maybe we can add a "priority" to the registration object like the normal search providers have?

Another thing I noticed is that In conversation is shown as headline instead of Messages when no filter is active:

But that we can also have a look at later. Maybe we can resolve that in Talk itself.

[nfebe]

Seems to be working after compiling locally (because you didn't commit that yet).

The "In conversation" should be moved further up so it is grouped with the other Talk-related filters

This however is currently still missing and impossible for Talk to do. Maybe we can add a "priority" to the registration object like the normal search providers have?

The order is currently not influenced in anyway at the frontend level as that's arranged in the backend. We have some options like :

• Adding priority as suggested : But the question of what is the priority? arises. Does it mean any app can set a priority that would override the (order) priorities sent from the backend? (As whatever priority we set has to be relative to the other ones, hence to achieve the desired results, we need to kinda hard code a priority and do frontend reordering, or copy the algorithm to calculate priority from the backend to the frontend yet making sure the inputs to calculate the priority is/are available before reordering)

• We could just arrange the filters in alphabetical order (?)

---

Another thing I noticed is that In conversation is shown as headline instead of Messages when no filter is active:

• That has to be updated in spreed/talk.

Brings the question of pluralization, we think figure out how to handle pluralization because ones the filter is not applied it is still considered a provider and in a case such as the one above it should read Messages but not sure whether when a particular conversation is applied is should read the same thing.

[nfebe]

@nickvergessen PS, had time to read my comments?

[nickvergessen]

But the question of what is the priority?

Integer, just like with the normal providers on the PHP api

We could just arrange the filters in alphabetical order

Sounds like the worst option and still doesn't allow to group Talk options together

[nfebe]

Integer, just like with the normal providers on the PHP api

It is sure an int, but the chosen integer if being set from the front-end needs to be carefully picked to make sure it ranks just at the right position. In this case, grouping with talk. How do we write that algorithm in a way that prevents any app that implements the filter plugin API from distorting the order by simply setting higher values? (Might introduce unnecessary complexity)

What seems easily achievable at this point, is creating a front-end ordering mechanism that simply inserts the new filters immediately after the last filter from a specific provider, without regard to any integer. The result is that, filters would be grouped according to providers, but all filters attached via the plugin API would appear in the order in which they were attached.

@nickvergessen would you consider this a middle-ground compromise?

[nickvergessen]

Ah you don't have the priority in the frontend, the backend sorts it and does not expose it.

I guess that sounds like the best option then

[nfebe]

Ah you don't have the priority in the frontend, the backend sorts it and does not expose it.

I guess that sounds like the best option then

Sure!

Before	After

https://github.com/nextcloud/spreed/pull/11672

cc: @nickvergessen @jancborchardt

[nfebe]

/compile

[Antreesy]

Clicking on Load more results changes the filtering method (or drops some query params at least). Is it expected?

Screencast from 05.03.2024 17:29:31.webm

[nfebe]

/compile /

[nfebe]

Outside of cases where to make the code clearer and more maintainable for the future, good PR :) Discussed with others, and the API deprecation can be handled later, just make sure to address https://github.com/nextcloud/server/pull/43189#issuecomment-1979172940 and its all good to me!

Sure thanks, would address that in follow up https://github.com/nextcloud/server/pull/43963 which is on its way in, I think it's best this goes in now so at the very least the spreed filter is working by feature freeze.