/h

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

Implement focusing on a group #2554

Closed
wants to merge 5 commits into
base: master
from

Conversation

Reviewers
No reviews
Assignees

@seanh seanh

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@seanh @nickstenning
@seanh
Contributor

seanh commented Sep 28, 2015

  • Don't break the stream or individual annotation pages
  • Change the shown annotations when the selected group changes
  • Also filter annotations from other groups out from live updates
  • Focus the public group on sign out

seanh added some commits Sep 28, 2015

@seanh
Remove dependencies on threading service 
Have WidgetController, StreamController and AnnotationViewerController
just use annotationMapper only and not use threading directly.

Added threads() and thread(id) to annotationMapper's API to support
this.
8071911
@seanh
Only show annotations from the selected group 
"Focus on a group", i.e. only show annotations from the group selected
in the groups dropdown, in the sidebar.

This commit filters out annotations from other groups from the initial
load of annotations when the sidebar loads. Many more details need to be
taken care of..
Loading status checks…
6fd2533

@seanh seanh added the WIP label Sep 28, 2015

@seanh seanh self-assigned this Sep 28, 2015

@seanh
Focus on a group when group changed in dropdown 
When the currently focused group is changed in the groups dropdown,
change the display (annotations in the sidebar, numbers in the bucket
bar, highlights in the document) to show only the annotations from the
newly-focused group.
Loading status checks…
574547e
h/static/scripts/annotation-mapper.js
function loadAnnotations(annotations) {
var loaded = [];
received = received.concat(annotations);

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

This assumes that the same annotation never gets passed to loadAnnotations() twice, which may not be true. received may need to become a set, or a dict keyed by annotation id, or something,
Copy link
@seanh

seanh Sep 28, 2015

Contributor

This assumes that the same annotation never gets passed to loadAnnotations() twice, which may not be true. received may need to become a set, or a dict keyed by annotation id, or something,
h/static/scripts/annotation-mapper.js
var received = [];
// The annotations that are currently loaded into the page context.
var loaded = [];

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

We could maybe avoid having to keep track of this list here, by just getting it from threading. I sort of feel like it's annotationMapper's job to keep track of this though, and threading is just a dump.
Copy link
@seanh

seanh Sep 28, 2015

Contributor

We could maybe avoid having to keep track of this list here, by just getting it from threading. I sort of feel like it's annotationMapper's job to keep track of this though, and threading is just a dump.
h/static/scripts/annotation-mapper.js
loadAnnotationsFromGroup(annotations, groups.focused().id);
}
function loadAnnotationsFromGroup(annotations, groupId) {

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

Loads the given annotations into the page context, but only the ones from the given group. Docstring needed.
Copy link
@seanh

seanh Sep 28, 2015

Contributor

Loads the given annotations into the page context, but only the ones from the given group. Docstring needed.
h/static/scripts/annotation-mapper.js
annotations.slice().forEach(function(annotation) {
loaded.splice(loaded.indexOf(annotation, 1));
});

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

This crazy dance is because when our $rootScope.$on('groupFocused', ... above calls unloadAnnotations(), the passed annotations array is the loaded array, and you can't modify an array while you iterate over it in JavaScript, so we need to iterate over a copy of it.
Copy link
@seanh

seanh Sep 28, 2015

Contributor

This crazy dance is because when our $rootScope.$on('groupFocused', ... above calls unloadAnnotations(), the passed annotations array is the loaded array, and you can't modify an array while you iterate over it in JavaScript, so we need to iterate over a copy of it.
h/static/scripts/annotation-mapper.js
},
thread: function(id) {
return threading.idTable[id];
}

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

These need docstrings
Copy link
@seanh

seanh Sep 28, 2015

Contributor

These need docstrings
@seanh
Fix tests
Loading status checks…
203e873
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()
Contributor

seanh commented Sep 28, 2015

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()

You have to sign out the sign in then switch back and forth between groups, to get this
Contributor

seanh commented Sep 28, 2015

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()

You have to sign out the sign in then switch back and forth between groups, to get this
@seanh
Fix JavaScript crash
Loading status checks…
f74ae82
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 28, 2015

Contributor

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()

You have to sign out the sign in then switch back and forth between groups, to get this

Fixed this, but there's still a problem that the numbers in the bucket bar get all wrong, if you sign out then sign in again then switch between groups multiple times
Contributor

seanh commented Sep 28, 2015

Getting a TypeError: Cannot read property 'children' of undefined in the console when switching back and forth between groups, from threading.annotationDeleted()

You have to sign out the sign in then switch back and forth between groups, to get this

Fixed this, but there's still a problem that the numbers in the bucket bar get all wrong, if you sign out then sign in again then switch between groups multiple times
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Sep 30, 2015

Contributor

So I've had a quick look at this and I think we're going to get ourselves into all sorts of confusion if we try and maintain all the state in the client.

For now, there's a much simpler (if marginally less efficient) way to do this. Have a look at this branch. It is by no means perfect, and has some of the same issues as this one (we need to forcibly switch to the public group on logout, for example), but requires substantially fewer changes to the code. The only downside is that we're reloading annotations from the server every time we change focus, but I think we can optimize that later.
Contributor

nickstenning commented Sep 30, 2015

So I've had a quick look at this and I think we're going to get ourselves into all sorts of confusion if we try and maintain all the state in the client.

For now, there's a much simpler (if marginally less efficient) way to do this. Have a look at this branch. It is by no means perfect, and has some of the same issues as this one (we need to forcibly switch to the public group on logout, for example), but requires substantially fewer changes to the code. The only downside is that we're reloading annotations from the server every time we change focus, but I think we can optimize that later.
@nickstenning

This comment has been minimized.

Show comment
Hide comment
@nickstenning

nickstenning Sep 30, 2015

Contributor

I'd be happy to pair on this with you at some point tomorrow, @seanh?
Contributor

nickstenning commented Sep 30, 2015

I'd be happy to pair on this with you at some point tomorrow, @seanh?
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 30, 2015

Contributor

@nickstenning I'm free now, but can also pair tomorrow if you want
Contributor

seanh commented Sep 30, 2015

@nickstenning I'm free now, but can also pair tomorrow if you want
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Sep 30, 2015

Contributor

Closing in favour of #2566
Contributor

seanh commented Sep 30, 2015

Closing in favour of #2566

@seanh seanh closed this Sep 30, 2015

@nickstenning nickstenning deleted the remove-dependencies-on-threading branch Feb 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment