Feat: Create filter-plugin architecture for unified search #43189

Fenn-CS · 2024-01-29T11:22:23Z

Description

This commit introduces the mechanism for apps out of the call, to add search filters to the unified search "Places" filter selector.
Resolves : #42914

TODO

Ensure nothing regarding this feature is triggered if talk is not installed
Document how to add filters to unified search, somewhere in developer notes? (Transferred to : Document Unified Search API documentation#11544)

Manual Testing

Set spreed/talk branch to : nextcloud/spreed#11575

jancborchardt

Looks good, 2 details:

As @marcoambrosini said: Rename the current "Conversations" filter to "Talk" for better clarity
The "In conversation" should be moved further up so it is grouped with the other Talk-related filters

Fenn-CS · 2024-02-06T10:24:55Z

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.

core/src/components/UnifiedSearch/ConversationsListModal.vue

marcoambrosini · 2024-02-08T08:00:10Z

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?

marcoambrosini

Looks good! The only thing that I would add is a search box on top of the places filter too. These can be a lot (see our company instance) and it might also get confusing.
It would be also great to set up search aliases, for example if someone searches "Chat", they're also presented with the "talk" and "conversation" filters.

Fenn-CS · 2024-02-12T16:37:38Z

The context of the PR has changed and actual UI changes that his PR initially contained are now in : nextcloud/spreed#11575

Fenn-CS · 2024-03-04T08:31:02Z

/compile

core/src/store/unified-search-external-filters.js

core/src/views/UnifiedSearchModal.vue

core/src/unified-search.js

core/src/store/unified-search-external-filters.js

core/src/unified-search.js

skjnldsv · 2024-03-04T17:09:52Z

core/src/views/UnifiedSearchModal.vue

@@ -258,8 +263,13 @@ export default {

 	},
 	mounted() {
+		subscribe('nextcloud:unified-search:add-filter', this.handlePluginFilter)


That's a no from me, using an event to actually handle this registrations are not a stable way to do so.

I'd suggest using a proper registration like the event bus or the file actions is doing.

skjnldsv · 2024-03-04T17:10:31Z

core/src/views/UnifiedSearchModal.vue

@@ -412,12 +423,27 @@ export default {
 		},
 		addProviderFilter(providerFilter, loadMoreResultsForProvider = false) {
 			if (!providerFilter.id) return
+			if (providerFilter.isPluginFilter) {
+				providerFilter.callback()


What is a callback? What does it do?
A callback that is called without any parameters is odd to me.

emoral435

Overall, good PR!

There are a few places where it gets confusing between the name and label attributes, and I agree with the API choices that the other reviewers have mentioned. Perhaps for tomorrows team call / Thursday PR chat's, you can schedule something to be aligned with the team, because I understand the time conflicts are not easy ahaha :)

core/src/store/unified-search-external-filters.js

core/src/unified-search.js

core/src/views/UnifiedSearchModal.vue

Dismissing my review as the code/UI parts of Talk are not here anymore

Antreesy · 2024-03-05T16:32:31Z

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

emoral435

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 #43189 (comment) and its all good to me!

This commit introduces the mechanism for apps out of the call, to add search filters to the unified search "Places" filter selector. Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Fenn-CS · 2024-03-06T01:06:11Z

/compile /

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>

Fenn-CS · 2024-03-06T01:28:57Z

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 #43189 (comment) and its all good to me!

Sure thanks, would address that in follow up #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.

addressed

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch 7 times, most recently from dd46a77 to 82cbdb7 Compare February 6, 2024 09:51

jancborchardt reviewed Feb 6, 2024

View reviewed changes

jancborchardt assigned Fenn-CS and marcoambrosini and unassigned marcoambrosini Feb 6, 2024

jancborchardt requested review from nickvergessen, Altahrim and marcoambrosini February 6, 2024 10:06

nickvergessen previously requested changes Feb 6, 2024

View reviewed changes

core/src/components/UnifiedSearch/ConversationsListModal.vue Outdated Show resolved Hide resolved

marcoambrosini reviewed Feb 8, 2024

View reviewed changes

marcoambrosini mentioned this pull request Feb 8, 2024

Add search box to "Places" filter #43442

Open

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch 4 times, most recently from c5920f4 to 33a4fb5 Compare February 12, 2024 16:31

Fenn-CS changed the title ~~Feat: Narrow search to single conversation in unified search~~ Feat: Create filter-plugin architecture for unified search Feb 12, 2024

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch 4 times, most recently from e25f0ff to 71da116 Compare February 12, 2024 22:14

Fenn-CS marked this pull request as ready for review February 12, 2024 22:22

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch 6 times, most recently from 81a824e to 857477c Compare March 4, 2024 08:30

ChristophWurst reviewed Mar 4, 2024

View reviewed changes

core/src/store/unified-search-external-filters.js Outdated Show resolved Hide resolved

core/src/views/UnifiedSearchModal.vue Outdated Show resolved Hide resolved

core/src/unified-search.js Outdated Show resolved Hide resolved

ChristophWurst added the pending documentation This pull request needs an associated documentation update label Mar 4, 2024

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch from 6754a8c to 0c38e89 Compare March 4, 2024 16:01

emoral435 self-requested a review March 4, 2024 16:11

skjnldsv reviewed Mar 4, 2024

View reviewed changes

core/src/store/unified-search-external-filters.js Show resolved Hide resolved

skjnldsv previously requested changes Mar 4, 2024

View reviewed changes

emoral435 reviewed Mar 5, 2024

View reviewed changes

core/src/store/unified-search-external-filters.js Show resolved Hide resolved

core/src/unified-search.js Outdated Show resolved Hide resolved

core/src/views/UnifiedSearchModal.vue Show resolved Hide resolved

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch from 0c38e89 to f61aee4 Compare March 5, 2024 10:54

Fenn-CS requested a review from skjnldsv March 5, 2024 11:12

nickvergessen removed their request for review March 5, 2024 11:34

emoral435 approved these changes Mar 5, 2024

View reviewed changes

Fenn-CS added 2 commits March 6, 2024 02:05

feat(core): create filter-plugin architecture for unified search

d35a49c

This commit introduces the mechanism for apps out of the call, to add search filters to the unified search "Places" filter selector. Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

refactor: migrate from vuex to pinia

647f4bc

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Fenn-CS force-pushed the 42914-search-in-single-talk-room branch from f61aee4 to 647f4bc Compare March 6, 2024 01:06

chore(assets): Recompile assets

a5c207f

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>

Fenn-CS merged commit 0a64b69 into master Mar 6, 2024
98 checks passed

Fenn-CS deleted the 42914-search-in-single-talk-room branch March 6, 2024 10:07

blizzz mentioned this pull request Mar 7, 2024

29.0.0 beta 1 #43991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Create filter-plugin architecture for unified search #43189

Feat: Create filter-plugin architecture for unified search #43189

Fenn-CS commented Jan 29, 2024 •

edited

Loading

jancborchardt left a comment

Fenn-CS commented Feb 6, 2024

marcoambrosini commented Feb 8, 2024

marcoambrosini left a comment

Fenn-CS commented Feb 12, 2024

Fenn-CS commented Mar 4, 2024

skjnldsv Mar 4, 2024

skjnldsv Mar 4, 2024

emoral435 left a comment •

edited

Loading

Antreesy commented Mar 5, 2024

emoral435 left a comment

Fenn-CS commented Mar 6, 2024

Fenn-CS commented Mar 6, 2024

Feat: Create filter-plugin architecture for unified search #43189

Feat: Create filter-plugin architecture for unified search #43189

Conversation

Fenn-CS commented Jan 29, 2024 • edited Loading

Description

jancborchardt left a comment

Choose a reason for hiding this comment

Fenn-CS commented Feb 6, 2024

marcoambrosini commented Feb 8, 2024

marcoambrosini left a comment

Choose a reason for hiding this comment

Fenn-CS commented Feb 12, 2024

Fenn-CS commented Mar 4, 2024

skjnldsv Mar 4, 2024

Choose a reason for hiding this comment

skjnldsv Mar 4, 2024

Choose a reason for hiding this comment

emoral435 left a comment • edited Loading

Choose a reason for hiding this comment

Antreesy commented Mar 5, 2024

emoral435 left a comment

Choose a reason for hiding this comment

Fenn-CS commented Mar 6, 2024

Fenn-CS commented Mar 6, 2024

Fenn-CS commented Jan 29, 2024 •

edited

Loading

emoral435 left a comment •

edited

Loading