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.

Sign up for GitHub

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

Jump to bottom

Fix: Remember opened tag in search and send previous index #1966

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Fix: Remember opened tag in search and send previous index #1966

merged 1 commit into from
Mar 17, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Mar 17, 2020

In #1941 we introduced two regressions and this patch fixes them.

  • search needs to remember the opened tag and limit results to those
    matching within that tag. this was overlooked when updating the
    search query and updating the filterTags from that search query

  • when passing the filterNotes action back to the application we
    have to make sure to pass the previous index. this was lost during
    the merge of the PR because because the additions from Move previous index store to Redux #1919, which
    fixed the previous regression for it, were deleted as part of
    moving ui/middleware into search/index

With these changes in place the two regressions should be fixed again.

Testing

Apart from the normal checklist note that #1941 introduced regressions
to the following items. Verify that they are fixed in this branch.

  • Selects note above recently-trashed note
  • Selects note above recently-restored note in trash view
  • Selects note above recently-deleted-forever note in trash view
  • Can search by keyword with tag selected

@dmsnell dmsnell added the Defect label Mar 17, 2020
@dmsnell dmsnell added this to the 1.16 milestone Mar 17, 2020
@dmsnell dmsnell requested a review from a team March 17, 2020 05:32
@dmsnell
Fix: Remember opened tag in search and send previous index
b0d30ad 
In #1941 we introduced two regressions and this patch fixes them.

 - search needs to remember the opened tag and limit results to those
   matching within that tag. this was overlooked when updating the
   search query and updating the `filterTags` _from_ that search query

 - when passing the `filterNotes` action back to the application we
   have to make sure to pass the previous index. this was lost during
   the merge of the PR because because the additions from #1919, which
   fixed the previous regression for it, were deleted as part of
   moving `ui/middleware` into `search/index`

With these changes in place the two regressions should be fixed again.
@dmsnell dmsnell force-pushed the fix/search-track-tags-and-previous-index branch from 4c35ce7 to b0d30ad Compare March 17, 2020 05:51
belcherj
belcherj approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

When you first load after clearing application data it doesn't always select the top note. I suspect that it hasn't finished loading all the notes before it selects a note.

This fixes the issues. Let's merge and then resolve the above issue if we think it is an issue.

@dmsnell dmsnell merged commit 1001409 into develop Mar 17, 2020
@dmsnell dmsnell deleted the fix/search-track-tags-and-previous-index branch March 17, 2020 16:38
@dmsnell
Copy link
Member Author

dmsnell commented Mar 19, 2020

Introduced a big performance regression when searching: since this selects the previous note on ever search we can end up rendering and re-rendering slow notes in the draft-js editor. This kills interactivity.

dmsnell added a commit that referenced this pull request Mar 26, 2020
@dmsnell
Search: Stop auto-selecting previous index (performance)
fc301ff 
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 dmsnell mentioned this pull request Mar 26, 2020
Merged
dmsnell added a commit that referenced this pull request Mar 30, 2020
@dmsnell
Search: Stop auto-selecting previous index (performance) (#1979)
0db494a 
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 dmsnell mentioned this pull request Apr 2, 2020
Merged
dmsnell added a commit that referenced this pull request Apr 2, 2020
@dmsnell
Search: Move out of WebWorker and back into main thread
df573c1 
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
@dmsnell
Search: Move out of WebWorker and back into main thread (#1982)
429c470 
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
Reviewers

@belcherj belcherj belcherj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.16
Development

Successfully merging this pull request may close these issues.
2 participants
@dmsnell @belcherj