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

Show a banner in timeline while location sharing service is running #5660

Merged
merged 4 commits into from
Apr 4, 2022

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Mar 29, 2022

We have already implemented the banner previously. This PR handles the visibility of the banner by observing the location service lifecycle.

@onurays onurays marked this pull request as draft March 29, 2022 12:59
@github-actions
Copy link

github-actions bot commented Mar 29, 2022

Unit Test Results

110 files  ±0  110 suites  ±0   1m 18s ⏱️ +3s
195 tests ±0  195 ✔️ ±0  0 💤 ±0  0 ±0 
650 runs  ±0  650 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 5ec6385. ± Comparison against base commit 539d198.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when registering account, then updates state and emits account created event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled, when registering account, then updates state and emits account created event

♻️ This comment has been updated with latest results.

@onurays onurays marked this pull request as ready for review March 31, 2022 10:49
@onurays onurays requested review from a team and ariskotsomitopoulos and removed request for a team March 31, 2022 10:51
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I added some comments. Keep in mind TimelineFragment is also used for threads, so you should handle that appropriately. You might want to disable that on threads? or an other business request.

@@ -482,6 +482,8 @@ class TimelineFragment @Inject constructor(
RoomDetailViewEvents.StopChatEffects -> handleStopChatEffects()
is RoomDetailViewEvents.DisplayAndAcceptCall -> acceptIncomingCall(it)
RoomDetailViewEvents.RoomReplacementStarted -> handleRoomReplacement()
RoomDetailViewEvents.ShowLocationSharingIndicator -> handleShowLocationSharingIndicator()
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify the code and not to add more lines in TImelineFragment while its already huge, I would have one Event ShowLocationSharingIndicator instead of two:


handleShowLocationSharingIndicator(show: Boolean){
    views.locationLiveStatusIndicator.isVisible = show
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, as we discussed internally. Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos left a comment

Choose a reason for hiding this comment

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

Thanks for the change, please check my comment about threads, while they use the same timeline and maybe will interfere with the location service.

@onurays onurays merged commit ff34ed9 into develop Apr 4, 2022
@onurays onurays deleted the feature/ons/live_location_banner_visibility branch April 4, 2022 09:55
@onurays
Copy link
Contributor Author

onurays commented Apr 4, 2022

Thanks for the change, please check my comment about threads, while they use the same timeline and maybe will interfere with the location service.

Thank you, we will create another PR to support threads.

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.

2 participants