#hypothes.is

<robertknight> https://github.com/hypothesis/h/pull/2550 is ready for review. Let me know if there are any other PRs there that I can help out with. Otherwise I'll move on to https://trello.com/c/ZIWFU0i9/93-improve-groups...

<seanh> nick: What have you done for focus-on-a-group so far? Looking for the relevant PRs that I should know about

<nick> basically nothing other than planning and cleaning up

<nick> translating the annotation mapper service to JS

<nick> my plan, briefly:

<nick> - make CRUD interactions from the client go through the annotation mapper service

<nick> (i.e. nothing other than the annotation mapper service interacts with the threading service)

<nick> - at that point hopefully everything would be mediated by annotationmapper, and changing focus would be as simple as `unload()` and then load for the focused group.

<nick> i have a WIP branch, actually

<nick> one second

<seanh> Ok, I'll take a look

<nick> i haven't got very far!

<seanh> No problem

<seanh> This will take me a while 'cos I'll have to (re-)learn my ways around the relevant bits of frontend, but I'll get on with it. I'll break it into steps. Step 1: annotations from the initial request for annotations from the server should go through annotation-mapper and get filtered by group

hypothesis/h#8210 (focus-on-a-group - 62d698b : Nick Stenning): The build passed.

Change view : https://github.com/hypothesis/h/compare/8294a60...

Build details : https://travis-ci.org/hypothesis/h/builds/82551185

<nick> @seanh: if I were you I'd focus on removing other components dependency on the threading service first

<seanh> By the way I've done most of the work on removing string limits from the db - I'll finish that off once this is done

<nick> and only then worry about the best way to do focus on a group

<nick> and feel free to give annotationMapper's methods better names

<nick> `load` for `loadAnnotations` is probably clearer :)

<nick> and thank you for the db stuff

<nick> I'll review that when elasticsearch isn't on fire any more

<seanh> Yuck, messy

<lenazun> :fire::fire: :fire_engine:

hypothesis/h#8211 (remove-dependencies-on-threading - 8071911 : Sean Hammond): The build passed.

Change view : https://github.com/hypothesis/h/commit/80719111...

Build details : https://travis-ci.org/hypothesis/h/builds/82565637

<nick> @lenazun: up and to the right -- that's good, isn't it? https://mon.hypothes.is/grafana/dashboard/db/el...

<lenazun> caching all the things

hypothesis/h#8212 (remove-dependencies-on-threading - 6fd2533 : Sean Hammond): The build was broken.

Change view : https://github.com/hypothesis/h/compare/8071911...

Build details : https://travis-ci.org/hypothesis/h/builds/82570541

<seanh> So annotationMapper needs to inject the groups service, so that it can ask what group is currently focused (this is needed for the initial load of annotations, for example)

<seanh> But the groups service also needs to inject annotationMapper, so that it can call it whenever the focused group changes - circular dependency

<seanh> I suppose this could be worked around by having the groups service do a $rootScope.$emit() when the focused group changes. A $on() function in annotationMapper currently can't access local variables like `loaded` that are normally available to annotationMapper's functions via closure

<seanh> Another solution might be to merge annotationMapper and groups into one thing

<nick> @seanh: that doesn't make a lot of sense

<nick> if the mapper is asking groups for the focused group, why would groups also need to tell the mapper?

<seanh> There are two cases

<seanh> 1. The initial load of annotations, 2. When the selected group in the dropdown changes

<seanh> I've actually just got it working, but I should probably look into refactoring 1 to just be another case of 2

<seanh> Yeah, I need to factor this better, but the good news is that I don't think focus-on-a-group is going to be very difficult

<nick> So typically in angular land you would solve this problem using the scope

<nick> so the groupList directive could take a callback parameter to execute when a group is clicked on

<nick> I don't think the groups service should need to know anything about the annotation mapper

<nick> it's just a container object for the singleton state of the "focused group"

<nick> I suppose you could put the callback there...

<nick> ...anyway, there are options :)

<seanh> The way that I have working right now is that the groups service just emits a "groupFocused" event

<nick> Ok, great. I'd like to get away from using events for control as far as possible, but I'm certainly happy to take a look at a PR so I can talk about the concrete rather than the abstract!

<seanh> Yeah, okay. I'll try it with events first and then we can talk, I agree they're bad

<seanh> nick: So this is pretty ugly: https://github.com/hypothesis/h/pull/2554/files but it works

<seanh> The ugliness mostly comes from annotationMapper now keeping track of two lists of annotations: all the ones it has received, and the ones it's currently showing

<nick> Cool. I'll take a look tomorrow.

<nick> I'm sure we can clean it up a bit.

hypothesis/h#8214 (remove-dependencies-on-threading - df00c89 : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/6fd2533...

Build details : https://travis-ci.org/hypothesis/h/builds/82590899