-
Notifications
You must be signed in to change notification settings - Fork 195
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
Defer realtime update #106
Conversation
This is one proposed solution for #90 |
18a6342
to
fc24bd8
Compare
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.
This looks really good. Really good when considering the time you've had to do this in.
Two comments:
- Pending update count should (probably?) include deletes. See comment inline.
- Switching groups should discard pending updates, because switching groups will result in annotations being loaded afresh. Not discarding updates when the group changes can result in the following situation:
- receive update U in group G1
- without applying update, switch to group G2
- without applying update, switch to group G1
- visible annotations now include update U, so applying the updates will (at best) have no effect.
} | ||
|
||
function countPendingUpdates() { | ||
return Object.keys(pendingUpdates).length; |
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.
Hmm. This doesn't include pending deletes, which means that deleted annotations will have their reply/share controls disabled with no explanation for the user as to why that is, and no means of removing them from view short of refreshing or waiting for a non-delete update to arrive over the websocket.
Perhaps you've already discussed this, but I reckon pending deletes should be included in this number.
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.
Yes, I agree this is confusing. I've prompted on Slack for some better ideas.
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.
Hmm. Shall we just put a (deleted) badge on these deleted annotation cards when we disable the controls, so that the user knows why the controls have been disabled?
If we want the user to be able to remove deleted annotations without reloading the page, I'd suggest putting an (x) on them that deletes the card, but I don't think we need to support that right now.
There's a couple of problems with deletes being included in the "View n updates" button (these've been discussed on the Trello card):
First issue:
- I am annotating and another user deletes an annotation and I receive the delete as a realtime update
- "View n updates" button appears
- I click it
- The annotation, which may have been scrolled off screen, a reply in a collapsed thread, and/or in a different tab (i.e. it may not be in view), disappears. From my point of view: nothing (that I can see) happened when I clicked the button.
- I scroll and click around trying to find the updates but there is no "updates" (no new content) to find
- I lose trust in the view updates notification which appears to notify about spurious updates
Second issue:
I am typing a reply to an annotation, "View n updates" appears, when I click it the annotation (which was deleted by its author) disappears, presumably taking my reply with it.
We could of course try to fix this by making it only delete annotations that I don't have an unsaved reply to, but that's adding complexity, and it introduces further problems of its own ... Simpler to have one way of handling all deletes that works for both annotations that I'm in the middle of replying to and ones that I'm not.
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.
The first issue you describe could be a bit confusing, but I don't really think it's the end of the world for now.
The second issue describe is present today, only without the benefit of an explicit action the user may understand led to the removal of the annotation to which they were replying.
2b85a0b
to
b00cf4f
Compare
b00cf4f
to
96f9e38
Compare
When the focused group changes, pending updates to already loaded annotations and deletions of already loaded annotations will be cleared in the ANNOTATIONS_UNLOADED event handler. Updates/deletions for annotations not yet loaded were not cleared however. See #106 (review)
To avoid having the contents of the sidebar shift in a distracting way while the user is reading or editing an annotation, do not apply real-time updates received via the WS immediately but instead alert the user to the updates via an icon in the top-bar and apply the updates when the user clicks the button. This commit implements support for caching updates in the streamer, showing an icon and applying updates when the user clicks the icon. Known issues: - The icon does not currently match the Refresh icon in the specs - The tooltip sits above the icon pointing down, instead of pointing up as per the mocks
Implement support for tooltips that point up at the target element and use this facility for the 'Apply Updates' icon in the toolbar.
Prevent the apply updates button from being squashed horizontally and thus wrapping onto multiple lines
Following the approach we have implemented for using <svg> icons on the website without all of the hassle that comes with icon fonts, implement an <svg-icon> component which renders icons as inline SVGs in the client and add the 'refresh' icon for use in the top bar.
Use the <svg-icon> component to render a refresh icon in the top bar and adjust the styling to center the icon and pending update count vertically.
When the focused group changes, pending updates to already loaded annotations and deletions of already loaded annotations will be cleared in the ANNOTATIONS_UNLOADED event handler. Updates/deletions for annotations not yet loaded were not cleared however. See #106 (review)
96f9e38
to
f334327
Compare
Given two annotations A and B and two windows 1 and 2:
-> Your reply is lost. Suggestion: an easy fix for now is to simply not do realtime deletes. By all means disable the actions on the deleted annotation's card (so I can't attempt to reply to a deleted annotation, for example) and show a (deleted) badge or icon on the card, but don't actually delete the card. In the case of an in-progress reply, you will also have to disable the actions on the reply as well so I can't try to post it. See #106 (comment) and also comments on the Trello card. Apologies for the long pause in this GIF while I deleted and edited annotations in the other window: |
@seanh This is an existing issue with realtime, and is not related to this PR. Let's worry about that independently of getting this merged. |
What's an existing issue? The losing of replies? |
Yes. |
I'm happy to see this merged with the losing replies issue still present. |
Ok, I'm still seeing some weirdness with the tooltips, but this is feature-flagged, so I'd rather we get this out for approval in principle, and we can address any little details if we want before pushing it live. (Specifically, the tooltip arrow for the update button appears to point down for me. If I then go and force a |
No objection to merging this, I'm going to continue my UX review and post issues here, not implying that we necessarily need to fix every issue that I find any time soon |
This is a known issue that has already been discussed on the Trello card, but worth recording here: it's possible that the new and updated annotations are not on screen when the user clicks the view updates button. In fact I'd say this is very likely on heavily annotated documents. This results in a button that appears to do nothing. In this case I'm on the annotations tab and a page note gets updated, but there are of course several ways that an annotation can be out of view: |
^ One way in which the above "nothing appears to happen when you click View updates" issue can manifest is when someone updates an annotation in a group other than the group that you're currently on. Should new or updated annotations in other groups trigger the view updates button at all? You can't see them without switching to the other group, and when switching to the other group it will reload them anyway. |
Note about preventing attempts to reply to deleted annotations: It's good for the client to prevent these by disabling the reply button. But it's not in principal possible to prevent all cases this way. Even if we fixed the issues above if the websocket is not working, or if I just haven't received the delete down the websocket yet, I can still attempt to reply to an annotation that the client doesn't yet know has been deleted but that has in fact been deleted from the server. Then I'll get the ugly server error shown above. So we do need to fix the server side behaviour here and send a friendly "Oops thats been deleted" error message. Unfortunately I don't think the server can currently tell the difference between a never-existed annotation ID and the ID of an annotation that was deleted. It shouldn't be too hard to fix though. |
This requires the
defer_realtime_updates
feature flag added in hypothesis/h#3865 to be merged and enabled in the H serviceThis implements the design for deferring application of updates received via the WS from https://trello.com/c/tRZ2H7iH/452-defer-applying-realtime-updates
When the
defer_realtime_updates
flag is enabled, any updates received via the WS are not applied immediately but instead an icon is shown in the top bar which applies the updates when clicked. This is shown both in the sidebar and stream.When a realtime deletion is received, the action icons for the annotation are disabled but the annotation is not actually removed from the loaded set until updates are applied.
Implementation Notes
The set of pending updates and deletions is currently maintained by the
Streamer
service. It should be moved to the app state, but that is currently undergoing a significant refactoring (#104) and also applying updates currently involves accessing the active group, information which is not currently stored in the app state. Putting this state in theStreamer
service for the time being will enable us to ship this quicker and sort out UX.Known Issues