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.

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

[android] Added dynamic scale widget to map view. #6555

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@clydebarrow
Contributor

clydebarrow commented Oct 3, 2016

Scale is enabled by default, can be disabled in xml or programmatically.
Scale is styleable via xml or programmatically for position, color and distance unit.
Test activity added to TestApp in map layout category.
Sundry refactoring and minor bug fixes found while working in MapView.java.

clydebarrow added some commits Oct 3, 2016

[android] Added dynamic scale to map view.
    Scale is styleable via xml or programmatically
    for position, color and distance unit.
    Test activity added to TestApp in map layout category.
    Some minor refactoring.
[android] Fix color not being saved for map style
Fix state not being saved in a couple of test activities
[android] revert a few changes after review.
Fix potential leak of LocationListener in mapview.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 3, 2016

@clydebarrow, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tobrun, @cammace and @bleege to be potential reviewers.

mention-bot commented Oct 3, 2016

@clydebarrow, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tobrun, @cammace and @bleege to be potential reviewers.

@clydebarrow

This comment has been minimized.

Show comment
Hide comment
@clydebarrow

clydebarrow Oct 3, 2016

Contributor

screenshot 2016-10-03 09 29 16

![scale1](https://cloud.githubusercontent.com/assets/2366188/19028208/4448d434-8985-11e6-83de-c7909a86cbc0.png)
Contributor

clydebarrow commented Oct 3, 2016

screenshot 2016-10-03 09 29 16

![scale1](https://cloud.githubusercontent.com/assets/2366188/19028208/4448d434-8985-11e6-83de-c7909a86cbc0.png)

@tobrun tobrun added the Android label Oct 3, 2016

@tobrun tobrun self-assigned this Oct 3, 2016

@tobrun

This comment has been minimized.

Show comment
Hide comment
@tobrun

tobrun Oct 3, 2016

Member

Thank you for contributing, this seems great!
Putting on my radar to review.

Member

tobrun commented Oct 3, 2016

Thank you for contributing, this seems great!
Putting on my radar to review.

@snodnipper

This comment has been minimized.

Show comment
Hide comment
@snodnipper

snodnipper Oct 3, 2016

As an outsider, my initial thought on this PR was whether a standalone widget may keep the SDK code tighter, similar to MAS. AFAIK, Google Maps doesn't have a scale bar within the Android SDK.

For our mbgl demo app, the UX team wanted the scale bar to move etc. based on the chrome around the map - so I had to move the compass and add one of our (branded) android scale bars (because gravity alone wasn't going to work).

Slightly topical given mapbox/mapbox-java#131 but we used RXJava to keep the code fairly clean. For example:

mMetersPerPixelObservable.subscribe(mpp -> mScaleBar.setMetersPerPixel(mpp););

snodnipper commented Oct 3, 2016

As an outsider, my initial thought on this PR was whether a standalone widget may keep the SDK code tighter, similar to MAS. AFAIK, Google Maps doesn't have a scale bar within the Android SDK.

For our mbgl demo app, the UX team wanted the scale bar to move etc. based on the chrome around the map - so I had to move the compass and add one of our (branded) android scale bars (because gravity alone wasn't going to work).

Slightly topical given mapbox/mapbox-java#131 but we used RXJava to keep the code fairly clean. For example:

mMetersPerPixelObservable.subscribe(mpp -> mScaleBar.setMetersPerPixel(mpp););
@clydebarrow

This comment has been minimized.

Show comment
Hide comment
@clydebarrow

clydebarrow Oct 3, 2016

Contributor

@snodnipper A valid question and one I had thought about.

Firstly there is definitely some demand for the feature - there is more than one feature request lodged here for something similar, and it was something I kept looking for on my maps.

Originally I implemented it outside the SDK by subclassing Mapview but there were a few details that anyone wanting to use that had to get right to make it work. Nothing show-stopping, but not quite plug and play.

Looking at the SDK it became apparent that there were already models of similar widgets - the compass and the attribution - that the scale bar would sit nicely alongside both in concept and implementation.

Finally, the MAS link you provided illustrates that at present there is, AFAIK, no established framework for stand-alone addons to mapbox. It's not immediately obvious (without reading the source) what that code does and whatever it is, it's just an example, not a plug-and-play widget.

One question I'm ambivalent on is whether the scale bar should be enabled by default (in my PR it is.) On balance I think that's fine, because it enables easy discovery that the feature exists.

Contributor

clydebarrow commented Oct 3, 2016

@snodnipper A valid question and one I had thought about.

Firstly there is definitely some demand for the feature - there is more than one feature request lodged here for something similar, and it was something I kept looking for on my maps.

Originally I implemented it outside the SDK by subclassing Mapview but there were a few details that anyone wanting to use that had to get right to make it work. Nothing show-stopping, but not quite plug and play.

Looking at the SDK it became apparent that there were already models of similar widgets - the compass and the attribution - that the scale bar would sit nicely alongside both in concept and implementation.

Finally, the MAS link you provided illustrates that at present there is, AFAIK, no established framework for stand-alone addons to mapbox. It's not immediately obvious (without reading the source) what that code does and whatever it is, it's just an example, not a plug-and-play widget.

One question I'm ambivalent on is whether the scale bar should be enabled by default (in my PR it is.) On balance I think that's fine, because it enables easy discovery that the feature exists.

@tobrun tobrun referenced this pull request Oct 4, 2016

Closed

Widget system #6570

@tobrun

This comment has been minimized.

Show comment
Hide comment
@tobrun

tobrun Oct 4, 2016

Member

Thanks you guys for the remarks, I ticketed the feedback into #6570.

Member

tobrun commented Oct 4, 2016

Thanks you guys for the remarks, I ticketed the feedback into #6570.

@tobrun

This comment has been minimized.

Show comment
Hide comment
@tobrun

tobrun Oct 4, 2016

Member

I'm going to add the android-future milestonefor now, we are currently planning a new release and don't want to impact stability by introducing new features at this point. We are currently thinking of merging this once the release lands. Probably in combination with refactoring in #6570. Thanks again for the contribution @clydebarrow!

Member

tobrun commented Oct 4, 2016

I'm going to add the android-future milestonefor now, we are currently planning a new release and don't want to impact stability by introducing new features at this point. We are currently thinking of merging this once the release lands. Probably in combination with refactoring in #6570. Thanks again for the contribution @clydebarrow!

@tobrun tobrun removed their assignment Oct 4, 2016

@tobrun tobrun added this to the android-future milestone Oct 4, 2016

@1ec5 1ec5 referenced this pull request Oct 7, 2016

Closed

Scale control #1278

@1ec5 1ec5 added this to Needed for Android SDK v5.0.0 in release-ios-v3.5.0-android-v5.0.0 Mar 4, 2017

@tobrun tobrun removed this from the android-v5.0.0 milestone Mar 6, 2017

@1ec5 1ec5 removed this from Needed for Android SDK v5.0.0 in release-ios-v3.5.0-android-v5.0.0 Mar 15, 2017

@tobrun

This comment has been minimized.

Show comment
Hide comment
@tobrun

tobrun Jan 5, 2018

Member

This is going to be implemented as a plugin in mapbox/mapbox-plugins-android#11

Member

tobrun commented Jan 5, 2018

This is going to be implemented as a plugin in mapbox/mapbox-plugins-android#11

@tobrun tobrun closed this Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment