-
Notifications
You must be signed in to change notification settings - Fork 39
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
Optimize notify_user to use get_publisher #80
Comments
Note to myself, how to test block/unblock manually in the console.
|
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
Forget I said that. I misunderstood a little what the code was doing. The |
For completeness naf-janus-adapter is setting data and notifications to true for the publisher here: https://github.com/networked-aframe/naf-janus-adapter/blob/e0842fabdfa87e3a2291031f1f45643a208f7d23/src/index.js#L557-L558 |
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
… publishers in the room to send the blocked/unblocked event (closes mozilla#80)
process_block
andprocess_unblock
are usingnotify_user(&event, &whom, switchboard.publishers_occupying(&joined.room_id));
so iterating over publishers in the rooms to find the session for the user id
whom
havingnotifications:true
.Looking at it for #55 when using multiple rooms, we should iterate in this case over all rooms and all sessions of those rooms to just find the session.
With an old version of the code, iterating over sessions of the room was better than iterating over all the sessions (this was what
get_publisher
was doing).But we now have an optimized switchboard and
get_publisher(user_id)
is just O(1).I think we can rewrite
process_block
andprocess_unblock
andnotify_user
to useget_publisher
here.Note that previously we didn't use the same semantic for publisher,
notify_user
is searching for a session withnotifications:true
and inprocess_join
we check fordata:true
.I currently don't have the block/unblock feature in my app, so it may be a little bit difficult for me to test any changes, although I could test it in Chrome console and send the request manually.
I may try doing a PR myself in a month for this if I'm going to work on #55.
The text was updated successfully, but these errors were encountered: