Skip to content
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

Add presence support #151

Merged
merged 32 commits into from
May 31, 2020
Merged

Add presence support #151

merged 32 commits into from
May 31, 2020

Conversation

devNan0
Copy link

@devNan0 devNan0 commented May 29, 2020

Yesterday there was the question if nio supports presence/status in the nio matrix room.

So i did a thing :)

Add support for m.presence in sync events. Spec
Add support for PresenceEvent callbacks
Add get_presence and set_presence to async client
Add presence, last_active_ago, currently_active and status_msg fields to MatrixUser

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2020

Codecov Report

Merging #151 into master will increase coverage by 0.03%.
The diff coverage is 91.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   89.52%   89.56%   +0.03%     
==========================================
  Files          41       42       +1     
  Lines        6645     6775     +130     
==========================================
+ Hits         5949     6068     +119     
- Misses        696      707      +11     
Impacted Files Coverage Δ
nio/client/base_client.py 86.47% <75.00%> (-0.58%) ⬇️
nio/client/async_client.py 93.09% <93.10%> (-0.02%) ⬇️
nio/api.py 88.02% <93.33%> (+0.21%) ⬆️
nio/responses.py 86.57% <96.29%> (+0.36%) ⬆️
nio/events/__init__.py 100.00% <100.00%> (ø)
nio/events/presence.py 100.00% <100.00%> (ø)
nio/rooms.py 100.00% <100.00%> (ø)
nio/schemas.py 99.04% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6d6c72...592efc8. Read the comment docs.

Copy link
Collaborator

@mirukana mirukana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!
About setting the presence, you might want to add support for set_presence parameter of the sync API, else it will be reset back to online whenever we sync.
For sync_forever(), on every iteration the loop should check some AsyncClient attribute that gets set when you use set_presence(), so it knows if it should use a different set_presence parameter.

nio/client/async_client.py Outdated Show resolved Hide resolved
nio/client/async_client.py Outdated Show resolved Hide resolved
nio/client/async_client.py Show resolved Hide resolved
@devNan0
Copy link
Author

devNan0 commented May 29, 2020

@mirukana done, but i think i should move this to the base_client, am i right?

PS: Still working on additional tests for the callbacks.

@mirukana
Copy link
Collaborator

mirukana commented May 29, 2020

If you're talking about AsyncClient.presence, it only makes sense in this client's context (for sync_loop). It would also be better as a private attribute (prefixed by an underscore) IMO, users should call set_presence() instead of touching it directly, which can lead to an inconsistent state.

@devNan0
Copy link
Author

devNan0 commented May 29, 2020

So i added callbacks and tests.

@poljar poljar merged commit 592efc8 into matrix-nio:master May 31, 2020
@Half-Shot
Copy link

Regarding 5f2370e, we're looking to drop user_id from the response in Synapse (and eventually Dendrite) in matrix-org/synapse#7606. I assume this won't cause immediate problems for you, but you should look to deprecate the field at some point since it's unspecced.

@poljar
Copy link
Collaborator

poljar commented Jun 1, 2020

It isn't marked as required in the schema and the type is Optional[str] so I think this is fine already.

@Half-Shot
Copy link

Sounds good. Since this is a new feature I shan't stress too hard about breaking everyone's clients.

@devNan0
Copy link
Author

devNan0 commented Jun 1, 2020

I only added the user_id as i found out that synapse returns it.
If it gets removed from synapse i will remove it from the code/schema.

@devNan0
Copy link
Author

devNan0 commented Jun 1, 2020

Changed in #155 now takes the user_id from the original request instead of the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants