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.

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: Sort revisions by note version #1605

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Sep 30, 2019

See #394 where the sorting was introduced

When we make changes to a note we append a modificationDate value
which serves as a timestamp of the change on the client device making
the change
. That is, the timestamp is meant to provide an indicator of
when in real time the change happened, but since it's based on a
device's clock we cannot rely on it or use it for proper sequencing.

It happens from time to time (especially when one device is offline)
that a revision which occurs after another remote change gets a
timestamp showing that it happened before because the local device
recorded the timestamp before the change made it to the Simperium
server.

When this happens the application will show those revisions out of the
actual order they took place and this can lead to confusing note
history. In this patch we're sorting the revisions based on the version
of the note and not the timestamp. This produces the proper sequencing
of changes in the note's history.

This change will present certain scenarios where it appears as though
the revisions are out-of-order because they are showing their
modification times even when those conflict with the version-ordering.
We should consider a way to communicate this, possibly with a tooltip
or a contextual notice when we detect these conflicts.

Testing

  1. Open two copies of the app, called Local and Remote.
  2. Create a note and open it on both devices; set the content to A
  3. Disconnect Local's network access.
  4. On Local add C to make the note AC
  5. With Remote connected, add B to make the note read AB after changing the note on Local.
  6. Reconnect Local and let the changes synchronize.

In develop you should find that the revision slider shows that the
change from Local happened first. In this branch you should see the remote
change happening first. The timestamps in this branch will appear out of order
but the actual note changes will be in order.

history step shown in revision slider develop this branch
1.
2. A A
3. ABC AB
4. AB ABC

This bug appears like a synchronization bug but is actually a display bug.

See #394 where the sorting was introduced

When we make changes to a note we append a `modificationDate` value
which serves as a timestamp of the change _on the client device making
the change_. That is, the timestamp is meant to provide an indicator of
when in real time the change happened, but since it's based on a
device's clock we cannot rely on it or use it for proper sequencing.

It happens from time to time (especially when one device is offline)
that a revision which occurs _after_ another remote change gets a
timestamp showing that it happened _before_ because the local device
recorded the timestamp before the change made it to the Simperium
server.

When this happens the application will show those revisions out of the
actual order they took place and this can lead to confusing note
history. In this patch we're sorting the revisions based on the version
of the note and not the timestamp. This produces the proper sequencing
of changes in the note's history.

This change will present certain scenarios where it appears as though
the revisions are out-of-order because they are showing their
modification times even when those conflict with the version-ordering.
We should consider a way to communicate this, possibly with a tooltip
or a contextual notice when we detect these conflicts.
@dmsnell dmsnell added this to the 1.10 milestone Sep 30, 2019
@dmsnell dmsnell requested a review from a team September 30, 2019 22:55
@dmsnell dmsnell merged commit c1c8327 into develop Oct 1, 2019
@dmsnell dmsnell deleted the fix/sort-revisions-by-version branch October 1, 2019 01:13
@loremattei loremattei modified the milestones: 1.10, 1.9 Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants