Fix #630 - Add map zoom control with checkbox preference in settings #867

m-yang · 2018-04-04T04:45:50Z

Work in progress PR for zoom control (#630).

CLAassistant · 2018-04-04T04:45:57Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ m-yang
❌ matty-23
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

m-yang · 2018-04-05T04:29:17Z

Getting a 503 error code during the CI build. Since I didn't change the gradle files, the error is probably external.

Since I'm new to this development environment, how important is it for the CI build to succeed for each commit?

barbeau · 2018-04-05T15:10:31Z

@matty-23 Looks like there was an error on the CI build downloading an artifact from Google's server, so definitely something outside of our control. I just restarted the build so hopefully that fixes it.

It's not critical that the CI build succeeds on each commit, but if it doesn't build I'd definitely take a look and see what the error is, and if it seems to be related to your changes (e.g., you forgot to commit a new file) then fix it.

m-yang · 2018-04-11T05:21:50Z

Made a pair of zoom in/out buttons to replace the default Google Maps buttons. Top image shows the unpressed state and the bottom image shows the pressed state.

Next steps are to setup the actual zoom functionality and make it responsive to the left handed mode.

Please let me know of any feedback before I move on. Thanks!

barbeau · 2018-04-11T13:58:37Z

@matty-23 Thanks for your work on this, it's looking good!

A few comments - here is the default zoom control on left and the custom one in this PR for reference (unpressed state):

I'd increase the width of the plus and minus by 1dp or so to more closely match the default controls
I'd change the color of plus and minus to be material design gray instead of theme green
The custom control seems more transparent, or less "white", than the default. I'd try slightly decreasing the transparency or increasing the "whiteness" to see if you can match the default control more closely
The default control has rounded corners - you can add <corners android:radius="6dp"/> in the <shape> to implement this
The default control has a slight drop shadow. You can try adding elevation and padding with an android:background of a solid color to see if that looks close.

Here's the default zoom control on left and custom on right in the pressed state:

Comments:

The pressed state looks a little too transparent, or not dark enough, compared to the default.

These are nitty-gritty details - you've mostly nailed the overall look.

m-yang · 2018-04-14T03:38:01Z

Thanks for the feedback!

As of now,

Buttons have been restyled.
Buttons respect left hand mode .
Zoom functionality works.

Just wanted to mention a couple points about what I've worked on.

It turns out that the material gray color is a bit lighter than what the default zoom control has.
I had some difficulties in perfecting the drop shadows. There's a small difference between what I have and what the default zoom control's drop shadow looks like and I'm not sure what steps to take to make it look more similar.

Let me know what you think of the changes! 😃

barbeau · 2018-04-17T15:53:37Z

onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java

+            layoutParams.setMarginStart(marginDp);
+            layoutParams.setMargins(marginDp, 0, 0, 0);
+
+        } else /*not left hand mode*/ {


You can eliminate the comment here, I think it's clear enough given the boolean name

barbeau · 2018-04-17T15:56:02Z

onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java

+
+        int marginDp = (int) getResources().getDimension(R.dimen.zoom_btn_layout_margin);
+
+        if(leftHandMode) {


If there a reason we shouldn't move this IF statement to the checkLeftHandMode() method so it's consistent with the FABs?

Also, here and elsewhere please use a space between the if and ( - should be if (

barbeau · 2018-04-17T15:56:45Z

onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java

-        mMapFragment.displayZoomControl(displayZoom);
+        LinearLayout zoomButtonsLayout = (LinearLayout) findViewById(R.id.zoom_buttons_layout);
+
+        if(displayZoom) {


should be if (

barbeau · 2018-04-17T16:05:23Z

@matty-23 Looks very nice! You're drop shadow is close enough, I wouldn't worry about any further changes for that. A few comments inline specific to the code.

One last design comment - the placement of the zoom control looks odd to me up against the side of the My Location (and Layers in the Tampa region) FAB(s). I understand that for usability you've grouped it next to the FAB (and it can't be above the FAB due to the Layers FAB expanding upward), but I'm leaning towards putting it on the side of the screen opposite of the My Location FAB. So, for right-hand mode it would be on the left side, and for left hand mode it would be on the right side. This would be closer to the default controls that appear on their own in a corner of the screen. Any thoughts on this?

m-yang · 2018-04-17T22:27:34Z

@barbeau
Thanks for your feedback.

I went ahead and reformatted the code with the AndroidStyle template and factored code for placing the the location FAB and zoom controls in the checkLeftHandMode() method.

So, for right-hand mode it would be on the left side, and for left hand mode it would be on the right side.

To make sure I'm understanding this correctly, the zoom controls would be on the opposite corner of the My Location FAB?

I do agree that with the zoom controls next to the Location and Layers FAB, the UI does seem cluttered. However, depending on the size of the device's screen, it may be difficult to physically reach zoom controls if they were on opposite corners. In my opinion, the zoom controls and FAB should be grouped together so they can be easily reached.

barbeau · 2018-04-17T23:09:44Z

the zoom controls would be on the opposite corner of the My Location FAB?

That's what I was proposing, but I agree on some wide devices that could cause usability issues for some users.

What if we rotate the control 90 degrees clockwise and center it horizontally at the bottom of the screen? This is actually where the Maps API v1 default zoom controls were - plus sign on right and minus on left. This way we wouldn't need to shift control for lefthand mode either.

barbeau · 2018-04-17T23:12:57Z

Example screenshot from Maps API v1 - https://forums.crackberry.com/attachments/bb10-android-app-sideloading-f279/211882d1382231529t-how-fix-any-android-app-dependent-google-maps-api-make-maps-working-2eobi3l.png

m-yang · 2018-04-19T01:58:51Z

Zoom buttons moved to the bottom of the screen. I think this layout looks better than previous layout and is more functional since there less chance of accidentally pressing the FAB when zooming, or vice versa.

What are your thoughts?

On a side note: I recently changed my github username and there's some issues with the CLA since the previous commits were tied to my old username. How can I address this?

barbeau · 2018-04-19T20:07:01Z

@m-yang I like it! I would swap the dimensions of the sides and top/bottom of the buttons, though.

So instead of this:

...dimensions would look like:

In other words, the height of each button should be less than the width. Right now it looks like two vertical buttons stacked side-by-side, instead of two horizontal buttons side-by-side. So, with the changes it would be a true 90 degree clockwise turn of the original vertical control.

Don't worry about the CLA, I'm not sure if there is a way to resolve it with your old name. As long as the change of names is documented in this PR you should be fine.

m-yang · 2018-04-20T01:19:04Z

Resized the zoom buttons so they are wider.

Might just be me, but the vertical "|" of the "+" sign gives the illusion that the button is less wide than the "-" button. I adjusted it so that there is slightly more left and right padding for the "+" button.

Let me know what you think.

barbeau · 2018-04-20T13:57:07Z

onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java

+
+        setFABLocation(leftHandMode);
+
+        //setZoomControlLocation(leftHandMode);


This can be removed now.

barbeau · 2018-04-20T13:57:14Z

onebusaway-android/src/main/java/org/onebusaway/android/ui/HomeActivity.java

@@ -1605,6 +1663,45 @@ private void checkLeftHandMode() {
        }
    }

+    private void setZoomControlLocation(boolean leftHandMode) {


This method can be removed now

barbeau · 2018-04-20T13:58:07Z

onebusaway-android/src/main/res/drawable/zoom_btn_drop_shadow_left.xml

+
+
+<layer-list xmlns:android="http://schemas.android.com/apk/res/android">
+


Could you please remove one of the new lines here (and elsewhere) so there is only one line in between elements?

barbeau · 2018-04-20T13:58:50Z

onebusaway-android/src/main/res/layout/main.xml

+                        android:layout_alignBottom="@+id/btnMyLocation">
+
+                    <ImageButton
+


Could you please remove the new line here for consistency?

barbeau · 2018-04-20T13:58:55Z

onebusaway-android/src/main/res/layout/main.xml

+                            android:src="@drawable/zoom_out_button"/>
+
+                    <ImageButton
+


Could you please remove the new line here for consistency?

barbeau · 2018-04-20T13:59:43Z

onebusaway-android/src/main/res/values/dimens.xml

+    <dimen name="zoom_out_btn_right_offset">@dimen/zoom_out_btn_left_offset</dimen>
+    <dimen name="zoom_in_btn_left_offset">4.5dp</dimen>
+    <dimen name="zoom_in_btn_right_offset">@dimen/zoom_in_btn_left_offset</dimen>
+


Could you please remove the extra new line here for consistency?

barbeau · 2018-04-20T14:00:06Z

onebusaway-android/src/main/res/values/strings.xml

+    <string name="preferences_show_zoom_controls_title">Show zoom controls</string>
+    <string name="preferences_show_zoom_controls_summary">Display zoom controls on map
+    </string>
+


Could you please remove the extra new lines here for consistency (just one line break)?

barbeau · 2018-04-20T14:01:30Z

@m-yang This looks good to me! Thanks for all your work on this. There is just some cleanup remaining that I commented on inline (remove unused left-hand mode for zoom buttons, remove extra newlines), and then we can get this merged.

m-yang · 2018-04-20T14:31:10Z

@barbeau

Thanks for the feedback!

I want to point out an issue. With left hand mode enabled, the My Location FAB partly covers the Google logo in the bottom corner of the screen. We would want to move the logo so that it's entirely visible to comply with the API's terms of service.

One option is to move the logo to the bottom right corner of the screen in left hand mode and have it remain in its bottom left corner in right hand mode. Another option we have is to move the FAB up a bit.

What are your thoughts?

barbeau · 2018-04-20T14:53:32Z

I want to point out an issue. With left hand mode enabled, the My Location FAB partly covers the Google logo in the bottom corner of the screen. We would want to move the logo so that it's entirely visible to comply with the API's terms of service.

That's a good point - I didn't catch that in the original implementation.

One option is to move the logo to the bottom right corner of the screen in left hand mode and have it remain in its bottom left corner in right hand mode.

Yes, I think this is the best solution. Could you please open a new issue for this? We can get this PR merged and tackle that in a separate workstream.

m-yang · 2018-04-20T20:39:25Z

@barbeau

Styled the code! Let me know your feedback.

barbeau · 2018-04-23T21:16:49Z

Looks great! Thanks for your work on this @m-yang! It will go out in the next release (not v2.3.5, which is already rolling out now).

* Based on commit 7aa3783 - Fix OneBusAway#630 - Add map zoom control with checkbox preference in settings (OneBusAway#867)

Add zoom control checkbox preference in settings

05690fc

barbeau added the work-in-progress label Apr 4, 2018

barbeau changed the title ~~Add zoom control checkbox preference in settings~~ WIP - Fix ##630 - Add zoom control checkbox preference in settings Apr 4, 2018

barbeau changed the title ~~WIP - Fix ##630 - Add zoom control checkbox preference in settings~~ WIP - Fix #630 - Add zoom control checkbox preference in settings Apr 4, 2018

Add ability to show/hide zoom controls on map

ccef245

Matthew Yang added 2 commits April 10, 2018 21:57

create custom zoom in/out buttons

cafe19b

adjust pressed color state for zoom out button

89056b2

Matthew Yang added 2 commits April 13, 2018 20:03

restyle zoom buttons and add left hand mode

aa7e6d2

implement map zooming for zoom control buttons

c23a784

barbeau reviewed Apr 17, 2018

View reviewed changes

refactor and format code

8fba983

place zoom in the bottom center

8f97f3f

resize zoom buttons

d0497f6

barbeau reviewed Apr 20, 2018

View reviewed changes

barbeau mentioned this pull request Apr 20, 2018

Location FAB covers Google logo in left hand mode #871

Closed

fix code style

c07f334

barbeau added enhancement and removed work-in-progress labels Apr 23, 2018

barbeau changed the title ~~WIP - Fix #630 - Add zoom control checkbox preference in settings~~ Fix #630 - Add map zoom control with checkbox preference in settings Apr 23, 2018

barbeau merged commit 7aa3783 into OneBusAway:master Apr 23, 2018

barbeau mentioned this pull request Apr 23, 2018

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

Closed

m-yang deleted the AddZoomControl branch April 25, 2018 08:34

barbeau mentioned this pull request Apr 30, 2018

Refactor OTP to have new OTP classes to replace the OBA GTFS classes opentripplanner/OpenTripPlanner#2495

Merged

themonki added a commit to themonki/onebusaway-android that referenced this pull request Jun 5, 2018

Update Spanish Translation

8dd77a5

* Based on commit 7aa3783 - Fix OneBusAway#630 - Add map zoom control with checkbox preference in settings (OneBusAway#867)

themonki mentioned this pull request Jun 5, 2018

Update Spanish Translation #885

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #630 - Add map zoom control with checkbox preference in settings #867

Fix #630 - Add map zoom control with checkbox preference in settings #867

m-yang commented Apr 4, 2018 •

edited by barbeau

Loading

CLAassistant commented Apr 4, 2018 •

edited

Loading

m-yang commented Apr 5, 2018

barbeau commented Apr 5, 2018

m-yang commented Apr 11, 2018

barbeau commented Apr 11, 2018 •

edited

Loading

m-yang commented Apr 14, 2018

barbeau Apr 17, 2018

barbeau Apr 17, 2018

barbeau Apr 17, 2018

barbeau Apr 17, 2018

barbeau commented Apr 17, 2018

m-yang commented Apr 17, 2018 •

edited

Loading

barbeau commented Apr 17, 2018 •

edited

Loading

barbeau commented Apr 17, 2018

m-yang commented Apr 19, 2018 •

edited

Loading

barbeau commented Apr 19, 2018

m-yang commented Apr 20, 2018 •

edited

Loading

barbeau Apr 20, 2018

barbeau Apr 20, 2018 •

edited

Loading

barbeau Apr 20, 2018

barbeau Apr 20, 2018

barbeau Apr 20, 2018

barbeau Apr 20, 2018

barbeau Apr 20, 2018

barbeau commented Apr 20, 2018

m-yang commented Apr 20, 2018 •

edited

Loading

barbeau commented Apr 20, 2018

m-yang commented Apr 20, 2018

barbeau commented Apr 23, 2018


		int marginDp = (int) getResources().getDimension(R.dimen.zoom_btn_layout_margin);

		if(leftHandMode) {


		setFABLocation(leftHandMode);

		//setZoomControlLocation(leftHandMode);



		<layer-list xmlns:android="http://schemas.android.com/apk/res/android">

		android:layout_alignBottom="@+id/btnMyLocation">

		<ImageButton

Fix #630 - Add map zoom control with checkbox preference in settings #867

Fix #630 - Add map zoom control with checkbox preference in settings #867

Conversation

m-yang commented Apr 4, 2018 • edited by barbeau Loading

CLAassistant commented Apr 4, 2018 • edited Loading

m-yang commented Apr 5, 2018

barbeau commented Apr 5, 2018

m-yang commented Apr 11, 2018

barbeau commented Apr 11, 2018 • edited Loading

m-yang commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbeau commented Apr 17, 2018

m-yang commented Apr 17, 2018 • edited Loading

barbeau commented Apr 17, 2018 • edited Loading

barbeau commented Apr 17, 2018

m-yang commented Apr 19, 2018 • edited Loading

barbeau commented Apr 19, 2018

m-yang commented Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

barbeau Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbeau commented Apr 20, 2018

m-yang commented Apr 20, 2018 • edited Loading

barbeau commented Apr 20, 2018

m-yang commented Apr 20, 2018

barbeau commented Apr 23, 2018

m-yang commented Apr 4, 2018 •

edited by barbeau

Loading

CLAassistant commented Apr 4, 2018 •

edited

Loading

barbeau commented Apr 11, 2018 •

edited

Loading

m-yang commented Apr 17, 2018 •

edited

Loading

barbeau commented Apr 17, 2018 •

edited

Loading

m-yang commented Apr 19, 2018 •

edited

Loading

m-yang commented Apr 20, 2018 •

edited

Loading

barbeau Apr 20, 2018 •

edited

Loading

m-yang commented Apr 20, 2018 •

edited

Loading