Remove AnnotationUISync service #127
Conversation
| @@ -272,6 +272,17 @@ function annotationExists(state, id) { | |||
| }); | |||
| } | |||
|
|
|||
| function findIDsForTags(state, tags) { | |||
There was a problem hiding this comment.
Couple of things here:
- This needs a doc comment.
findByTagandfindByIDboth have the signature(annotations, XXX)rather than(state, XXX), and this should probably be the same.- This should probably live next to
findByTagandfindByID?
More generally, I'm still concerned that we keep adding O(N^2) lookup functions... this is just going to come and bite us in the future on pages with lots of annotations.
There was a problem hiding this comment.
This should probably live next to findByTag and findByID?
This function takes the global state object for consistency with the other selector functions (in this module and others) which are exported from the module (see the functions just above this one). findByTag and findByID are local helpers.
More generally, I'm still concerned that we keep adding O(N^2) lookup functions...
In this PR, I'm just moving this function here from WidgetController.
Until recently, there was no single unique key for annotations, but now we guarantee that every annotation has a unique local tag as soon as it is added to the store, so we could now replace the annotations array with a tag => Annotation map and then maintain a separate ID => tag map for example.
| @@ -52,8 +53,14 @@ describe('FrameSync', function () { | |||
| }); | |||
|
|
|||
| beforeEach(function () { | |||
| fakeAnnotationUI = fakeStore({annotations: []}); | |||
| fakeAnnotationUI.updateAnchorStatus = sinon.stub(); | |||
| fakeAnnotationUI = fakeStore({annotations: []}, { | |||
There was a problem hiding this comment.
Hmm. Question. Is there any reason we couldn't use the real store here? Surely the whole point of a redux store is that it has no side-effects of its own, so we could just use the real thing and stub out specific methods if we wanted to ensure they were called correctly?
There was a problem hiding this comment.
The core (state, action) => state function has no side effects, but the actual dispatch function does because of middleware which does things like trigger an Angular $digest cycle after each update (and in future, we may also use this as a place to capture metrics, capture errors etc.)
robertknight
force-pushed
the
remove-annotation-ui-sync
branch
from
6a9c9f6 to
59fec2f
Compare
robertknight
force-pushed
the
remove-annotation-ui-sync
branch
from
59fec2f to
e7d88c4
Compare
robertknight
dismissed
nickstenning’s stale review
I've addressed the comment in #127 (comment)
There was a problem hiding this comment.
Selecting a highlight in the document gives me the following console error:
frame-sync.js:121 Uncaught TypeError: annotationUI.findIDsForTags is not a function
I get the same error when I click on one of the bucket indicators in the bucket bar.
Also, hovering on an annotation highlight doesn't seem to do anything.
robertknight
force-pushed
the
remove-annotation-ui-sync
branch
from
e7d88c4 to
54789ba
Compare
As a step towards having all bridge event handlers in one place, move these event handlers into frameSync. * Add tests for findIDsForTags() and for re-exporting of this function from annotationUI
Its functionality has now been moved into the FrameSync service, except
for the logic that was intended to synchronize the "Show Annotation
Highlights" logic across multiple frames, since this is clearly broken
[1].
The two pieces of functionality related to this we need to support at
the moment are:
1. The `showHighlights` config option to set whether highlights are
initially visible.
2. The highlight toggle button in the sidebar's outer frame
Both of these work without the sidebar app's involvement.
[1] See hypothesis/h#3433 and
hypothesis/h#3295
robertknight
force-pushed
the
remove-annotation-ui-sync
branch
from
54789ba to
42d9aba
Compare
|
LGTM. |
This consolidates all of the event handlers for messages coming from connected frames into the sidebar app into the
FrameSyncservice and removes theAnnotationUISyncservice.showAnnotations,toggleAnnotationSelection,sidebarOpenedandfocusAnnotationsmessages have been moved into theFrameSyncserviceAnnotator.Guest) that are not in the same frame as the host? If so I'll need to re-instate this.The functionality related to highlight visibility that does work which we will still need to support is:
Both of these work without the sidebar app's involvement.