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

[ios , macos] Exclude CJK resources from offline download by default #14952

Merged
merged 15 commits into from
Jul 16, 2019

Conversation

m-stephen
Copy link
Contributor

Closes: #14176
After this PR: #14862 merged, we exclude CJK resources from offline download by default.

@m-stephen m-stephen added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Jun 18, 2019
@m-stephen m-stephen requested a review from a team June 18, 2019 07:16
@m-stephen m-stephen requested a review from 1ec5 as a code owner June 18, 2019 07:16
@m-stephen m-stephen changed the title Exclude CJK resources from offline download by default [iOS, macOS]Exclude CJK resources from offline download by default Jun 18, 2019
@m-stephen
Copy link
Contributor Author

cc: @chloekraw @julianrex

@chloekraw
Copy link
Contributor

Thanks for this PR, @m-stephen. Is this blocked by #14271?

@m-stephen
Copy link
Contributor Author

Thanks for this PR, @m-stephen. Is this blocked by #14271?

@chloekraw I don't think so, just on iOS side. I wrote a offline pack demo today and make a clean(no cache) app for testing:

  • Bounds: [LatLng(31.306819,120.578003), LatLng(31.32,120.59)]
  • MinZoom: 1
  • MaxZoom: 16
  • Style: MAPBOX_STREETS
  • Include ideographs: PingFang TC

And after download these offline resources, I switch to airplane mode and I think it rendered well:
image

cc: @zugaldia

@m-stephen
Copy link
Contributor Author

Btw, this PR should be approved after #14862 merged because we have changed MGLIdeographicFontFamilyName to a multiple value, especially when it is changed to a Boolean value, which means we turn off the local rendering.

@friedbunny friedbunny added this to the release-p milestone Jun 19, 2019
@friedbunny

This comment has been minimized.

@m-stephen

This comment has been minimized.

@julianrex

This comment has been minimized.

@m-stephen
Copy link
Contributor Author

@julianrex Sorry for forgetting upload test codes, already uploaded now. Please review it, thanks!

@friedbunny friedbunny changed the title [iOS, macOS]Exclude CJK resources from offline download by default [ios , macos] Exclude CJK resources from offline download by default Jul 9, 2019
Copy link
Contributor

@friedbunny friedbunny left a comment

Choose a reason for hiding this comment

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

A few nits and changelog edits, but otherwise LGTM.

platform/darwin/test/MGLOfflineRegionTests.m Outdated Show resolved Hide resolved
platform/darwin/test/MGLOfflineRegionTests.m Outdated Show resolved Hide resolved
platform/darwin/test/MGLOfflineRegionTests.m Outdated Show resolved Hide resolved
platform/darwin/test/MGLOfflineRegionTests.m Outdated Show resolved Hide resolved
platform/ios/CHANGELOG.md Outdated Show resolved Hide resolved
platform/macos/CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
m-stephen and others added 5 commits July 10, 2019 10:06
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
Co-Authored-By: Jason Wray <friedbunny@users.noreply.github.com>
@LukasPaczos
Copy link
Member

In #14952 (comment) you mentioned:

Include ideographs: PingFang TC

@m-stephen do you mean that your local font family for glyphs generation is set to PingFang TC, right? The downloaded region still doesn't include any ideographs?

@m-stephen
Copy link
Contributor Author

m-stephen commented Jul 11, 2019

@m-stephen do you mean that your local font family for glyphs generation is set to PingFang TC, right? The downloaded region still doesn't include any ideographs?

Yes. For clarifying this case, the PingFang TC is the local font family name, and setting the offline region's Include ideographs to NO. I didn't get any rendering issue on my phone. @LukasPaczos

@chloekraw
Copy link
Contributor

@m-stephen - we'd love to get this PR in before the release-p beta branch cut tomorrow. Could you resolve the conflicts and nits in this PR so we can get it merged? Thanks!

cc @chriswu42 @mapbox/maps-ios

@m-stephen m-stephen merged commit d2434d0 into master Jul 16, 2019
@m-stephen
Copy link
Contributor Author

@m-stephen - we'd love to get this PR in before the release-p beta branch cut tomorrow. Could you resolve the conflicts and nits in this PR so we can get it merged? Thanks!

Sorry for forgetting to merge this, already merged! @chloekraw

@chloekraw
Copy link
Contributor

No problem - thanks!

@m-stephen m-stephen deleted the Stephen-Exclude-offline-CJK-By-Default branch July 16, 2019 04:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS offline text rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ios] Exclude CJK resources from offline download by default.
5 participants