[qt] Rename QMapboxGL to QMapLibreGL #338
Conversation
There was a problem hiding this comment.
This is really cool, thanks a lot @ntadej for all the hard work! Some diffs seem to be IDE or auto-formatting related. Do we have some sort of formatting in CI?
For reviewing it would be really beneficial if you could split the changes into multiple commits where in one commit you move a file, then in a next one you replace mapbox with maplibre, and then in a next one you move parts of the code around. I know this is a bit of an overhead but for reviewing and making sure that everything is still there it would be really beneficial to have multiple commits...
Thanks Oliver for the review. I can try to split the commits if it helps with the review. Note that I did split some files so it may not be possible there. We do not have any formatting in the CI and all I did was manual. We can setup something but it's hard with such a large codebase. |
|
If the splitting into multiple commits is unpractical, I can also do a side by side comparison with to tabs and switching back and forth between them. Just let me know when this is ready for a final review and then I can go over the copied code... |
Splitting commits will certainly be a lot of work... Let me finish the rename first and then we can decide. |
|
I opened #340 on code formatting. I think that would be nice to have and I am happy to look into this... |
|
@ntadej, thank you for this work! It looks like other platforms haven't moved to MapLibre notation and are keeping Mapbox for now. But for Qt, it is a good time to make this change before QML/Qt plugin will make it into Qt. Right now breakage is probably for rather small number of apps. When done, I suggest to tag the first Qt version as well on repo level. Will start to look through it; will be able to compile a bit later |
There was a problem hiding this comment.
I had to add two QDebug includes to make it compile. Corresponding patch it at rinigus@50749b8
Still need to check on SFOS - at least one patch doesn't apply, would need to adjust it
|
One more patch is needed to fix CMake configs: rinigus@3d64920 |
|
After my patches, looks like QMapLibreGL native and adjusted MapboxGL-QML plugin are both building just fine. Thanks for making these changes! Tested builds on PC Linux and multiple SFOS versions (x86, arm32, aarch64) |
|
For @wipfli, I will test in more detail tomorrow on more platforms, but I think this is now ready to go in. I also tried to make multiple commits now after the initial one to help with the review. For @rinigus, I did some more renaming and you may want to build again but I do not expect any failures besides classname and includes update. |
|
@rinigus, just to let you know, I added you to the repository so you can use GitHub approvals to approve the MR. This will help Qt changes get in faster. |
|
@ntadej: thanks! let me recompile and adjust mapbox-gl-qml plugin for new changes in API and header locations. will be back after those tests |
Versioning is tricky in Native anyways. I think the last Qt version was 1.6, so I bumped it to 2.0.0 in the |
|
Why is the versioning tricky? |
Actually I think Qt followed Core versioning. Maybe it would be a reasonable idea to also bump that to 2.0 and keep Qt in sync with it? |
Look under "Releases" for this library. It's a mess with separate "releases" for Android and iOS. Nothing for Qt. Ideally, this mess should be stopped and some single release version tagged for all platforms. Which is probably just a dream at this stage... |
|
@ntadej you suggest to bump the core version to v2? I think we could rename mapbox to maplibre in the core and the do one big breaking change across platforms. Or what were you thinking of? |
|
I see @rinigus. But with the tag prefixes for the different platforms one can sort of see what is what... But yes, maybe splitting into a core plus a bunch of platform repos would be easiest... I don't know... |
|
@ntadej: finished testing on PC after adjusting my QML plugin. All seems to be working and new naming scheme is more consistent than before. No more mix with QMapLibre vs QMapLibreGL. I hope that this API will stick and we can focus on development :) . Will notify separately when SFOS builds will finish |
Yes, I plan to tag 2.0.0 once I implement the CI for all platforms I plan to support + implement user agent configuration. This should hopefully be relatively fast once this is merged. Note that user agent already changed in this MR, but is not configurable yet! |
|
@ntadej: rather massive PR. Looked through, not in super details, and ported Mapbox GL QML plugin to it. Code compiled on Linux PC and range of SFOS versions, as before. LGTM for merging; approved the changes as well. Thank you for your work! |
|
I promoted @rinigus from triage to write. Welcome to the club :) https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-roles-for-an-organization |
|
@wipfli, thanks! |
* Rename QMapboxGL to QMapLibreGL * Add missing QDebug includes * Fix Qt CMake config * Fix geojson warning * Remove redundant OpenGL dependency * Put everything under QMapLibreGL namespace * Reorganise headers Co-authored-by: Rinigus <rinigus.git@gmail.com>
Rename
QMapboxGLtoQMapLibreGLand split public headers a bit.Files are also reorganised into platform-specific (under
mbgl) and wrapper utilities (underutils).I'd appreciate the review from @rinigus, otherwise this should be purely technical. Tagging also @wipfli.
Keeping draft for now as I want to do some more testing, but I want to get some feedback already.