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 ClientOptions to be able to pass around client name and version #365

Merged
merged 3 commits into from Aug 13, 2022

Conversation

ntadej
Copy link
Collaborator

@ntadej ntadej commented Jul 9, 2022

Add ClientOptions to be able to pass around client name and version. The first client is Qt platform user agent setting. I can try to update other platforms that set user agent later.

The result example is

Vremenar/0.6.0 (macOS 12.4) MapLibreGL/9e31d615 (Qt 6.3.1)

Will investigate in a separate PR on how to override the git hash with a version when building a release build.

@ntadej ntadej requested review from rinigus and wipfli July 9, 2022 20:58
@ntadej ntadej force-pushed the qt-user-agent branch 2 times, most recently from ba6ce2a to 995cb41 Compare July 9, 2022 21:47
@ntadej ntadej closed this Jul 9, 2022
@rinigus
Copy link
Collaborator

rinigus commented Jul 10, 2022

This looks rather large. Let me know if you still want a review as you closed the PR.

@ntadej
Copy link
Collaborator Author

ntadej commented Jul 10, 2022

It was a mistake. Not sure what happened.

@wipfli
Copy link
Member

wipfli commented Jul 11, 2022

I don't quite understand what the use case is for client option. This is not just Qt related, right?

In many places client option appears next to resource options. I wonder if we should put the information which is in client options into resource options?

@ntadej
Copy link
Collaborator Author

ntadej commented Jul 11, 2022

I don't quite understand what the use case is for client option. This is not just Qt related, right?

In many places client option appears next to resource options. I wonder if we should put the information which is in client options into resource options?

It is to set client metadata basically. For now Qt user agent is set based on that but I plan to update other http classes for other platforms witht that.

I did not want to put into resource options as that is specific to the resource used (so tiles settings).

Copy link
Member

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

I reviewed half of the files up and including to platform/default. Please use the full varable names like resourceOptions instead of resourceOpts. Other than that, my main question is if this is a breaking change for iOS or Android users...

benchmark/api/render.benchmark.cpp Outdated Show resolved Hide resolved
benchmark/api/query.benchmark.cpp Outdated Show resolved Hide resolved
include/mbgl/map/map.hpp Outdated Show resolved Hide resolved
include/mbgl/storage/online_file_source.hpp Show resolved Hide resolved
include/mbgl/util/client_options.hpp Show resolved Hide resolved
platform/default/src/mbgl/storage/http_file_source.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/http_file_source.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/local_file_source.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/main_resource_loader.cpp Outdated Show resolved Hide resolved
platform/default/src/mbgl/storage/mbtiles_file_source.cpp Outdated Show resolved Hide resolved
@wipfli
Copy link
Member

wipfli commented Jul 13, 2022

And thanks for your work @ntadej :)

@ntadej
Copy link
Collaborator Author

ntadej commented Jul 16, 2022

Hi @wipfli, I cleaned-up the code a bit:

  • do not use abbreviated variable names
  • set defaults in mbgl::Map and mbgl::MapSnapshotter

Regarding iOS and Android. In iOS none of the headers were changed. In Android all interface should go through Java.

@ntadej ntadej requested a review from wipfli July 16, 2022 11:02
Copy link
Member

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

Thanks for you work @ntadej. This looks good to me. I left one or two coding questions for my own education. The render tests on my ubuntu machine after running #350 (comment) resulted in only one failing test, which is render-tests/icon-text-fit/enlargen-both which already fails on main.

CHANGELOG.md Outdated Show resolved Hide resolved
src/mbgl/util/client_options.cpp Show resolved Hide resolved
Co-authored-by: Oliver Wipfli <oliver.wipfli@leichteralsluft.ch>
@rinigus
Copy link
Collaborator

rinigus commented Jul 16, 2022

I'll try to build it for SFOS as well.

@rinigus
Copy link
Collaborator

rinigus commented Jul 16, 2022

Scratch that SFOS build - our OBS is down and not sure when I can do it.

@wipfli
Copy link
Member

wipfli commented Jul 16, 2022

What is SFOS and OBS?

@rinigus
Copy link
Collaborator

rinigus commented Jul 16, 2022

SFOS - Sailfish OS - mobile Linux distro
OBS - Open Build Service - set of servers that allows SFOS developers to build for a range of SFOS versions and architectures.

In the context of QMapLibreGL - SFOS is using ancient Qt (5.6) due to licensing changes introduced later. As we have some bits proprietary, company behind SFOS is reluctant to change. I am using QMapLibreGL for mapping app, hence my interest to make sure it works on older Qt as well :)

But looks like these changes are mainly on C++ part which should be fine as we have reasonably up to date gcc

@ntadej
Copy link
Collaborator Author

ntadej commented Jul 16, 2022

@rinigus, are there docker images with Sailfish OS? We may be able to setup some basic CI with them.

@rinigus
Copy link
Collaborator

rinigus commented Jul 16, 2022

@ntadej: there are some, but those are unofficial and I don't know how well they track SFOS releases. OBS has been a tool of choice for many of us. I'll have to look into docker SFOS images, but don't expect it soon - quite swamped right now.

Copy link
Collaborator

@rinigus rinigus left a comment

Choose a reason for hiding this comment

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

I managed to compile on for SFOS and it passed nicely. As I don't have time to review these large changes in any reasonable time and since @wipfli already did it, I will approve the changes on my side to avoid stalling PR

@rinigus
Copy link
Collaborator

rinigus commented Jul 16, 2022

As additional note: it is surprising that a relatively simple change in client options requires so many changes. I guess it is a shortcoming of the used library design

@ntadej ntadej requested a review from roblabs August 10, 2022 18:05
@ntadej
Copy link
Collaborator Author

ntadej commented Aug 10, 2022

Hi @roblabs, as we did not have time to discuss this one, I wonder if you have an opinion. I plan to implement this also for iOS and Android in a separate PR.

@ntadej ntadej merged commit 1341f33 into maplibre:main Aug 13, 2022
@ntadej ntadej deleted the qt-user-agent branch August 13, 2022 13:36
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

3 participants