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

Convert colors to sRGB for mbgl; fix title case unit tests #11391

Merged
merged 3 commits into from
Mar 8, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Mar 6, 2018

When converting an NSColor to mbgl::Color on macOS 10.13 High Sierra, automatically convert to the sRGB color space instead of the calibrated RGB color space. Colors set via the runtime styling API, as well as any colors displayed in the attribution buttons, should be more accurate now.

This PR fixes the macOS SDK’s unit tests so that they pass on High Sierra. The calibrated RGB color space differs between Sierra and High Sierra. In addition, a compile-time conditional compilation check added in #10224 has been refined to work on macOS, and the macOS targets have been upgraded to Swift 4 (a pro forma procedure).

Fixes #11289.

/cc @frederoni @friedbunny @julianrex

@1ec5 1ec5 added bug macOS Mapbox Maps SDK for macOS tests labels Mar 6, 2018
@1ec5 1ec5 added this to the macos-v0.7.0 milestone Mar 6, 2018
@1ec5 1ec5 self-assigned this Mar 6, 2018
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 6, 2018

mapbox/mapbox-gl-js#6285 tracks officially documenting the color space expected in the style JSON file format.

@frederoni
Copy link
Contributor

The implementation looks good. Not sure why it doesn't round trip.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 6, 2018

Still struggling with the tests on CircleCI, which apparently runs under Sierra rather than High Sierra:

    ✗ testProperties, ((rawLayer->getBackgroundColor()) equal to (propertyValue)) failed: ("<01000000 00000000 0000803f e8b5183e 00000000 0000803f 50f7048f c67f0000 a08c760a 01000000 501eb958 00000000 30ef048f c67f0000 b0ed048f c67f0000 f0ed048f c67f0000 50c5048f c67f0000 00706eb0 ff7f0000>") is not equal to ("<01000000 00000000 0000803f 00000000 00000000 0000803f a08c760a 01000000 30cb1c0a 01000000 50f7048f c67f0000 c01db958 ff7f0000 bb8b6eb0 ff7f0000 07030000 00000000 102c760a 01000000 201eb958 ff7f0000>") - Setting backgroundColor to a constant value expression should update background-color.
    ✗ testProperties, ((layer.backgroundColor) equal to (constantExpression)) failed: ("NSCalibratedRGBColorSpace 0.986246 0.00712079 0.0274342 1") is not equal to ("NSCalibratedRGBColorSpace 1 0 0 1") - backgroundColor should round-trip constant value expressions.

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 8, 2018

21 commits later, the builds are green. 😓 I’m going to squash and merge this one.

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.

😓

@@ -66,8 +66,17 @@ - (void)testTileSetFromTileURLTemplates {
// when the tile set has attribution infos
MGLAttributionInfo *mapboxInfo = [[MGLAttributionInfo alloc] initWithTitle:[[NSAttributedString alloc] initWithString:@"Mapbox"]
URL:[NSURL URLWithString:@"https://www.mapbox.com/"]];
MGLColor *redColor = [MGLColor redColor];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is redundant to the following lines.

Assume mbgl needs colors in the sRGB color space rather than the calibrated RGB color space. Fixed link colors when creating attribution from HTML.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug macOS Mapbox Maps SDK for macOS tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants