Skip to content
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

[Location sharing] Invisible text on map symbol #6688

Merged
merged 2 commits into from
Aug 2, 2022

Conversation

mnaturel
Copy link
Contributor

@mnaturel mnaturel commented Jul 29, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Convert the map symbol drawable to bitmap to be able to see text on it.

Motivation and context

Closes #6687

Screenshots / GIFs

Before After

Tests

  • Share you current location in a room with a user without picture
  • Open the new message
  • Check the shortname of the user is displayed on the map pin

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 11

Checklist

@mnaturel mnaturel added the PR-Small PR with less than 20 updated lines label Jul 29, 2022
@mnaturel mnaturel marked this pull request as ready for review July 29, 2022 08:31
@mnaturel mnaturel requested review from a team and ouchadam and removed request for a team July 29, 2022 08:32
@sonarcloud
Copy link

sonarcloud bot commented Jul 29, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ouchadam
Copy link
Contributor

ouchadam commented Aug 2, 2022

it would be handy to have screenshots for this fix 🙏

@@ -162,7 +163,7 @@ class MapTilerMapView @JvmOverloads constructor(
pinDrawable?.let { drawable ->
if (!safeMapRefs.style.isFullyLoaded ||
safeMapRefs.style.getImage(state.pinId) == null) {
safeMapRefs.style.addImage(state.pinId, drawable)
safeMapRefs.style.addImage(state.pinId, drawable.toBitmap())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know why this behaviour is different?

addImage(drawable) chains into BitmapUtils.getBitmapFromDrawable(drawable) which in turn calls Bitmap.createBitmap(drawable.getIntrinsicWidth(), drawable.getIntrinsicHeight(), Bitmap.Config.ARGB_8888)

whereas drawable.toBitmap() calls Bitmap.createBitmap(width, height, config ?: Config.ARGB_8888)

I would have thought them to be equivalent? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't curious enough to understand... Let me look at it to answer this question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some investigation it seems the only difference I see is that in BitmapUtils.getBitmapFromDrawable() they are creating a new drawable with Drawable drawable = constantState.newDrawable().mutate(); and then the bitmap is created from this new drawable. I suspect the Text part disappears due to this call. In the doc, it says density dependent properties may not be scaled well (https://developer.android.com/reference/android/graphics/drawable/Drawable.ConstantState#newDrawable()). Testing with following code leads to the same issue:

val bitmap = drawable.constantState?.newDrawable()?.toBitmap()
bitmap?.let { safeMapRefs.style.addImage(state.pinId, it) }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great to know, thanks for looking into it! 💯

@mnaturel
Copy link
Contributor Author

mnaturel commented Aug 2, 2022

it would be handy to have screenshots for this fix 🙏

I agree. I updated the description with captures.

@mnaturel mnaturel requested a review from ouchadam August 2, 2022 13:08
@mnaturel mnaturel merged commit a53de92 into develop Aug 2, 2022
@mnaturel mnaturel deleted the fix/mna/missing-text-on-map-symbol branch August 2, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Location sharing] Invisible text on map symbol
2 participants