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

Content search highlighting #5817

Merged
merged 6 commits into from
Jun 12, 2024
Merged

Content search highlighting #5817

merged 6 commits into from
Jun 12, 2024

Conversation

elzody
Copy link
Contributor

@elzody elzody commented May 20, 2024

💬 Summary

Part of #5816
This PR implements search highlighting in Text. It is mainly a TipTap extension which subscribes to an event emitted with a search query, and will add highlighting styles to the editor document if matches to the search query are found.

How to test this PR From the browser console, emit an event with the event bus:
_nc_event_bus.emit('text:editor:search', { query: 'Test' })

Replace 'Test' with the string you want to search for.

Screenshot

image

📝 TODO

  • search highlighting

☑️ Checklist

  • Tests
  • Formatting + Linting
  • Signed-off commits

@elzody elzody added this to the Nextcloud 30 milestone May 20, 2024
@elzody elzody self-assigned this May 20, 2024
@elzody elzody mentioned this pull request May 20, 2024

This comment was marked as off-topic.

@elzody elzody force-pushed the feat/content-search-highlight branch from 0b79206 to 9bef1db Compare June 5, 2024 16:08
@elzody elzody marked this pull request as ready for review June 5, 2024 16:41
@elzody elzody linked an issue Jun 6, 2024 that may be closed by this pull request
@elzody elzody removed a link to an issue Jun 6, 2024
@juliusknorr
Copy link
Member

For easier testing just paste this in the js console:

_nc_event_bus.emit('text:editor:search', { query: 'Test' })

@juliusknorr
Copy link
Member

Some odd behaviour seems that the search is reset when a sync request comes in, that would still be good to fix, otherwise this looks really good 👍

@elzody elzody force-pushed the feat/content-search-highlight branch from 9bef1db to db68c8b Compare June 6, 2024 19:17
@elzody
Copy link
Contributor Author

elzody commented Jun 6, 2024

Some odd behaviour seems that the search is reset when a sync request comes in...

Interestingly, it doesn't happen for me in Collectives, but when I test it by making a plain markdown document in Files and doing a search, it does this. Will look into it further.

UPDATE: I think I fixed it :D

@juliushaertl

@elzody elzody requested a review from grnd-alt June 7, 2024 18:55
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

How would i clear the highlight now?

@elzody
Copy link
Contributor Author

elzody commented Jun 10, 2024

@max-nextcloud Sending an empty string as a search query should clear the highlighting.

This PR is mostly to get the foundation of the search highlighting into Text, and other apps can then use it in their own way and provide the user with a way to clear the highlighting, and it will then just send an empty search query to Text to clear the highlighting

I am also open to other suggestions on how to clear the highlighting if an empty search query doesn't make sense for it

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

One tweak still. Approving anyway so you can move on.

if (tr.docChanged || (newQuery !== oldQuery)) {
query = newQuery
} else {
query = oldQuery
Copy link
Collaborator

@max-nextcloud max-nextcloud Jun 12, 2024

Choose a reason for hiding this comment

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

I think you can just return oldState the previous value here. The doc did not change and the query also stayed the same.

That way you also don't need the query variable and can just use newQuery.

Copy link
Collaborator

@max-nextcloud max-nextcloud Jun 12, 2024

Choose a reason for hiding this comment

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

Oh... the way this is defined in prosemirror is slightly confusing as oldState contains the entire state. I think you will need to use all four args of the function so you can reuse the previous value of this plugins state:

apply(tr, value, oldState, newState) {
	...
	if (tr.docChanged || (newQuery !== oldQuery)) {
		...
	} else {
		return value // same as before
	}

Copy link
Contributor Author

@elzody elzody Jun 12, 2024

Choose a reason for hiding this comment

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

So, I think you mean something like this?

apply(tr, value, oldState, newState) {
    const { query: oldQuery } = searchQueryPluginKey.getState(oldState)
    const { query: newQuery } = searchQueryPluginKey.getState(newState)

    if (tr.docChanged || (newQuery !== oldQuery)) {
        const searchResults = runSearch(tr.doc, newQuery)
        return highlightResults(tr.doc, searchResults)
    } else {
        return value
    }
},

It was indeed kind of confusing, I thought it the value parameter there was something I could use, but the docs did not really elaborate on it, so originally I operated on the oldState to be safe. But this would make more sense.

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
@elzody elzody force-pushed the feat/content-search-highlight branch from a6db799 to 59cec5c Compare June 12, 2024 13:34
@elzody elzody enabled auto-merge (squash) June 12, 2024 13:36
@elzody elzody merged commit 25292a2 into main Jun 12, 2024
60 checks passed
@elzody elzody deleted the feat/content-search-highlight branch June 12, 2024 14:02
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.

3 participants