Search: Stop auto-selecting previous index (performance) #1979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In #1941 we introduced instant search results. In #1919 and in #1966 we refactored the `previousIndex` into Redux The result was that we weren't resetting the `previousIndex` properly and so on every search we would select _some_ note, often the first one in the list. If this note were large or slow to render then the performance gains we achieved with search were destroyed by the cost of rendering the notes. In this patch we're properly resetting the `previousNote` whenever we actually filter the notes. This means that searches will clear the selected note if it's not in the search results. This also brings back the performance gains we got by refactoring search itself.
dmsnell
added a commit
that referenced
this pull request
Apr 2, 2020
Fixes #1941 Supplants #1966 Supplants #1979 It turns out that having search inside a WebWorker isn't as necessary as we thought and at the same time presents one major obstacle: search must be asynchronous. This causes a few problems with the app is currently designed, namely that the interaction between `previousIndex`, trash/delete/restore operations, and the note list. The primary goal of the larger state-refactor project has been to eliminate non-atomic state updates and the WebWorker's asyncronous mandate means that there is no way to synchronously update state, which means that there's no way to run the trashing actions at the same time that we update the search filter. This leaves an awkward rendered state where the note in the note list is trashed and the toolbar above it shows the trash "Delete forever" and "Restore" buttons but we're still looking at the note list. In order to resolve these bugs and eliminate further issues I have brought the search back into the main thread. Why? Won't this destroy all that we gained in terms of performance?" you might ask. No, actually most of the performance gain came from changes I made to the search mechanism _while_ moving it into a WebWorker. I had wanted it to be in a WebWorker because it was slow, but now that we can see that it's very very fast and shouldn't be a UI blocker we don't have the same motivation to get it out of the main thread. In summary, this patch moves the search back into the main thread and exposes the `updateFilter()` function so that it can be called synchronously for actions which demand immediate UI updates.
dmsnell
added a commit
that referenced
this pull request
Apr 3, 2020
Fixes #1941 Supplants #1966 Supplants #1979 It turns out that having search inside a WebWorker isn't as necessary as we thought and at the same time presents one major obstacle: search must be asynchronous. This causes a few problems with the app is currently designed, namely that the interaction between `previousIndex`, trash/delete/restore operations, and the note list. The primary goal of the larger state-refactor project has been to eliminate non-atomic state updates and the WebWorker's asyncronous mandate means that there is no way to synchronously update state, which means that there's no way to run the trashing actions at the same time that we update the search filter. This leaves an awkward rendered state where the note in the note list is trashed and the toolbar above it shows the trash "Delete forever" and "Restore" buttons but we're still looking at the note list. In order to resolve these bugs and eliminate further issues I have brought the search back into the main thread. Why? Won't this destroy all that we gained in terms of performance?" you might ask. No, actually most of the performance gain came from changes I made to the search mechanism _while_ moving it into a WebWorker. I had wanted it to be in a WebWorker because it was slow, but now that we can see that it's very very fast and shouldn't be a UI blocker we don't have the same motivation to get it out of the main thread. In summary, this patch moves the search back into the main thread and exposes the `updateFilter()` function so that it can be called synchronously for actions which demand immediate UI updates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #1941 we introduced instant search results.
In #1919 and in #1966 we refactored the
previousIndexinto ReduxThe result was that we weren't resetting the
previousIndexproperlyand so on every search we would select some note, often the first
one in the list. If this note were large or slow to render then the
performance gains we achieved with search were destroyed by the cost
of rendering the notes.
In this patch we're properly resetting the
previousNotewheneverwe actually filter the notes. This means that searches will clear
the selected note if it's not in the search results. This also brings
back the performance gains we got by refactoring search itself.
Testing
Open an account with large or slow-to-render notes.
Search for almost anything.
In
developyou might notice that the app becomes unresponsive.In this branch you should see full responsiveness.
Verify that when trashing, restoring, and permanently deleting a
note that the one above it in the note list gets selected.