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

Expose live location sharing labs flag (default: false) and re-enable background location access (PSF-1127) #6324

Merged
merged 4 commits into from
Jun 21, 2022
Merged

Expose live location sharing labs flag (default: false) and re-enable background location access (PSF-1127) #6324

merged 4 commits into from
Jun 21, 2022

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Jun 21, 2022 •
edited
Loading

This makes the labs flag for live location sharing available in the app settings (default value still false) and reverts #6299 which itself reverted background location access. In other words, background location access is enabled now.

I also tweaked some of the comments because we'll keep the build setting even after live location sharing is finished so that forks can easily disable location-related features.

Pull Request Checklist

  • I read the contributing guide
  • UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
  • Accessibility has been taken into account.
  • Pull request is based on the develop branch
  • Pull request contains a changelog file in ./changelog.d
  • You've made a self review of your PR
  • Pull request includes screenshots or videos of UI changes
  • Pull request includes a sign off

@Johennes
Expose live-location sharing labs flag (default: false)
f89631b 
Signed-off-by: Johannes Marbach <johannesm@element.io>
@Johennes
Revert "Merge pull request #6299 from vector-im/johannes/back-out-bac…
b255bcd 
…kground-location-access"

This reverts commit c50f23f, reversing
changes made to ff06c68.
@Johennes Johennes marked this pull request as draft June 21, 2022 07:50
@Johennes Johennes changed the title Expose live location sharing labs flag and enable background location access (PSF-1127) Expose live location sharing labs flag (default: false) and re-enable background location access (PSF-1127) Jun 21, 2022
@Johennes Johennes marked this pull request as ready for review June 21, 2022 07:53
@Johennes Johennes requested review from a team, stefanceriu, gileluard, ismailgulek and aringenbach and removed request for a team, stefanceriu and gileluard June 21, 2022 07:53
aringenbach
aringenbach reviewed Jun 21, 2022
View reviewed changes
Riot/Managers/Settings/RiotSettings.swift
@@ -153,7 +153,7 @@ final class RiotSettings: NSObject {
var enableUISIAutoReporting

/// Indicates if live location sharing is enabled
@UserDefault(key: UserDefaultsKeys.enableLiveLocationSharing, defaultValue: BuildSettings.liveLocationSharingEnabled, storage: defaults)
@UserDefault(key: UserDefaultsKeys.enableLiveLocationSharing, defaultValue: false, storage: defaults)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this should still use a BuildSettings value

Config/BuildSettings.swift Outdated Show resolved Hide resolved
ismailgulek
ismailgulek suggested changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@ismailgulek ismailgulek left a comment

Choose a reason for hiding this comment

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

Some comments inline, otherwise LGTM. And a reminder for the iOS team: we'll probably get a rejection from Apple again when we release this.

Riot/Modules/LocationSharing/UserLocationService.swift Outdated Show resolved Hide resolved
Config/BuildSettings.swift Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jun 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

No Coverage information No Coverage information
No Duplication information No Duplication information

aringenbach
aringenbach reviewed Jun 21, 2022
View reviewed changes
Copy link
Contributor

@aringenbach aringenbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

ismailgulek
ismailgulek approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@ismailgulek ismailgulek left a comment

Choose a reason for hiding this comment

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

LGTM!

@Johennes Johennes merged commit 18ab841 into develop Jun 21, 2022
@Johennes Johennes deleted the johannes/enable-lls branch June 21, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ismailgulek ismailgulek ismailgulek approved these changes

@aringenbach aringenbach aringenbach 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.
3 participants
@Johennes @ismailgulek @aringenbach