-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
Conversation
ba6ce2a
to
995cb41
Compare
This looks rather large. Let me know if you still want a review as you closed the PR. |
It was a mistake. Not sure what happened. |
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). |
There was a problem hiding this 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...
And thanks for your work @ntadej :) |
Hi @wipfli, I cleaned-up the code a bit:
Regarding iOS and Android. In iOS none of the headers were changed. In Android all interface should go through Java. |
There was a problem hiding this 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.
Co-authored-by: Oliver Wipfli <[email protected]>
I'll try to build it for SFOS as well. |
Scratch that SFOS build - our OBS is down and not sure when I can do it. |
What is SFOS and OBS? |
SFOS - Sailfish OS - mobile Linux distro 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 |
@rinigus, are there docker images with Sailfish OS? We may be able to setup some basic CI with them. |
@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. |
There was a problem hiding this 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
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 |
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. |
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
Will investigate in a separate PR on how to override the git hash with a version when building a release build.