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.

Sign up for GitHub

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

Jump to bottom

Feature/aris/presence #4090

Merged
merged 6 commits into from
Oct 13, 2021
Merged

Feature/aris/presence #4090

merged 6 commits into from
Oct 13, 2021

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Sep 27, 2021 •
edited
Loading

Presence Management

User Presence: ["online", "offline", "unavailable"]

  • Presence in DM room list
  • Presence in DM chat
  • Presence in all Rooms member details

Issue #70

Screenshot_20210928_120548
Screenshot_20210928_120653
Screenshot_20210929_011613
Screenshot_20210930_183045
Screenshot_20210930_182404
Screenshot_20210930_182959
Screenshot_20210930_182417
Screenshot_20210930_183007
Screenshot_20210930_182932
Screenshot_20210930_182824

@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as draft September 27, 2021 14:59
@DoM1niC
Copy link

DoM1niC commented Sep 27, 2021 •
edited
Loading

Presence in RoomDetails and in Room would be nice to have :) I like this PR 💯

like Web

bmarty
bmarty reviewed Sep 27, 2021
View reviewed changes
gradle.properties Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 27, 2021 •
edited
Loading

Unit Test Results

  44 files  ±0    44 suites  ±0   45s ⏱️ -4s
  87 tests ±0    87 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
228 runs  ±0  228 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ffa5e1a. ± Comparison against base commit 7338982.

♻️ This comment has been updated with latest results.

ganfra
ganfra requested changes Sep 27, 2021
View reviewed changes
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

Some remarks, otherwise, good job (not tried)

...-android/src/main/java/org/matrix/android/sdk/internal/session/presence/di/PresenceModule.kt Outdated Show resolved Hide resolved
...rc/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt Outdated Show resolved Hide resolved
...trix/android/sdk/internal/session/sync/handler/SyncResponsePostTreatmentAggregatorHandler.kt Outdated Show resolved Hide resolved
...dk-android/src/main/java/org/matrix/android/sdk/internal/database/model/RoomSummaryEntity.kt Outdated Show resolved Hide resolved
matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/RoomSummary.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItem.kt Outdated Show resolved Hide resolved
matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/presence/PresenceService.kt Outdated Show resolved Hide resolved
bmarty
bmarty reviewed Sep 27, 2021
View reviewed changes
.../src/main/java/org/matrix/android/sdk/internal/database/model/presence/UserPresenceEntity.kt Outdated
var lastActiveAgo: Long? = null,
var statusMessage: String? = null,
var isCurrentlyActive: Boolean? = null,
var avatarUrl: String? = null
Copy link
Member

Choose a reason for hiding this comment

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

why not storing also the display name?

bmarty
bmarty requested changes Sep 27, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Good work, I know the PR is still Draft, I've just made first remarks (static review).

bmarty
bmarty reviewed Sep 28, 2021
View reviewed changes
...main/java/org/matrix/android/sdk/internal/session/presence/service/DefaultPresenceService.kt Outdated
setPresenceTask.execute(SetPresenceTask.Params(userId, presence, message))
}

override suspend fun fetchPresence(userId: String) = getPresenceTask.execute(GetPresenceTask.Params(userId))
Copy link
Member

Choose a reason for hiding this comment

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

As said by @ganfra you should add a method to get LiveData of a user presence (or if you want to use Flow, it would be better!), to make it live in the UI.

ganfra
ganfra reviewed Sep 29, 2021
View reviewed changes
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

Some small remaining remarks

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/presence/PresenceService.kt Outdated Show resolved Hide resolved
...roid/src/main/java/org/matrix/android/sdk/internal/session/presence/model/PresenceContent.kt Outdated Show resolved Hide resolved
bmarty
bmarty reviewed Oct 1, 2021
View reviewed changes
...roid/src/main/java/org/matrix/android/sdk/internal/database/model/RoomMemberSummaryEntity.kt
var userPresence: UserPresenceEntity? = null
set(value) {
if (value != field) field = value
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it make sense to test != for Realm object, @BillCarsonFr WDYT?

Copy link
Contributor Author

@ariskotsomitopoulos ariskotsomitopoulos Oct 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I believe it does not make sense, but there are other implementations like this in RoomSummaryEntity:

var latestPreviewableEvent: TimelineEventEntity? = null set(value) { if (value != field) field = value }

var userDrafts: UserDraftsEntity? = null set(value) { if (value != field) field = value }

So if we decide to remove it let's remove it from the other places too, if you agree

...oid/src/main/java/org/matrix/android/sdk/internal/database/mapper/RoomMemberSummaryMapper.kt Outdated Show resolved Hide resolved
...roid/src/main/java/org/matrix/android/sdk/internal/database/query/RoomMemberEntityQueries.kt Outdated
.equalTo(RoomMemberSummaryEntityFields.USER_ID, userId)
.findAll()
.map {
it.userPresence = userPresenceEntity
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to do this? The link between it.userPresence and userPresenceEntity is always the same, no?

Copy link
Contributor Author

@ariskotsomitopoulos ariskotsomitopoulos Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I think you are partially right, we need that only for the first linking, after that it will auto update the changes, so I simply add in the query .isNull(RoomMemberSummaryEntityFields.USER_PRESENCE.$) so now it will only do that for the first linking. Do you think there is a better way for the initial linking with Realm?

...android/src/main/java/org/matrix/android/sdk/internal/session/presence/model/UserPresence.kt Outdated Show resolved Hide resolved
.../main/java/org/matrix/android/sdk/internal/session/room/membership/RoomMemberEventHandler.kt Outdated
roomId,
userId,
roomMember,
// To resolve an edge cases like, on initial sync RoomMemberEventHandler is called after PresenceSyncHandler,
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can change and call PresenceSyncHandler after RoomMemberEventHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I deep dived into the problem about what is happening. I will also add a comment in the codebase explaining that. Let me explain, when we use insertOrUpdate then we have two results:

  1. Insert : A brand new object is created with a primary key etc and is inserted in the DB
  2. Update: A primary key is already found in that table so the framework replace the existing object with the new one.

The problem is when there is an update! With An example will be more clear. Let's say we have the following record:
[ MyPrimaryKey: 123, catName: "Anna", dogName:"John" , .... etc]

and now lets say server has an update with [ MyPrimaryKey: 123, catName: "Ketty"]

So now if we do insertOrUpdate with an object containing only MyPrimaryKey and catName. All the other records will be replaced by null, so after the insertOrUpdate our record will be updated to:
[ MyPrimaryKey: 123, catName: "Anna", dogName:NULL , .... etc]

So to avoid this there are two solutions:

  1. Manually check if the record exists, and update only the fields we want to update
  2. Pick the old value and pass it to new object creation (Faster if the select is with @indexed fields)

While userId and roomId are @indexed, I think the second one is clearer and more efficient. What do you think?

vector/src/main/java/im/vector/app/core/ui/views/PresenceStateImageView.kt Show resolved Hide resolved
vector/src/main/res/layout/fragment_room_detail.xml Outdated Show resolved Hide resolved
@DoM1niC
Copy link

DoM1niC commented Oct 4, 2021

Hey Aris,
can you fix the conflicts plz :) I wanne use it in my fork....

THX for all your Work!!!!

@DoM1niC
Copy link

DoM1niC commented Oct 4, 2021

Circle of offline presence is only transparent instead of filled color

image

@ariskotsomitopoulos
Copy link
Contributor Author

Circle of offline presence is only transparent instead of filled color

image

hmm strange, what android do you have device/version? is the same with light mode too?

@DoM1niC
Copy link

DoM1niC commented Oct 4, 2021 •
edited
Loading

Circle of offline presence is only transparent instead of filled color
image

hmm strange, what android do you have device/version? is the same with light mode too?

Android 11 on Android Studio yes light mode is the same issue since lastest Commits

@DoM1niC
Copy link

DoM1niC commented Oct 4, 2021

image

@DoM1niC
Copy link

DoM1niC commented Oct 4, 2021

Some Users are fine and some not lol 😂

Screenshot_20211004-201051_3DNS_Messenger.png

@ariskotsomitopoulos
Copy link
Contributor Author

Some Users are fine and some not lol 😂

Screenshot_20211004-201051_3DNS_Messenger.png

hmm I think your colors are altered, do you use any custom Theme? Maybe thats why as I can see and other colors are altered

@ariskotsomitopoulos
Copy link
Contributor Author

Circle of offline presence is only transparent instead of filled color

image

Can you check again? It seems like I had to disable it in servers that do not support presence. So now I guess it will work!

@DoM1niC
Copy link

DoM1niC commented Oct 6, 2021 •
edited
Loading

Circle of offline presence is only transparent instead of filled color
image

Can you check again? It seems like I had to disable it in servers that do not support presence. So now I guess it will work!

yeah a good workaround the Presence is now a fully avatar instead of the missing image .. The image appears until a presence status is recieved :)

After the User is switched offline / online the presence image is fine anyway.

@ariskotsomitopoulos
* Implement Presence Service:
9ab59a5 
     - Get Presence Status
     - Set Presence Status
* Integrate presence in room details screen
* Integrate presence in room people's view
* Update UI to support presence
* Fix bug when insertOrUpdate was called on RoomMemberEventHandler and override the correct presence value in RoomMemberSummaryEntity
* Improve performance on updateUserPresence in RoomMemberSummaryEntity entity
* Remarks & linter fixes
* Disable presence when there is no m.presence events. In some servers like matrix.org is disabled atm.
* Enhance UI Presence on DM room lists to support dark/light theme
* Restore missing lines in gradle.properties to speed up debugging
@ariskotsomitopoulos
Add public room indicator to RoomDetailFragment & RoomProfileFragment
e4c3457
@ariskotsomitopoulos ariskotsomitopoulos mentioned this pull request Oct 7, 2021
Closed
@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as ready for review October 8, 2021 08:21
bmarty
bmarty approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, I added 2 commits with some cleanup

Florian14
Florian14 reviewed Oct 13, 2021
View reviewed changes
vector/src/main/res/layout/view_stub_room_member_profile_header.xml
@@ -56,7 +56,7 @@
app:layout_constraintEnd_toStartOf="@+id/memberProfileNameView"
app:layout_constraintHorizontal_chainStyle="packed"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="@+id/memberProfileNameView"/>
app:layout_constraintTop_toTopOf="@+id/memberProfileNameView" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not have @+id for app fields. It is quite enough to have it in the declaration level of the view, even if it is later in the code, it does not change anythink for the compiler and reduce the view references in the code. WDYT @bmarty ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree this is not mandatory to have + for referenced ids, but what would be the main advantage to remove them across the whole codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

This will just reduce the view occurrences when you ctrl + click on a viewId. Probably also optimize the code inspection but not sure this is significant

Copy link
Member

Choose a reason for hiding this comment

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

I see, so this is about dev exp (which is important!). Can you create an issue so we can handle that? Having a lint check would be nice too.

ganfra
ganfra approved these changes Oct 13, 2021
View reviewed changes
Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

LGTM

@bmarty bmarty merged commit 085da6c into develop Oct 13, 2021
@bmarty bmarty deleted the feature/aris/presence branch October 13, 2021 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Florian14 Florian14 Florian14 left review comments

@bmarty bmarty bmarty approved these changes

@ganfra ganfra ganfra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ariskotsomitopoulos @DoM1niC @bmarty @ganfra @Florian14