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

chore: updated MapLibre Native dependency to use the ios-v6.0.0 versi… #23

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

mindthefish
Copy link
Contributor

updated MapLibre Native dependency to use the ios-v6.0.0 version (https://github.com/maplibre/maplibre-native/releases/tag/ios-v6.0.0).
Full refactoring to MapLibre / MLN naming convention still to be done.
Please test and review in-depth, as most of the refactoring was made on a find&replace base.

@mindthefish mindthefish mentioned this pull request Feb 3, 2024
@boldtrn boldtrn mentioned this pull request Feb 5, 2024
@boldtrn
Copy link
Collaborator

boldtrn commented Feb 5, 2024

Thank you very much for looking into this. I will do some testing.

What's your general feeling testing this, should we rather try to merge this ASAP or keep open for testing for some time?

@mindthefish
Copy link
Contributor Author

For me, running the sample application works as it did before the refactoring. However, I had problems loading the Mapbox map style before and after, despite the access key (or does the style only consist of single-colored country areas without roads?). So setting the pin, retrieving the route, starting active navigation, voice output, worked without problems in my tests.
I would prefer to merge as soon as possible, especially to make quick progress with the SPM PR that has just been closed. Still, I'm relatively new to Swift, so I'd be grateful if someone with more expertise would take a few minutes to review. Thank you!

@boldtrn
Copy link
Collaborator

boldtrn commented Feb 5, 2024

or does the style only consist of single-colored country areas without roads?

Yes, this is currently intended, you can see this style used a few times here: https://maplibre.org/

You can set your own map style to overwrite this.

I would prefer to merge as soon as possible

I think this would be the best approach as well 👍

Still, I'm relatively new to Swift

Yep, same here :)

@boldtrn
Copy link
Collaborator

boldtrn commented Feb 5, 2024

BTW: the CI failed, maybe this needs to be updated?

@mindthefish
Copy link
Contributor Author

Thank you for the hint. I was wondering, why the CI checks did not run automatically. Can I somehow trigger the CI?

@boldtrn
Copy link
Collaborator

boldtrn commented Feb 5, 2024

I was wondering, why the CI checks did not run automatically. Can I somehow trigger the CI?

I think your first PR needs to get merged, before that your CI runs need to be manually approved. So next PR should work automatically 👍 .

@@ -1115,45 +1115,45 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
@objc(MBNavigationMapViewDelegate)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be MLNavigationMapViewDelegate as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably the same should apply to MBNavigationMapViewCourseTrackingDelegate?

Copy link
Contributor Author

@mindthefish mindthefish Feb 7, 2024

Choose a reason for hiding this comment

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

Ok, that slipped through. The same problem exists in countless other places. Where I'm not sure: We have swapped the prefix MGL for MLN SDK-wide. But for the @objc attribute the prefix MB was used. Should we then change this to ML or align the prefix and use MLN here as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense to swap out MBGL to MLN. We could do this in this PR or do this in a follow-up. I think only renaming a smaller part would be a bit weird.

I would be fine with merging this PR sooner rather than later and do a second renaming, maybe after #24 is merged as well to avoid too many conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MGL to MLN is not the problem. I was referring to the @obj attributes which have the prefix MB with 98 occurencies in 41 files:

image

Is there a problem renaming them to MLN as well?

We can also wait until #24 is merged, so I can resolve the conflicts before merging this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rename the @objc prefixes as well. I am fine with splitting this into two different PRs.

Personally, I think the Maplibre update is pretty high prio, so I'd say, let's merge it sooner rather than later. So everyone has enough time to test the current main before we do a release. Before we are releasing a new version, we should have the renaming done.

I don't know the timeline of #24.

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

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

Very nice PR, thank you very much for working on this. Would you mind writing a Changelog entry, describing what you changed and what implementers need to change?

Also could you check the naming issue I commented on?

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for working on this.

@boldtrn
Copy link
Collaborator

boldtrn commented Feb 7, 2024

Ok, I think let's go ahead with merging this, I will create a follow-up for renaming the @objc annotations.

@boldtrn boldtrn merged commit 3ab0456 into maplibre:main Feb 7, 2024
1 check passed
@mindthefish
Copy link
Contributor Author

Ok, I think let's go ahead with merging this, I will create a follow-up for renaming the @objc annotations.

Ok, this could be part of a more general refactoring from Mapbox related naming to MapLibre. I just tried to find out which prefix works best and came across errors when renaming e.g. @objc(MBStyle) to @objc(MLNStyle), because this class name name is already defined in MapLibre native itself. So we would need to agree on a renaming scheme that works.

Thank you for approving and merging.

@mindthefish mindthefish deleted the Update-to-MapLibre-6.0.0 branch February 8, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants