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

Navigating up from filter display repositions the camera #297

Open
RobinIsTheBird opened this issue Jan 3, 2017 · 5 comments
Open

Navigating up from filter display repositions the camera #297

RobinIsTheBird opened this issue Jan 3, 2017 · 5 comments
Labels
bug medium

Comments

@RobinIsTheBird
Copy link
Contributor

RobinIsTheBird commented Jan 3, 2017 •
edited

@dboyer, need your input on whether this is desirable behavior, but I find it unexpected and jarring, and thought I had introduced a bug until I discovered it happening in the released android app.

Visit a treemap that includes your current location. The camera pans & zooms to your current location, which is good.

Pan and zoom away from the current location.

Tap the filter button, then tap the OTM logo, AKA the Navigate Up button.

It pans/zooms to your current location again. Seems like it ought to just return to where you were on the map.

The same problem exists when navigating up from tree details.

If you have location turned off, it pans/zooms to the default location and z, even more disconcerting.
@maurizi
Copy link
Contributor

maurizi commented Jan 3, 2017

Definitely a bug in my opinion.

Shouldn't be too hard to fix I think - worst comes to worst, we can record in a member variable of the MainMapFragment whether the camera has been moved yet, though I think we can get something cleaner than that.

@RobinIsTheBird
Copy link
Contributor Author

RobinIsTheBird commented Jan 4, 2017 •
edited

The solution is to override TabLayout#onNewIntent and specify

<activity
    android:name="org.azavea.otm.ui.TabLayout"
    android:launchMode="singleTop" ...

in AndroidManifest.xml.

In this solution, in order for TabLayout to distinguish between a change of treeMap and a Navigate Up from tree details or the filter, the switcher needs to do an intent.setData with the uri for the new InstanceInfo.

TabLayout#onNewIntent(intent) needs to

  • call setIntent with the new intent
  • if intent.getData() yields a uri,
    • do its present reloadInstanceInfo or something similar
    • at the end of the InstanceInfo callback chain, call a method on MainMapFragment similar to its post-PR Refactor for Asynchronous control flow #298 onActivityCreated method.

If intent.getData() returns null, nothing further needs to be done.

References:

RobinIsTheBird added a commit to RobinIsTheBird/otm-android that referenced this issue Jan 4, 2017
@RobinIsTheBird
Refactor for Asynchronous control flow
53b03cf 
Rollbar registered the following error:
```
java.lang.NullPointerException: Attempt to invoke virtual method
'void com.google.android.gms.maps.GoogleMap.moveCamera(
    com.google.android.gms.maps.CameraUpdate)'
on a null object reference

at org.azavea.otm.ui.MainMapFragment.lambda$null$3
    (MainMapFragment.java:488)
```

It's probably a race condition.
Tame the asynchronous control flow to eliminate race conditions.

AFAIK, this fixes all race conditions except open question OpenTreeMap#1 below.
Unknown how many rollbar errors it will eliminate, but should
eliminate the original npe.

Refer to
* [MapView](https://developers.google.com/android/reference/com/google/android/gms/maps/MapView)
* [GoogleMap](https://developers.google.com/android/reference/com/google/android/gms/maps/GoogleMap)
* [MapsInitializer](https://developers.google.com/android/reference/com/google/android/gms/maps/MapsInitializer)
* [GoogleApiAvailability](https://developers.google.com/android/reference/com/google/android/gms/common/GoogleApiAvailability)
* [Fragment Life Cycle](https://developer.android.com/guide/components/fragments.html)

Changes:

- Use jDeferred to synchronize between asynchronous flows
- Move most initialization from onCreateView to `onActivityCreated`
- Move google api connect, map fetch, and add mode cancel to `onStart`
- Update `MapHelper` to use current google availability api

Open questions:

1. Need a way to manage the `MapHelper` google api dialog to determine
   whether the play store services problem gets resolved,
   and fail catastrophically if it doesn't.
2. Need a way to test missing play store services without wrecking my phone.
3. Fails to fix OpenTreeMap#297, clearing the filter repositions the camera
4. Probably shouldn't create new `Deferred` objects if they
   already exist, but haven't noticed any negative effects of doing so
5. I don't know enough about the tile cache to be sure when it needs
   to be cleared.

--

Connects to OpenTreeMap#294
@RobinIsTheBird RobinIsTheBird mentioned this issue Jan 4, 2017
Merged
@RobinIsTheBird RobinIsTheBird self-assigned this Jan 4, 2017
@RobinIsTheBird RobinIsTheBird removed their assignment Jan 4, 2017
@RobinIsTheBird RobinIsTheBird changed the title Clearing the filter repositions the camera Navigating up from filter display repositions the camera Jan 4, 2017
@maurizi
Copy link
Contributor

maurizi commented Jan 4, 2017

As we discussed in person, this can be fixed rather easily by overriding the up-navigation handling in FilterDisplay to go back to the existing TabLayout activity, rather than creating a new activity.

@RobinIsTheBird
Copy link
Contributor Author

RobinIsTheBird commented Jan 5, 2017 •
edited

@maurizi,
singleTop + onNewIntent is the general fix to both problems. It's no bigger deal than overriding up-nav handling in FilterDisplay, but more general.

@RobinIsTheBird RobinIsTheBird mentioned this issue Jan 6, 2017
Open
@RobinIsTheBird
Copy link
Contributor Author

Related to #301 (after switching tree maps, using the back button ... previous tree map). The singleTop + onNewIntent fix proposed here might also fix that issue.

@jwalgran jwalgran added the bug label Jan 19, 2017
@dboyer dboyer added the medium label Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug medium
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jwalgran @dboyer @RobinIsTheBird @maurizi