Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Library resource prefix breaks current attrs.xml #4021

Closed
tobrun opened this issue Feb 18, 2016 · 5 comments · Fixed by #4109
Closed

Library resource prefix breaks current attrs.xml #4021

tobrun opened this issue Feb 18, 2016 · 5 comments · Fixed by #4109
Assignees
Labels
Android Mapbox Maps SDK for Android bug

Comments

@tobrun
Copy link
Member

tobrun commented Feb 18, 2016

For #3119 it seemed a great improvement to add resourcePrefix to build.gradle.
This enforces that resources should start with a certain prefix.
For this project I have chosen for the mapbox_ prefix.

This resulted in following strings.xml:

 <?xml version="1.0" encoding="utf-8"?>
<resources>
    <string name="mapbox_compassContentDescription">Map compass. Click to reset the map rotation to North.</string>
    <string name="mapbox_attributionsIconContentDescription">Attribution icon. Click to show attribution dialog.</string>
    <string name="mapbox_attributionsDialogTitle">Mapbox Android SDK</string>
    <string name="mapbox_attributionTelemetryTitle">Make Mapbox Maps Better</string>
    <string name="mapbox_attributionTelemetryMessage">You are helping to make OpenStreetMap and Mapbox maps better by contributing anonymous usage data.</string>
    <string name="mapbox_mapboxIconContentDescription">The Mapbox logo.</string>
    <string name="mapbox_infoWindowTitle">Title</string>
    <string name="mapbox_infoWindowDescription">Description</string>
    <string name="mapbox_infoWindowAddress">Address</string>

    <!-- these are public -->
    <string name="mapbox_style_mapbox_streets">mapbox://styles/mapbox/streets-v8</string>
    <string name="mapbox_style_emerald">mapbox://styles/mapbox/emerald-v8</string>
    <string name="mapbox_style_light">mapbox://styles/mapbox/light-v8</string>
    <string name="mapbox_style_dark">mapbox://styles/mapbox/dark-v8</string>
    <string name="mapbox_style_satellite">mapbox://styles/mapbox/satellite-v8</string>
    <string name="mapbox_style_satellite_streets">mapbox://styles/mapbox/satellite-hybrid-v8</string>
</resources>

This is a great improvement in comparison with before, the resource prefix requires us to follow certain guidelines. Now with implementing #4018, I'm noticing this is required also for other types of resources:
eg attrs.xml:

screen shot 2016-02-18 at 16 36 34

At first this doesn't seem a problem, but when you look at the end result in java code:

typedArray.getBoolean(R.styleable.mapbox_MapView_mapbox_zoom_enabled, true));

instead of

typedArray.getBoolean(R.styleable.MapView_scroll_enabled, true)

I'm personally not a big fan of this, but this API is only for internal use.
End users will only see the following API:

screen shot 2016-02-18 at 16 40 50

What are your opinion on this? @bleege @zugaldia

@tobrun tobrun added bug Android Mapbox Maps SDK for Android labels Feb 18, 2016
@tobrun tobrun added this to the android-v4.0.0 milestone Feb 18, 2016
@bleege
Copy link
Contributor

bleege commented Feb 18, 2016

@tobrun Let's hold on this for time being as there are more immediate needs that require our bandwidth, namely better testing support.

@tobrun
Copy link
Member Author

tobrun commented Feb 19, 2016

@bleege agreed! I'm quickly going to disable this for now in #4040. Not cool having errors/warnings in the IDE. This is still something that we should discuss before releasing 4.0.0 since as you noticed can be a breaking change.

@zugaldia
Copy link
Member

I'm personally not a big fan of this, but this API is only for internal use.

If it's only for internal use, not a major problem. However, let's avoid unnecessary mapbox_ prefixes like in mapbox_style_mapbox_streets and let's stick to one kind of capitalization (mapbox_compassContentDescription vs mapbox_style_mapbox_streets).

This is still something that we should discuss before releasing 4.0.0 since as you noticed can be a breaking change.

If the only public facing change is for mapbox_access_token, can we just implement manually that one, and leave the larger lift for after 4.0.0?

@tobrun
Copy link
Member Author

tobrun commented Feb 25, 2016

@zugaldia
both the styles and the attributes you mentioned above are public.
You can see all our public resources in public.xml

For now I'm going to drop the mapbox_ prefix for all resources except for mapbox_access_token.

@zugaldia
Copy link
Member

@tobrun 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants