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

Update note: user needs to switch to another one, and go back to get the updated result #1879

Closed
surfer0627 opened this issue Feb 4, 2020 · 2 comments · Fixed by #1880
Closed

Update note: user needs to switch to another one, and go back to get the updated result #1879

surfer0627 opened this issue Feb 4, 2020 · 2 comments · Fixed by #1880
Labels
bug Something isn't working

Comments

@surfer0627
Copy link

Steps to reproduce:

  1. Open and login to simplenote.com.
  2. Select note (e.g. name: xxx)
  3. Edit note. (add / delete text)
  4. Press "preview" button.

Actual behavior:

Unable to update note.

Expected behavior:

Note updated successfully.

  1. Select note (e.g. name: yyy)
  2. Select note (e.g. name: xxx again)
    (Now, note xxx updated successfully.

System configuration:

Windows version:

6.1.7601 service pack 1

Name and version of other software in use when reproducing the issue:

screen reader: NVDA 2019.2.1
@surfer0627 surfer0627 added the bug Something isn't working label Feb 4, 2020
dmsnell added a commit that referenced this issue Feb 4, 2020
@dmsnell
Fix: Update selected note when making changes in the editor view
c7a331b 
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
@dmsnell dmsnell mentioned this issue Feb 4, 2020
Merged
@dmsnell
Copy link
Member

dmsnell commented Feb 4, 2020

Thanks @surfer0627 - it appears that some changes we're making (to stabilize the platform) introduced this without us noticing. Should be fixed soon!

@surfer0627
Copy link
Author

@dmsnell
Thank you for all of your kindly help.

This was referenced Feb 5, 2020
Closed
Closed
dmsnell added a commit that referenced this issue Feb 6, 2020
@dmsnell
Fix: Update selected note when making changes in the editor view (#1880)
9c14685 
* Fix: Update selected note when making changes in the editor view

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

* Handle tag updates too by always re-selecting note on updates from the 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: Update selected note when making changes in the editor view Automattic/simplenote-electron
2 participants
@dmsnell @surfer0627