MAPSIOS-118 Implement DictionaryEncoder for Layer properties. #1476

maios · 2022-07-19T18:06:18Z

Encode Layer properties directly to a dictionary, instead of encoding to data using JSONEncoder and then serializing the data to a JSON dictionary.
Support encoding null values based on encoding context.
When updating a layer, merge the old properties to new properties, if a property is reset (old is non-nil and new is nil) then we will use the default value.

NOTE: This will depend on the upstream PR https://github.com/mapbox/mapbox-gl-native-internal/pull/3736 in order to support passing null StyleTransition

Pull request checklist:

Write tests for all new functionality. If tests were not written, please explain why.
Add documentation comments for any added or updated public APIs.
Describe the changes in this PR, especially public API changes.
Add a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).

- Encode Layer properties directly to a dictionary, instead of encoding to data using JSONEncoder and then serializing the data to a JSON dictionary. - Support encoding null values based on encoding context.

evil159

Very interesting to see a custom implementation of Swift's encoders and decoders 👍

evil159 · 2022-07-27T10:43:54Z

Tests/MapboxMapsTests/Foundation/StyleTests.swift

+
+        let layoutProperties = try XCTUnwrap(rootProperties["layout"] as? [String: Any])
+        XCTAssertEqual(layoutProperties["visibility"] as? String, "visible", "visibility is not reset and should keep old value")
+    }


Check for final layer values to be as expected.

it does not work with the test setup like this, because we would need to get a dictionary of layer's properties then decode it to a Layer, so we could only verify that when we call Style.updateLayer it will invoke its StyleManager with the expected parameters, which is a Dictionary of merged old properties and new properties.

Can we have integration tests for this?

evil159 · 2022-07-27T11:09:13Z

Sources/MapboxMaps/Foundation/DictionaryCoding/DictionaryEncoder.swift

+        }
+
+        func encodeIfPresent<T: Encodable>(_ value: T?, forKey key: Key) throws {
+            let shouldEncodeNilValue = userInfo[.shouldEncodeNilValues] as? Bool ?? false


With shouldEncodeNilValues baked into the source it'll be more clear to expose it as a property on DictionaryEncoder rather than user info key.
Primary use case for userInfo is for the user of the API to pass some contextual info down the line.

good point, this is done now!

evil159 · 2022-07-27T11:23:42Z

Sources/MapboxMaps/Foundation/DictionaryCoding/DictionaryEncoder.swift

+            }
+        }
+
+        func encodeIfPresent<T: Encodable>(_ value: T?, forKey key: Key) throws {


It seems that something is wrong with this - this method does not seem to be called for primitive types(like Double - e.g. minzoom is not encoded).

my bad, I didn't commit encodeIfPresent for primitive types :-D

evil159 · 2022-07-27T11:34:10Z

Tests/MapboxMapsTests/Foundation/DictionaryEncoderTests.swift

+            ["value": 10]
+        )
+        XCTAssertEqual(
+            try sut.encode(DummyNilable<Int>(value: nil)) as? [String: Int?],


There's some funky stuff going on - this works with a generic struct. But when using a similar specialized struct - the result is an empty dictionary.

private struct DummyFloatEncodable: Encodable { let value: Float? }

Maybe a sure way to test these would be to define a dedicated struct for every test 🤔

yes, I am also thinking of using stylegen to generate the tests case for each Layer's struct :-D let's see!

This test is expected the dictionary to be empty though? Did you mean to comment this in other places?

evil159 · 2022-07-27T11:37:24Z

Tests/MapboxMapsTests/Foundation/StyleTests.swift

@@ -344,4 +344,44 @@ final class StyleTests: XCTestCase {
            bounds: CoordinateBounds(southwest: .random(), northeast: .random()))
        )
    }
+
+    func testStyleCanUpdateLayer() throws {


Add tests that check that:

nilling out unmodified properties works correctly

updating a property value that already has a value works correctly

evil159 · 2022-07-27T11:44:50Z

Sources/MapboxMaps/Foundation/DictionaryCoding/DictionaryEncoder.swift

+        T.self == Data.self || T.self == NSData.self ||
+        T.self == Date.self || T.self == NSDate.self ||
+        T.self == Decimal.self || T.self == NSDecimalNumber.self ||
+        T.self == URL.self || T.self == NSURL.self


What does it mean that only these types are supported?

aye, the function name should be changed into something that makes more sense, but this is to check for the type and encode them properly (maybe later will be based on configuration i.e data will be encoded with base64, date will be encoded with ISO8601 etc)
I will rename the function.

- Add shouldEncodeNilValues as a property of DictionaryEncoder. - Fix encodeIfPresent for primitive types when a container types is not generic and update unit tests. - Update Data, Date, Decimal and URL encoding to match the default behavior of JSONEncoder.

evil159 · 2022-08-01T11:08:59Z

Sources/MapboxMaps/Foundation/DictionaryCoding/DictionaryEncoder.swift

+        }
+
+        func encodeIfPresent(_ value: Bool?, forKey key: Key) throws {
+            try encodeIfPresent(NilEncodable(value), forKey: key)


What does wrapping the value into NilEncodable do here? For me it seems that it is the same as passing value directly, if the value is nil - nothing will be encoded, non-nil - it will be encoded.

because these are overloading methods with same signature, so I wrap them in a struct so it will trigger func encodeIfPresent<T: Encodable>(_ value: T, forKey key: Key) instead, otherwise I would have to do the check for shouldEncodeNilValues in all the methods of encodeIfPresent
Another approach would be having a private method

private func encodeNilIfNeeded<T: Encodable>(_ value: T?, forKey key: Key) throws { guard value != nil || shouldEncodeNilValues else { return } try encode(value, forKey: key) }

then have all the encodeIfPresent to point to this one, what do you think?

Ah, makes sense 👍 I'm not sure if that is the case of premature optimization, but it seems to me that wrapping every primitive into a whole struct is could impact encoding performance. Because of this I'd prefer to have it as a separate method(like encodeNilIfNeeded in your comment).

this is done now :)

evil159

Well done 👍

maios added 4 commits July 20, 2022 00:59

MAPSIOS-118 Implement DictionaryEncoder for Layer properties.

755e731

- Encode Layer properties directly to a dictionary, instead of encoding to data using JSONEncoder and then serializing the data to a JSON dictionary. - Support encoding null values based on encoding context.

Remove new function as StyleEncodable protocol requirements

af9eed6

Update tests

69fc05b

Merge old and new layer properties in Style.updateLayer

97110da

maios marked this pull request as ready for review July 26, 2022 15:27

maios requested a review from a team as a code owner July 26, 2022 15:27

Update CHANGELOG.md

d5d3c2d

evil159 requested changes Jul 27, 2022

View reviewed changes

maios added 2 commits July 27, 2022 23:01

Address PR reviews

de79b9b

- Add shouldEncodeNilValues as a property of DictionaryEncoder. - Fix encodeIfPresent for primitive types when a container types is not generic and update unit tests. - Update Data, Date, Decimal and URL encoding to match the default behavior of JSONEncoder.

Update Style Integration tests

4060a07

maios requested review from evil159 and a team July 27, 2022 16:45

evil159 reviewed Aug 1, 2022

View reviewed changes

maios added 2 commits August 1, 2022 19:53

Update unit tests

cb09e35

Use encodeNilIfNeeded instead of wrapper struct

48ecfd6

evil159 approved these changes Aug 2, 2022

View reviewed changes

maios added 2 commits August 3, 2022 10:33

Merge branch 'main' into feature/MAPSIOS-118_layer-encode-nil

d8275b0

Remove unused NilEncodable struct

dede5c4

maios merged commit eeccb0e into main Aug 3, 2022

maios deleted the feature/MAPSIOS-118_layer-encode-nil branch August 3, 2022 05:08

mapbox-github-ci-writer-public-1 bot pushed a commit that referenced this pull request May 12, 2023

[v10] Add renamed flag to PuckBearingSource related APIs (#1476)

d9c97cb

OdNairy pushed a commit that referenced this pull request Aug 22, 2023

[v10] Add renamed flag to PuckBearingSource related APIs (#1476)

2129149

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAPSIOS-118 Implement DictionaryEncoder for Layer properties. #1476

MAPSIOS-118 Implement DictionaryEncoder for Layer properties. #1476

maios commented Jul 19, 2022 •

edited

Loading

evil159 left a comment

evil159 Jul 27, 2022

maios Jul 27, 2022

evil159 Jul 27, 2022 •

edited

Loading

evil159 Jul 27, 2022

maios Jul 27, 2022

evil159 Jul 27, 2022

maios Jul 27, 2022

evil159 Jul 27, 2022

evil159 Jul 27, 2022

maios Jul 27, 2022

maios Jul 27, 2022

evil159 Jul 27, 2022

maios Jul 27, 2022

evil159 Jul 27, 2022

maios Jul 27, 2022

evil159 Aug 1, 2022

maios Aug 1, 2022

evil159 Aug 1, 2022

maios Aug 1, 2022

evil159 left a comment

MAPSIOS-118 Implement DictionaryEncoder for Layer properties. #1476

MAPSIOS-118 Implement DictionaryEncoder for Layer properties. #1476

Conversation

maios commented Jul 19, 2022 • edited Loading

Pull request checklist:

evil159 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evil159 Jul 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

evil159 left a comment

Choose a reason for hiding this comment

maios commented Jul 19, 2022 •

edited

Loading

evil159 Jul 27, 2022 •

edited

Loading