Don't enter compass mode before location is found

[smichel17]

Fixes #3147

This is a draft because I'm going to add a few more commits, and I may want to refactor after that, to avoid making the same fix in many places.

[smichel17]

@westnordost The MainFragment appears to be getting location updates in two places.

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/MainFragment.kt

Lines 278 to 282 in d6f31b5

	/* ---------------------------- LocationAwareMapFragment.Listener --------------------------- */
	override fun onLocationDidChange() {
	updateLocationPointerPin()
	}

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/MainFragment.kt

Lines 568 to 573 in d6f31b5

	private fun onLocationChanged(location: Location) {
	val isFollowingPosition = mapFragment?.isFollowingPosition ?: false
	gpsTrackingButton?.visibility = if (isFollowingPosition) View.INVISIBLE else View.VISIBLE
	gpsTrackingButton?.state = LocationState.UPDATING
	updateLocationPointerPin()
	}

Are both of these needed? If not, which would you prefer to be removed?

If it's the same to you, I am slightly leaning towards receiving location updates in the MainFragment and passing them down into the MapFragment. This way, the same flow can be adapted to handle location availability updates— the MapFragment currently does not receive them, and this is why the location pointer still displays after gps is turned off: in MainFragment::updateLocationPointerPin, it only checks the MapFragment's location, which is never reset to null when location services are disabled.

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/MainFragment.kt

Lines 703 to 707 in d6f31b5

	val location = mapFragment.displayedLocation
	if (location == null) {
	locationPointerPin.visibility = View.GONE
	return
	}

[westnordost]

Both are needed, because both do different things. Maybe the naming is off:

onLocationDidChange is called after the map fragment updated its displayed location (scroll around the map)

onLocationChanged is called when the user's location changed (GPS location)

[westnordost]

Anyway, I'll merge this as it is, the change you did so far fixes the mentioned bug.

[smichel17]

Well, ok. I was going to get back to this, but I can continue in another PR :)

Both are needed, because both do different things.

[written several days ago] Okay, I see: the LocationAwareMapFragment only receives location updates (and thus, calls back to the MainFragment) when it is tracking the user's location, and the MainFragment only receives a single update, when location is first enabled.

onLocationDidChange is called after the map fragment updated its displayed location (scroll around the map)

This is not exactly true, as far as I can tell. In summary:

• The MainFragment triggers the MapFragment to begin location tracking any time location services are enabled.
• The MapFragment does not stop location tracking except when location services are turned off or when the fragment is closed (in onStop()).
• The location update is always passed back up to the MainFragment, regardless of whether the map location changed (edit: in other words, even it is still displaying the same bbox on screen).

LocationAwareMapFragment subscribes to location changes here (and nowhere else):

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/LocationAwareMapFragment.kt

Lines 106 to 109 in 0ca3031

	fun startPositionTracking() {
	locationMapComponent?.isVisible = true
	locationManager.requestUpdates(2000, 5f)
	}

Which in turn is called only here:

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/MainFragment.kt

Lines 542 to 549 in 0ca3031

	private fun onLocationIsEnabled() {
	gpsTrackingButton.visibility = View.VISIBLE
	gpsTrackingButton.state = LocationState.SEARCHING
	mapFragment!!.startPositionTracking()
	setIsFollowingPosition(wasFollowingPosition)
	locationManager.requestSingleUpdate()
	}

And handles them like this:

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/LocationAwareMapFragment.kt

Lines 136 to 142 in 0ca3031

	private fun onLocationChanged(location: Location) {
	this.displayedLocation = location
	locationMapComponent?.location = location
	compass.setLocation(location)
	centerCurrentPositionIfFollowing()
	listener?.onLocationDidChange()
	}

…I'd call this, "The MainFragment subscribing to location updates, with extra steps." 😄

It makes me feel like the abstraction boundary is wrong, somewhere.

I understand the relationship between the MainFragment and GPS button: the button is a dumb view*, which the fragment updates sometimes. I don't really understand the relationship with the map fragment. It's a 3-class hierarchy, seemingly just to split the logic up in to manageable/logical chunks. It definitely is the source of truth for map state. But the responsibilities for managing state updates and the "business logic" are split up between the different classes and the MainFragment in confusing ways.

Actually, the abstraction boundaries are not that confusing. It seems like MainFragment is intended to have the high level logic, for coordinating the various components. Then the fragments hold the state and implementation details. It's just that a few corner cases, like shouldCenterCurrentPosition(), blur the line of what's an implementation detail.

*Actually, there's one place where the fragment uses the state of the view as part of control flow… perhaps we should change this.

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/map/MainFragment.kt

Lines 625 to 628 in 0ca3031

	private fun onClickTrackingButton() {
	val mapFragment = mapFragment ?: return
	if (gpsTrackingButton.state.isEnabled) {
	setIsFollowingPosition(!mapFragment.isFollowingPosition)

I'm not totally confident in my analysis of the abstraction boundaries, and I'm less certain what I would like to change. So for now I will change nothing, and proceed with my original plan: finish the bugfixes "locally" however I need to, then check if there is anything begging to be refactored :)

---

Finally, completely unrelated: I've noticed in several places we do something like
displayedLocation.let { LatLon(it.latitude, it.longitude) }.

Would you accept a PR adding an extension function, Location.toLatLon()?
If so, which file would be the right file to add that? LocationUtil.java (with converting to kotlin), a new file, or somewhere else?

[westnordost]

Would you accept a PR adding an extension function, Location.toLatLon()?
If so, which file would be the right file to add that? LocationUtil.java (with converting to kotlin), a new file, or somewhere else?

Yes, yes

[matkoniecz]

@smichel17 Thanks for this fix! I got bitten by it and failed to track down or notice what is causing problems.

[smichel17]

@westnordost Before you release v34, there's one more quick bugfix I can get in…

[smichel17]

#3213

Also, I think the changelog gets this backwards.

Fix tapping compass rotates map back to North even when currently searching for GPS location

The conditions are right, but the undesired behavior was entering compass mode, not rotating back to north. I would suggest phrasing, but I'm not sure how you want to refer to compass mode.