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

Missing/Cleanup Services #5864

Open
2 of 6 tasks
bmarty opened this issue Apr 28, 2022 · 1 comment
Open
2 of 6 tasks

Missing/Cleanup Services #5864

bmarty opened this issue Apr 28, 2022 · 1 comment
Labels
matrix-sdk T-Task Refactoring, enabling or disabling functionality, other engineering tasks

Comments

@bmarty
Copy link
Member

bmarty commented Apr 28, 2022

Following what has been done in #5858, there is some cleanup to do in our Services API. The SDK should provide a high level and grouped API by functionality. Some new functionalities have been added without really following this rule.

What is missing, at a Session level:

  • A SyncService. Move some methods that are defined directly to the Session. There is a clash name with an existing SyncService :/. Not sure how to handle that, if we can keep 2 things with the same name (not ideal) or rename some.

What is missing, at a Room level:

  • A PollService, to be able to create poll, vote, edit, and close a poll
  • A LocationSharingService, to be able to send a static location, start a live location sharing, send location during a live location sharing, and stop a live location sharing. Today the app has to send state event and location event, this not high level enough
  • An EditionService, to be able to edit any Event. Could also include the redaction of an Event. Also the API editTextMessage and other similar API are taking a TimelineEvent as a parameter, I think it could be better if an eventId is used instead. The SDK can get the TimelineEvent from the eventId if necessary (using TimelineEventDataSource)
  • Clarify ThreadsLocalService and ThreadsService and maybe merge them together and let the SDK do its best depending on the server capability?
  • Other ?

This issue is just a matter of reorganize our API, no new code should be necessary.

Better to handle it when #5858 has been merged.

@bmarty bmarty added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 28, 2022
@bmarty bmarty changed the title Missing Services Missing/Cleanup Services Apr 28, 2022
@ariskotsomitopoulos
Copy link
Contributor

Regarding ThreadsLocalService and ThreadsService. It is quite challenging to make them exist in parallel while we had to persist the old schema ( for old clients ) and also create a new one. At first I tried having a common interface with generics that will return different objects/types but it was not that clear as a solution.

As times goes by ThreadsLocalService should be deprecated while almost all servers will support threads. But I agree definitely it is not ideal. The idea was that ThreadsLocalService would be temporarily, so it would be quite simple to remove it like that. However, it seems that we will never be 100% precent about what the home servers supports, so maybe a refactor there is worth it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matrix-sdk T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

No branches or pull requests

2 participants