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

Fix search selections when content is changed #2846

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

sandymcfadden
Copy link
Contributor

@sandymcfadden sandymcfadden commented Apr 15, 2021

Fix

Fixes #2809

Update:
There are still a few issues search selection when the content of the note has changed. I've changed this back to a draft to come back and address sometime in the future.

Currently, when you search for a term, change the selected search match, then edit the content to remove a search match the selected search indicators can become incorrect. For example, it could say you are at match 4 of 3.
This corrects that.

Test

  1. Have a note with multiple instances of word in it
  2. Search for word
  3. Move the current match position in a note with > button or with Ctrl/Cmd+G, so that it becomes greater than one (e.g. 5 of 5)
  4. Start removing the instances of word until one remains ~> the indicator will show 5 of 1

Release

  • Fixed issue where the search results navigation bar could show incorrect position if the content of a note was changed.

@sandymcfadden sandymcfadden added this to the 2.10.0 milestone Apr 15, 2021
@sandymcfadden sandymcfadden requested a review from a team April 15, 2021 11:02
@sandymcfadden sandymcfadden self-assigned this Apr 15, 2021
@codebykat
Copy link
Member

Hmm. So testing this out, the search highlight seems to vanish. Is that expected?

for example, here I have a search and I'm on "8 of 13"

Screen Shot 2021-04-15 at 11 57 04 PM

I edit a match prior to the one I'm currently focused on, and it shows "8 of 12", but nothing is highlighted.

Screen Shot 2021-04-15 at 11 57 14 PM

@sandymcfadden sandymcfadden modified the milestones: 2.10.0, 2.11.0 Apr 18, 2021
@sandymcfadden
Copy link
Contributor Author

Thank you. I didn't catch that case. I'm not sure how to proceed here when the content changes and there is a search selection.

Right now we store the index in the array of matched search results. So if the content changes that index may not be valid anymore. In that case, do we reset the selection to the first search match?

Should we also store the range for the current search match selection? Then if the content changes we could try to select the closest match to the previously selected range?

The case I tried to address here was pretty easy. If the selected index is higher than the number of matches we have, select the last one instead, but the other cases seem more difficult.

@sandymcfadden sandymcfadden modified the milestones: 2.11.0, 2.12.0 May 1, 2021
@codebykat
Copy link
Member

Hmm. I would say we reset it to the nearest valid search selection if we can't figure out what was actually selected before the content changed. I am honestly not sure how that would feel. We could try resetting to the first selection too and just see what feels better.

I think it feels buggy to have it still say "8 of 12" or whatever without a selection being highlighted at all, so we should try to handle that case somehow.

Should we also store the range for the current search match selection? Then if the content changes we could try to select the closest match to the previously selected range?

This is a cool idea.

@sandymcfadden sandymcfadden modified the milestones: 2.12.0, 2.13.0 May 15, 2021
@sandymcfadden sandymcfadden modified the milestones: 2.13.0, 2.14.0 May 28, 2021
@sandymcfadden sandymcfadden modified the milestones: 2.14.0, Future Jun 28, 2021
@belcherj belcherj removed the request for review from a team September 27, 2021 12:34
@belcherj
Copy link
Contributor

Removed the reviewers on this PR until we are ready to take another look.

@sandymcfadden sandymcfadden marked this pull request as draft September 30, 2021 16:56
@codebykat codebykat added not verified The reported issue was not repro'ed yet [feature] search Anything related to searching. labels May 9, 2024
@codebykat
Copy link
Member

We need to see if we can still reproduce this, I refactored the search decorations in #3183 and I believe they are being recalculated when the model changes, but I'm not sure if the selected index is being properly invalidated.

@dmsnell
Copy link
Contributor

dmsnell commented May 15, 2024

it looks like this is fairly stale, but just in case, it would be nice to see if it can be approached through middleware instead of as an action dispatched from the view component. I'm sure there are a number of edges where things jump around unexpectedly because of this.

@belcherj
Copy link
Contributor

Hi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[feature] search Anything related to searching. not verified The reported issue was not repro'ed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Search] Match position indicator does not update when matches number changes
4 participants