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

Promote live location labs flag [PSF-959] #6350

Merged
merged 8 commits into from
Jun 29, 2022

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Jun 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

We want to show users the Live Location Sharing option with an informative dialog.

Motivation and context

Screenshots / GIFs

Screenshot
lls_promoting_labs_flag

Tests

  • Disable the live location sharing labs setting if it is enabled
  • In a room, choose the location sharing attachment option from the composer
  • Choose "Share live location"
  • You should see the dialog like the attached screenshot above
  • If the user enables location sharing and clicks the OK button then the flag in labs settings should also be enabled
  • Share a live location

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@onurays onurays requested review from a team, ouchadam and ganfra and removed request for a team June 20, 2022 14:34

<!-- Live Location Sharing Labs Flag Promotional BottomSheet -->
<string name="live_location_labs_promotion_title">Live location sharing</string>
<string name="live_location_labs_promotion_description">Please note: this is a labs feature using a temporary implementation. This means you will not be able to delete your location history, and advanced users will be able to see your location history even after you stop sharing your live location with this room.</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this the final copy? it's a little bit techy~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. I think it is not so hard to understand for users.

private val liveLocationLabsFlagPromotionListener = object : VectorBaseBottomSheetDialogFragment.ResultListener {
override fun onBottomSheetResult(resultCode: Int, data: Any?) {
// Check if the user wants to enable the labs flag
if (resultCode == VectorBaseBottomSheetDialogFragment.ResultListener.RESULT_OK && (data as? Boolean) == true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment above could be replaced by extracting this condition to its own function

private fun hasUserOptedInToLiveLocationLabs(code, data) {
  return resultCode == VectorBaseBottomSheetDialogFragment.ResultListener.RESULT_OK && (data as? Boolean) == true 
}

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, done.

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

tiny refactor comment, not a blocker, LGTM 💯

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.

Small remarks

companion object {
fun newInstance(resultListener: ResultListener): LiveLocationLabsFlagPromotionBottomSheet {
return LiveLocationLabsFlagPromotionBottomSheet().also {
it.resultListener = resultListener
Copy link
Member

Choose a reason for hiding this comment

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

Not a big big deal, but we will loose the listener if the Fragment is recreated by system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Fixed.

@@ -1047,6 +1047,12 @@ class VectorPreferences @Inject constructor(
return defaultPrefs.getBoolean(SETTINGS_LABS_ENABLE_LIVE_LOCATION, false)
}

fun setLiveLocationLabsEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

add parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@onurays onurays requested a review from ganfra June 28, 2022 12:07
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.

See my remark, sorry I should have detailed more in the first place.
Also fix the ktlint!

}
}

private val fragmentLifecycleCallbacks = object : FragmentManager.FragmentLifecycleCallbacks() {
Copy link
Member

Choose a reason for hiding this comment

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

A bit weird to use this for Listener, please use fragment result API instead https://developer.android.com/guide/fragments/communicate#fragment-result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

@onurays onurays requested a review from ganfra June 28, 2022 14:13
@ganfra
Copy link
Member

ganfra commented Jun 28, 2022

Lets merge this if everything is green!

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@onurays onurays merged commit abea685 into develop Jun 29, 2022
@onurays onurays deleted the feature/ons/promote_live_location_labs_flag branch June 29, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants