Move previous index store to Redux #1919

belcherj · 2020-02-20T15:44:33Z

Fix

Moves the previousIndex state from app state to redux. This implementation is mimicking what we were doing in app state and should not be considered a final implementation of how this should be done. One app state is migrated we can take a better look at how to implement this properly.

Also a fix for:

Selects note above recently-trashed note ⭕️ selects top note in list 5
Selects note above recently-restored note in trash view ⭕️ selects top note in list 5
Selects note above recently-deleted-forever note in trash view ⭕️ selects top note in list 5

In #1851 we inadvertently broke this behavior and then in #1895 removed the mechanism that had previously enabled it.

Taken from: #1911 Thanks @dmsnell

Test

Test that selects note above recently-trashed note
Test that selects note above recently-restored note in trash view
Tets that selects note above recently-deleted-forever note in trash view

dmsnell · 2020-02-22T02:13:35Z

lib/state/ui/middleware.ts

@@ -8,7 +8,9 @@ let searchTimeout: NodeJS.Timeout;

 export const middleware: S.Middleware = store => {
  const updateNotes = () =>
-    store.dispatch(filterAction(filterNotes(store.getState())));
+    store.dispatch(
+      filterAction(filterNotes(store.getState()), store.getState().ui.noteIndex)


What was it needed for here?

If we change the note reducer to return null in cases where the note is no longer in the filtered list (instead of using action.notes[0]) then could we eliminate passing it here? Whereas the <NoteList> could say "oh, I have a null note but I have a previousIndex" and then keep that logic as a ~~render-tie~~ render-time concern?

FILTER_NOTES gets run on most actions and will clobber any attempt to set the selected note with notes[0] if none is selected. https://github.com/Automattic/simplenote-electron/pull/1919/files#diff-dfb3add3b5c868976c680f25330e1c32L191

If we remove this functionality from the note reducer for FILTER_NOTES we lose note selection on search when a note no longer exists in the results.

I have to wonder if it will easier to sort this out sometime down the road when we have more control in the Redux state.

dmsnell · 2020-02-26T19:31:03Z

Testing

Notes

While the Markdown setting transferred and the preview button appeared and disappeared as expected, the <NoteList /> in the receiving window (this build) didn't update in response to the Markdown. I believe that this is existing behavior in the app and unrelated to this PR. The note list should immediately re-render and show the "plaintext markdown syntax" when Markdown is disabled for the note but it stays in the rendered preview, removing the leading # from the note title.
The trash/restore cycle renders some unexpected interstitial screens where the note renders as a deleted note but isn't removed from the display. In develop the note immediately disappears and turns into a Simplenote logo on a blank background in the editor pane. Fixed in additional commits

In my test I was only able to use ⇥ and ⇧+⇥ for altering the list indentation when the rest of the line was blank. this is an existing behavior.
Linkifying only appears to be happening in the markdown preview. This is an existing behavior.
Something remains seriously wonky with printing. I can only print once per application session. I verified also by building the app with NODE_ENV=production and trying again. Also it does a funny shimmy when it opens the print dialog and also when it actually prints. Must be something funny with window.print() I'm guessing - possibly stylesheet related?

† these are known existing issues

In this change we're clearing out the selected note immediate when performing the trash/restore/delete-forever operations to avoid showing an unnecessary interstitial Simplenote logo or the newly-rendered note contents. In addition to clearing out the note state we are immediately triggering a note filter operation to update the note list and avoid a similar awkward in-between state.

belcherj · 2020-02-26T20:09:12Z

Testing

Logout
Login with wrong password fails
Login with correct password
Create new note appears in other device
Changes to new note sync to other device
Changes to new note sync from other device (after debounce delay)
New tag immediately syncs to other device
New tag immediately syncs from other device
Removed tag immediately syncs to other device
Removed tag immediately syncs from other device
Note publishes with link
Note unpublishes
Note publish change syncs from other device (visible with dialog open)
Markdown setting syncs from other device
Preview mode disappears/reappears when receiving remote changes to markdown setting
Note pinning syncs immediately from either direction
Note pinning works regardless if clicking in list view or from note info
Viewing history on one device leaves note unchanged on other device
Restoring history immediately syncs note from both directions
Restoring history revisions updates pinned status
Restoring history revisions updates markdown status
Restoring history revisions updates publish status
Can view trashed notes by clicking on Trash
Can delete note forever from trash screen
Can restore note from trash screen
Can trash note
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 preview markdown with 👁 button
Can flip to edit mode with 👁 button
Markdown rendered in note list when markdown enabled
Markdown syntax unrendered in note list when markdown disabled
Can filter by tag when clicking on tag in tag drawer
Can add tag to note and have it appear in filtered tag view when previously not in filter
Can search by keyword with tag selected
Searching in the search field highlights matches in note list
Searching in the search field highlights matches in the note editor
Searching in the search field highlights matches in the note editor during preview
Clearing the search field immediately updates filtered notes
Clicking on different tags or All Notes or Trash immediately updates filtered notes
Can search by keyword, filtering debounced
Tag auto-completes appear when typing in search field
Typing tag: does not result a list of all tags in the autocompleter
Typing tag: and something else, like tag:te results in autocomplete results starting with that something else, e.g. test
Tag suggestions suggest tags regardless of case
Search field updates with results of tag:test format search string
Can toggle sidebar
Syncs when introducing sequential surrogate pairs sharing the same high surrogate, e.g. 🅰🅱 to 🅰🅰🅱
When disabling network connectivity, changes start counting in unsync'ed changes counter
When going back online changes sync and counter resets to All chagnes synced
Can change analytics sharing setting
Changing Note Display mode immediately updates and reflects in note list
With sidebar disabled, toggling Line Length between Narrow and Full removes and adds border around note content appropriately and immediately.
Changing Sort Type mode immediately updates and reflects in note list
Toggling Sort Order immediately updates and reflects in note list for each sort type
For each sort type the pinned notes appear first in the note list
Changing Theme immediately updates app for desired color scheme
Using the Insert checklist item from the format menu inserts a checklist
"Undo" undoes the last edit
Typing - [x] creates a checked checklist item
Typing - [ ] created an unchecked checklist item
Typing - creates a list
Typing tab in a list item underneath another list item indents item
Typing shift tab in the same spot unindents the list
Changing - to + changes the list item bullet, also for * and • (\u2022)
All list bullet types render to markdown lists
Added URL is linkified
When clicking on link it opens in new window
CmdOrCtrl + shift + p toggles preview mode
CmdOrCtrl + shift + (k or up) selects next note above current note, stops at top of list
CmdOrCtrl + shift + (j or down) selects next note below current note, stops at bottom of list
CmdOrCtrl + t toggles tab list
CmdOrCtrl + n creates new note
CmdOrCtrl + shift + n when in small screen mode navigates to note list
Can print note in plaintext view
Can print note in Markdown-preview view

dmsnell

Tested and everything seems ship-shape as it relates to this PR. The previous behaviors work as expected. Thanks @belcherj

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.

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.

belcherj self-assigned this Feb 20, 2020

belcherj requested a review from a team February 20, 2020 16:12

belcherj added the [status] needs code review label Feb 20, 2020

belcherj marked this pull request as ready for review February 20, 2020 16:12

dmsnell reviewed Feb 22, 2020

View reviewed changes

belcherj added 3 commits February 24, 2020 13:48

Move previous index store to Redux

19115ff

Account for close notes that dont set index

4cdbbe6

Remove debugging code

eb57255

belcherj force-pushed the refactor/previous-index-to-redux branch from 4f331a8 to eb57255 Compare February 24, 2020 18:48

Rework the previousIndex logic

65e23d5

belcherj and others added 3 commits February 26, 2020 14:45

Properly set selected note to null on trashing

ab3798a

Missed some obvious references

3f4b146

dmsnell approved these changes Feb 26, 2020

View reviewed changes

belcherj merged commit 1127d65 into develop Feb 26, 2020

belcherj deleted the refactor/previous-index-to-redux branch February 26, 2020 20:35

dmsnell mentioned this pull request Mar 17, 2020

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

Merged

4 tasks

dmsnell mentioned this pull request Mar 26, 2020

Search: Stop auto-selecting previous index (performance) #1979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move previous index store to Redux #1919

Move previous index store to Redux #1919

belcherj commented Feb 20, 2020 •

edited

Loading

dmsnell Feb 22, 2020 •

edited

Loading

belcherj Feb 25, 2020

belcherj Feb 25, 2020

dmsnell commented Feb 26, 2020 •

edited

Loading

belcherj commented Feb 26, 2020 •

edited

Loading

dmsnell left a comment

Move previous index store to Redux #1919

Move previous index store to Redux #1919

Conversation

belcherj commented Feb 20, 2020 • edited Loading

Fix

Test

dmsnell Feb 22, 2020 • edited Loading

Choose a reason for hiding this comment

belcherj Feb 25, 2020

Choose a reason for hiding this comment

belcherj Feb 25, 2020

Choose a reason for hiding this comment

dmsnell commented Feb 26, 2020 • edited Loading

Testing

Notes

belcherj commented Feb 26, 2020 • edited Loading

Testing

dmsnell left a comment

Choose a reason for hiding this comment

belcherj commented Feb 20, 2020 •

edited

Loading

dmsnell Feb 22, 2020 •

edited

Loading

dmsnell commented Feb 26, 2020 •

edited

Loading

belcherj commented Feb 26, 2020 •

edited

Loading