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

[Android]Add MapEventFactory #14309

Merged
merged 12 commits into from
Apr 18, 2019
Merged

[Android]Add MapEventFactory #14309

merged 12 commits into from
Apr 18, 2019

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Apr 3, 2019

Remove map event creation code from the mapbox-events-android library and add it to map sdk.
Waiting for new version of telemetry sdk after this PR is merged.

@Chaoba Chaoba added the Android Mapbox Maps SDK for Android label Apr 3, 2019
@Chaoba Chaoba self-assigned this Apr 3, 2019
@Chaoba Chaoba changed the title [WIP}Add MapEventFactory [WIP][Android]Add MapEventFactory Apr 4, 2019
@andrlee andrlee requested a review from tobrun April 4, 2019 22:32
@Chaoba Chaoba changed the title [WIP][Android]Add MapEventFactory [Android]Add MapEventFactory Apr 8, 2019
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Sorry for requesting the following changes. I'm aware that you are just migrating the code from mapbox-events to here but this is the ideal opportunity for us to enforce code convention/cohension. Would you be able to pick this up @Chaoba ? I requested only changes to MapClickEvent but they are applicable to all MapEvent derived classes.

Next to those changes, can we also add some tests validating the contents of the event?

@Chaoba
Copy link
Contributor Author

Chaoba commented Apr 9, 2019

@tobrun Thanks for your advise, you are right, let's take this opportunity to make it best.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Made some comments on OfflineDownloadEndEvent but they are applicable to other events as well. Really like the new state model objects to avoid duplication.

If the comment about the parceable interface is correct and we don't need it . Maybe we can introduce a base class that extends Event and we can hide the empty parceable implementation? We could also move or enforce other shared items as: PhoneState and event.

Thank you for adding tests! One thing useful to test for is to see of the model classes are getting correctly serialized with Gson. So if we convert from and back with Gson we should have the same object. (to make this work, we will have to start adding equals/hashcode and a toString to all our events).

@Chaoba
Copy link
Contributor Author

Chaoba commented Apr 10, 2019

If the comment about the parceable interface is correct and we don't need it . Maybe we can introduce a base class that extends Event and we can hide the empty parceable implementation? We could also move or enforce other shared items as: PhoneState and event.

When this PR is merged, we can remove this empty implementation.

@tobrun
Copy link
Member

tobrun commented Apr 12, 2019

If the comment about the parceable interface is correct and we don't need it . Maybe we can introduce a base class that extends Event and we can hide the empty parceable implementation? We could also move or enforce other shared items as: PhoneState and event.

When this PR is merged, we can remove this empty implementation.

Lets include those changes as well in this PR, any eta on merging that?

@Chaoba
Copy link
Contributor Author

Chaoba commented Apr 15, 2019

Lets include those changes as well in this PR, any eta on merging that?

@andrlee Could you review and merge this PR? It seems we are blocked by it.

@andrlee
Copy link
Contributor

andrlee commented Apr 15, 2019

hey @Chaoba i'd like to avoid making changes to the public signatures of Event class and i don't believe that you should be blocked by it. i'd suggest to add a child class with default implementation to this PR.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looks good, :shipit:, two minor nit comments

@tobrun
Copy link
Member

tobrun commented Apr 17, 2019

@Chaoba the CI build is failing because we need to update the mapbox-events submodule in https://github.com/mapbox/mapbox-gl-native/tree/master/platform/android/vendor. LMK if you have any questions on how to do that.

@tobrun tobrun added the beta blocker Blocks the next beta release label Apr 17, 2019
@tobrun
Copy link
Member

tobrun commented Apr 18, 2019

4.4.1 of telemetry was released, can we integrate that in this PR? https://github.com/mapbox/mapbox-events-android/releases/tag/telem-4.4.1-core-1.3.0


schemaContainsFields(schema, fields);
}

Copy link
Member

Choose a reason for hiding this comment

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

@LukasPaczos LukasPaczos mentioned this pull request Apr 18, 2019
@LukasPaczos
Copy link
Member

I'm going to go ahead and merge. Remaining work ticketed in #14459.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android beta blocker Blocks the next beta release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants