-
Notifications
You must be signed in to change notification settings - Fork 565
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: Update selected note when making changes in the editor view #1880
Merged
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
Resolves #1879 It appears that we removed a flow in #1851 which updated app state in response to updates from the note editor[1]. While this didn't impact the operation of the editor it did present itself as a bug when previewing markdown-rendered notes. The editor is stateful on its own and so continued to function independently of the state of the selected note's content. The preview, however, grabs the contents of the selected note whenever it's activated and then renders those into a DOM node. This means that it showed the old contents of the note unless you navigated to another note and back, which swapped out the selected note with the new updated contents. In this patch, which currently is probably wrong in some way, we're updating the selected note when calling `onChangeContent` so that we don't get out of sync with the data sources. Hopefully at the end of our state cleanup and stabilization project this will disappear but for now this fix catches the gap that left split data sources. Probably there were other bugs we didn't discover as a result of this. Mainly we need to test and audit for traces of cycles/loops in the update so that we don't start spinning or overwriting data we just entered. In my brief testing the easy cases worked as I expected them to simply and singly. [1]: https://github.com/Automattic/simplenote-electron/pull/1851/files#diff-3c87750a7cc32e0127447ffc78f5c0d5L300
belcherj
approved these changes
Feb 4, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test but code looks good.
…e server When tags were being updated in the pap they also came through this function but they weren't coming through `onUpdateContent`, meaning that our fix overlooked tags and probably also system tags and the like. In this patch we're no longer selectively updating the note if it has a remote patch (indicating that it came from another device). Instead, we're always updating and selectively setting `hasRemoteUpdate` based on that knowledge. This ensures that we maintain consistency with local updates.
Testing
|
Merging over failing tests since they are failing for other reasons; something in the AppVeyor config. |
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.
Resolves #1878
Resolves #1879
Resolves #1882
It appears that we removed a flow in #1851 which updated app state in response
to updates from the note editor1. While this didn't impact the operation of
the editor it did present itself as a bug when previewing markdown-rendered
notes. The editor is stateful on its own and so continued to function
independently of the state of the selected note's content. The preview,
however, grabs the contents of the selected note whenever it's activated and
then renders those into a DOM node. This means that it showed the old contents
of the note unless you navigated to another note and back, which swapped out
the selected note with the new updated contents.
In this patch, which currently is probably wrong in some way, we're updating
the selected note when calling
onChangeContent
so that we don't get out ofsync with the data sources. Hopefully at the end of our state cleanup and
stabilization project this will disappear but for now this fix catches the
gap that left split data sources.
Probably there were other bugs we didn't discover as a result of this.
Mainly we need to test and audit for traces of cycles/loops in the update
so that we don't start spinning or overwriting data we just entered. In my
brief testing the easy cases worked as I expected them to simply and singly.
In
develop
In this branch
Props to @surfer0627 for reporting