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

Add support to insert a layer above a reference layer #31

Merged
merged 4 commits into from
Jun 23, 2023

Conversation

JonasVautherin
Copy link
Collaborator

Right now, only the below layer is exposed. This exposes the above layer as well.

I do realise that there is some code generation setup in the repo (see scripts), but I don't really know how to use them, or even if you are planning on keeping them (the repo does not seem very active to be honest).

Are the changes fine like this, or should I try to get them through the codegen? For the latter, how do you run the codegen?

@RomanBapst: FYI

@louwers
Copy link
Collaborator

louwers commented Jun 16, 2023

@JonasVautherin Yes this repo is pretty stale.

Please see this issue: maplibre/maplibre-native#1189

We're planning to merge the Annotations functionality back into the main repo, for easier maintainability. If you could share your thoughts, also in the context of this issue, that would be very helpful.

@JonasVautherin
Copy link
Collaborator Author

Yes this repo is pretty stale.

Does that mean we should just maintain and publish a fork until we see how it goes with #1189 and get a chance to have our changes merged there?

@fynngodau
Copy link
Collaborator

@JonasVautherin Definetly a good question! I was not aware that we had code generation in this repo at all.

To answer the question how to use it: provided that you are at the root of your repository, and that nodejs is installed,

  1. Make sure the submodule is in place with git submodule init && git submodule update
  2. Run node plugin-annotation/scripts/code-gen.js

I think we can extract multiple todos from this:

  • document codegen
  • sync codegen with current code (as we had multiple PRs that did not update the codegen code, including from me)

@JonasVautherin
Copy link
Collaborator Author

sync codegen with current code (as we had multiple PRs that did not update the codegen code, including from me)

So now I think I have more questions 😅:

@louwers above seemed to suggest that those plugins would be rewritten soon. Should we bother getting the codegen in sync with the previous changes? I'm happy to make the codegen work for this PR, but it means that you will lose your previous changes, right?

@louwers
Copy link
Collaborator

louwers commented Jun 16, 2023

@JonasVautherin Happy to merge this and make a release.

Yeah there is code gen involved, I haven't really looked into it much. 🥲 Not sure if the files you edited are involved, but I don't think so. Some of the files you touched are indeed generated.

@louwers
Copy link
Collaborator

louwers commented Jun 22, 2023

@RomanBapst Could you also review these changes?

@JonasVautherin Let me know when it is ready to merge.

@JonasVautherin
Copy link
Collaborator Author

@RomanBapst Could you also review these changes?

Note that all I did was move the changes into the template and have the file auto-generated. So it's really the same thing, except for two UiThread annotations that we apparently did not need before (and that are now autogenerated).

Let me know when it is ready to merge.

@louwers: I think it would be nice to first merge #36, such that we could see here that it passes the unit tests here 😇

Comment on lines 376 to 381
style.addLayer(layer);
} else {
if (belowLayerId != null) {
style.addLayerBelow(layer, belowLayerId);
} else if (aboveLayerId != null) {
style.addLayerAbove(layer, aboveLayerId);
} else {
style.addLayer(layer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code appears to imply that aboveLayerId is ignored when belowLayerId is set. Do you think it would make sense to have a behavior noticeable to the developer to inform them that one parameter is being ignored?

In a Kotlinified version of this file (re. maplibre/maplibre-native#1255) I would expect both of them to be optional parameters and an exception to be thrown if both are provided.

@fynngodau
Copy link
Collaborator

I think it would be nice to first merge #36, such that we could see here that it passes the unit tests here

I merged #36, so if you were to rebase onto main, we would be able to get new test results :)

RomanBapst and others added 2 commits June 22, 2023 23:30
@JonasVautherin
Copy link
Collaborator Author

I think it's starting to look good 🎉

Copy link
Collaborator

@fynngodau fynngodau left a comment

Choose a reason for hiding this comment

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

@JonasVautherin I agree 🙂

@louwers I'll let you have a final look as well before merging.

@louwers louwers merged commit a19c210 into maplibre:main Jun 23, 2023
1 check passed
@JonasVautherin JonasVautherin deleted the above-layer-support branch June 23, 2023 08:08
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.

None yet

4 participants