Fix copyright attributions of map views [PSF-1058] - [PSF-1072] #6247

onurays · 2022-06-07T13:16:44Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Static map copyright text on the timeline item was not readable because of the font size
Static map copyright text on the timeline item of live location sharing was behind the informative tile
Mapbox copyright attribution items on the maximized map were behind the user list bottom sheet
This PR suggests to use custom copyright text on the timeline item and to fix the position of attribution items

Motivation and context

Required copyright attributes should always be visible and readible

Screenshots / GIFs

Before	After	Maximized

Tests

Share static location and you should see copyright text on timeline location item
Share live location (after enabling from labs flag) and you should see the copyright icon in maximized screen

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

yostyle · 2022-06-07T14:17:04Z

vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt

+                setLogoMargins(dimensionConverter.dpToPx(8), 0, 0, dimensionConverter.dpToPx(208))
+                setAttributionMargins(dimensionConverter.dpToPx(96), 0, 0, dimensionConverter.dpToPx(208))


Could you use the size of the view that these values depend on instead of using static values ?

Good idea, done. Although it is not possible to calculate the width of the "mapbox" logo.

…tes.

bmarty

One remark about the new string.

bmarty · 2022-06-07T21:38:46Z

vector/src/main/res/values/strings.xml

@@ -3036,6 +3036,7 @@
    <string name="labs_enable_live_location_summary">Temporary implementation: locations persist in room history</string>
    <string name="live_location_bottom_sheet_stop_sharing">Stop sharing</string>
    <string name="live_location_bottom_sheet_last_updated_at">Updated %1$s ago</string>
+    <string name="location_map_view_copyright">© MapTiler © OpenStreetMap contributors</string>


Do we really want this string to be translated? I think this is not necessary.

Exactly, moved.

mnaturel

I have just added some small remarks.

mnaturel · 2022-06-08T07:53:26Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/item/AbsMessageLocationItem.kt

@@ -111,5 +113,6 @@ abstract class AbsMessageLocationItem<H : AbsMessageLocationItem.Holder> : AbsMe
        val staticMapImageView by bind<ImageView>(R.id.staticMapImageView)
        val staticMapPinImageView by bind<ImageView>(R.id.staticMapPinImageView)
        val staticMapErrorTextView by bind<TextView>(R.id.staticMapErrorTextView)
+        val mapCopyrightTextView by bind<TextView>(R.id.mapCopyrightTextView)


Could we follow the same convention as for other ids by starting by staticMap prefix?

mnaturel · 2022-06-08T07:54:54Z

vector/src/main/java/im/vector/app/features/location/UrlMapProvider.kt

@@ -63,10 +62,8 @@ class UrlMapProvider @Inject constructor(
            append(height)
            append(".png")
            append(keyParam)
-            if (!localeProvider.isRTL()) {


The localeProvider dependency can be removed from this class since it is no longer used.

Right, done.

mnaturel · 2022-06-08T07:57:06Z

vector/src/main/java/im/vector/app/features/location/UrlMapProvider.kt

-                append("&attribution=bottomleft")
-            }
+            // Since the default copyright font is too small we put a custom one on map
+            append("&attribution=0")


Should we use attribution=false if it works: the intention may be clearer with a boolean.

Nice, works, done.

mnaturel · 2022-06-08T08:16:05Z

vector/src/main/java/im/vector/app/features/location/live/map/LocationLiveMapViewFragment.kt

+            mapboxMap.uiSettings.apply {
+                // Place copyright above the user list bottom sheet
+                setLogoMargins(dimensionConverter.dpToPx(8), 0, 0, bottomSheetHeight + dimensionConverter.dpToPx(8))
+                setAttributionMargins(dimensionConverter.dpToPx(96), 0, 0, bottomSheetHeight + dimensionConverter.dpToPx(8))


Could you also add this setAttributionMargins in MapTilerMapView? In this view, we only call setLogoMargins.

It is working well with default margins. Since I moved the position of attribution here I also needed re-set all the margins.

What I meant is what you have done is great (we see the logo + information icon). But I have seen in MapTilerMapView, we only call the setLogoMargins implying we only see the logo but not the information icon. I know it is out of the scope of this PR but we could align this behaviour and add the call to setAttributionMargins in MapTilerMapView so that we can see the information icon.

We actually see both logo and the info icon in MapTilerMapView. You can see it in maximized static location sharing.

I think the info icon is missing in the screen where user selects location sharing option. It is hidden under the options view. This is due to missing call to setAttributionMargins.

Oh, I missed this screen!

Nice catch, done!

bmarty · 2022-06-08T11:07:16Z

vector/src/main/res/values/donottranslate.xml

@@ -42,4 +42,6 @@
    <string name="ftue_auth_email_verification_subtitle">To confirm your email address, tap the button in the email we just sent to %s</string>
    <string name="ftue_auth_email_verification_footer">Did not receive an email?</string>
    <string name="ftue_auth_email_resend_email">Resend email</string>
+
+    <string name="location_map_view_copyright">© MapTiler © OpenStreetMap contributors</string>


Can you add translatable="false" please?

I didn't know that, done.

sonarqubecloud · 2022-06-09T10:44:16Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

bmarty

LGTM

onurays added 4 commits June 7, 2022 15:07

Create new styles for map view copyright text.

b455bf0

Use custom TextView for map view copyright.

b284b92

Fix position of the copyright on maximized map.

ec1eb49

Changelog added.

4b50058

onurays changed the title ~~Fix copyright attributions of map views~~ Fix copyright attributions of map views [PSF-1058] - [PSF-1072] Jun 7, 2022

onurays requested review from a team, yostyle and mnaturel and removed request for a team June 7, 2022 13:25

yostyle reviewed Jun 7, 2022

View reviewed changes

Use dynamic height to calculate the position of the copyright attribu…

68c6e52

…tes.

onurays requested a review from yostyle June 7, 2022 15:23

bmarty reviewed Jun 7, 2022

View reviewed changes

mnaturel reviewed Jun 8, 2022

View reviewed changes

Code review fixes.

adde210

onurays requested review from bmarty and mnaturel June 8, 2022 09:38

Remove unused import.

d0ab6af

bmarty reviewed Jun 8, 2022

View reviewed changes

Set translatable as false for the copyright text.

9b22880

onurays requested a review from bmarty June 8, 2022 12:18

Code review fix.

f9f1b3e

mnaturel approved these changes Jun 9, 2022

View reviewed changes

onurays requested review from a team and removed request for yostyle and a team June 10, 2022 10:46

bmarty approved these changes Jun 10, 2022

View reviewed changes

bmarty merged commit 990a683 into develop Jun 10, 2022

bmarty deleted the feature/ons/fix_static_map_copyright_size branch June 10, 2022 11:56

bmarty mentioned this pull request Jul 20, 2023

Pixel perfect static images element-hq/element-x-android#933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copyright attributions of map views [PSF-1058] - [PSF-1072] #6247

Fix copyright attributions of map views [PSF-1058] - [PSF-1072] #6247

onurays commented Jun 7, 2022 •

edited

Loading

yostyle Jun 7, 2022

onurays Jun 7, 2022

bmarty left a comment

bmarty Jun 7, 2022

onurays Jun 8, 2022

mnaturel left a comment

mnaturel Jun 8, 2022

onurays Jun 8, 2022

mnaturel Jun 8, 2022

onurays Jun 8, 2022

mnaturel Jun 8, 2022

onurays Jun 8, 2022

mnaturel Jun 8, 2022

onurays Jun 8, 2022

mnaturel Jun 8, 2022

onurays Jun 9, 2022

mnaturel Jun 9, 2022

onurays Jun 9, 2022

onurays Jun 9, 2022

bmarty Jun 8, 2022

onurays Jun 8, 2022

sonarqubecloud bot commented Jun 9, 2022

bmarty left a comment

		setLogoMargins(dimensionConverter.dpToPx(8), 0, 0, dimensionConverter.dpToPx(208))
		setAttributionMargins(dimensionConverter.dpToPx(96), 0, 0, dimensionConverter.dpToPx(208))

Fix copyright attributions of map views [PSF-1058] - [PSF-1072] #6247

Fix copyright attributions of map views [PSF-1058] - [PSF-1072] #6247

Conversation

onurays commented Jun 7, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a 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

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

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

sonarqubecloud bot commented Jun 9, 2022

bmarty left a comment

Choose a reason for hiding this comment

onurays commented Jun 7, 2022 •

edited

Loading