WIP - Fix #871 - Fix Logo Padding in left hand mode

[m-yang]

Fixed issue (#871) by adjusting logo padding when left hand mode is enabled.

The fix required that HomeActivity implement a callback to listen for the the onMapReady call in the BaseMapFragment since the checkLeftHandMode in HomeActivity executed before the map was ready, leading to some issues where padding wasn't set properly when app is first opened.

Let me know what you think.

[CLAassistant]

All committers have signed the CLA.

[barbeau]

@m-yang Now that https://github.com/OneBusAway/onebusaway-android/pull/867 is merged, could you please rebase this branch on the current master branch and force-push to same remote branch to replace the current PR contents? It will be easier for me to review that way.

[barbeau]

@m-yang Regarding the failing CLA check - could you please see https://github.com/cla-assistant/cla-assistant/issues/186, and try following the steps outlined there to see if that fixes it?

[m-yang]

@barbeau Rebased this PR on the current master branch.

Incidentally, the CLA issue seems to have been fixed as well. Before the rebase, there were commits from my old username which may have caused problems with the CLA.

[barbeau]

Thanks for taking a shot at this! Nice CLA is fixed too.

The left margin will need to be set based on screen width which varies per device. For example, here's how it looks on a Samsung Galaxy S8+:

Note that this margin also affects the compass position at the top of the screen when you rotate the map:

I think this is ok for left hand mode as long as it has the correct margin based on device screen width too.

[barbeau]

The fix required that HomeActivity implement a callback to listen for the the onMapReady call in the BaseMapFragment since the checkLeftHandMode in HomeActivity executed before the map was ready, leading to some issues where padding wasn't set properly when app is first opened.

I think a less complex solution that avoids additional interfaces and callbacks is to check the left hand preference when instantiating the BaseMapFragment, and if true pass the left map margin into the BaseMapFragment via an Intent when it's instantiated. You can then read this Bundle via Bundle args = getArguments(); in BaseMapFragment.onCreateView() and save the left margin value to the Bundle mLastSavedInstanceState via mLastSavedInstanceState.putInt(MapParams.MAP_PADDING_LEFT, mMapPaddingLeft);. The fragment should automatically pick this up then in initMapState() when it pulls the left margin from the mLastSavedInstanceState. Note that you should only write the left margin to mLastSavedInstanceState if the Bundle exists (i.e., getArguments() isn't null) and the leftMargin key/value pair exists in the Bundle. Right now the MAP_PADDING parameters are persisted to the bundle just to save the state when rotating the phone (or after being destroyed and recreated by the Android system with a previous app state) - by implementing the above you'd be injecting an initial value for the left margin when BaseMapFragment is created.

See the use of ArrivalsListFragment from HomeActivity for an example of passing values from an Activity to a Fragment via Intents, and let me know if you have any questions.

Also, the HomeActivity.checkLeftHandMode() method should probably be eliminated, with the preference check for left hand mode being moved directly into setFABLocation().

[m-yang]

@barbeau

Thanks for your response.

You can then read this Bundle via Bundle args = getArguments();

... save the left margin value to the Bundle mLastSavedInstanceState via mLastSavedInstanceState.putInt(MapParams.MAP_PADDING_LEFT, mMapPaddingLeft);

Just want to clarify an issue about Bundles in the BaseMapFragment.

In BaseMapFragment's onCreateView method, I am working with two bundles.

I am reading the left padding passed in from HomeActivity through Bundle bundle = getArguments(). Then, I'm calling mLastSavedInstanceState.putInt(MapParams.MAP_PADDING_LEFT, mMapPaddingLeft);

However, I'm getting a null reference error when calling mLastSavedInstanceState.putInt(MapParams.MAP_PADDING_LEFT, mMapPaddingLeft);

If I do a null on mLastSavedInstanceState prior to the putInt() call, the left hand padding is not saved in the mLastSavedInstanceState that will be read by initMapState().

What are some suggestions as to debugging this issue?

[barbeau]

@m-yang Sorry for the delay getting back to you, I've been swamped lately and got buried in some other tasks. Hopefully I'll have a chance to look at this soon.

[barbeau]

@m-yang mLastSavedInstanceState will be null if there isn't a previously saved map state (this is a feature of the Android framework). In that case, you would need to instantiate a new Bundle and assign it to mLastSavedInstanceState in onCreateView() within if (MapHelpV2.isMapsInstalled(getActivity())) {.

Thinking more and looking at initMap(), though, I'd prefer to separate this particular use case from the case when restoring from a previous state. This new solution would be different in that instead of instantiating a new Bundle when mLastSavedInstanceState is null you'd save the left margin in a separate class member variable and leave mLastSavedInstanceState null. Then, you'd check this variable in the else clause within initMap():

        if (savedInstanceState != null) {
            initMapState(savedInstanceState);
        } else {
            // here

...and if the padding is saved you'd set the padding value there in the args bundle after it's instantiated but before initMapState() is called:

            Bundle args = getActivity().getIntent().getExtras();
            // The rest of this code assumes a bundle exists, even if it's empty
            if (args == null) {
                args = new Bundle();
            }
            // here

That's similar to how we're handling if the map center has been initialized by checking if values have been set for the MapParams.CENTER_LON, etc.

initMap() is a bit tricky as it's actually handling at least 3 different cases - fragment is newly instantiated with no state (savedInstanceState is null), it's setting the map position if passed in from another Fragment (MapParams.CENTER_LAT is set in Intent extras), and trying to set the map position if that's previously been saved in onPause() (via PreferenceUtils.maybeRestoreMapViewToBundle(args);). To be honest it's been a while since I've worked in these methods so I may be missing a case here where we might break something, so it will need to be well tested.

Let me know if you have any questions about this. Also please push any code you have so far if you have more questions (even if it doesn't work) - it's easier for me to comment that way and if needed pull it in myself and run.