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

[FEATURE][needs-docs] Add to Browser panel XYZ connection to provide default OpenStreetMap tiles #4352

Closed
wants to merge 2 commits into from
Closed

Conversation

jgrocha
Copy link
Member

@jgrocha jgrocha commented Apr 10, 2017

Description

This PR adds a default XYZ connection to the Browser panel. No layer is added to the map.
With this connection available, users can add the OpenStreetMap as a new layer with just one double click on it.

This feature was discusssed on the mailing list. The thread was started by @pcav.

The result is illustrated in the following screen shot:

openstreetmap xyz connection

UserAgent

The OpenStreetMap Tile Usage Policy regarding the use of a valid HTTP User-Agent identifying the application is respected. Even if the user changes the UserAgent in settings, the string QGIS/version is always added to the UserAgent defined. For this development version, the default userAgent string is Mozilla/5.0 QGIS/2.99.0-Master.

XYZ requests to another server (running MapProxy under Apache) were logged to confirm the valid userAgent:

a.geomaster.pt:80 188.250.64.156 - - [09/Apr/2017:22:44:25 +0000] "GET /mapproxy/wmts/osmpb/osm_grid/13/3897/3091.png HTTP/1.1" 200 10269 "-" "Mozilla/5.0 QGIS/2.99.0-Master"
a.geomaster.pt:80 188.250.64.156 - - [09/Apr/2017:22:44:25 +0000] "GET /mapproxy/wmts/osmpb/osm_grid/13/3899/3091.png HTTP/1.1" 200 8980 "-" "Mozilla/5.0 QGIS/2.99.0-Master"

Unit test

I was not able to have QgsSettings filled with the settings on the unit test. I've wrote this initial unit test, but it should be improved once the QgsSettings usage is more stable.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@nirvn
Copy link
Contributor

nirvn commented Apr 11, 2017

+1 to add some default there, -1 for way it's being done in this PR.

We would only trigger addition of an OSM tile server upon profile creation.

ATM this PR will re add the server if manually removed, or even simply renamed.

Thanks for tackling this. It'll surely be appreciated by many users.

@NathanW2
Copy link
Member

@jgrocha I'm going to add an initProfile method in my #4184 PR soon. Mind if we keep this open and integrate it into that hook once it's done.

The profile manager will fire a signal when a new profile is first being run in order to setup base defaults.

@NathanW2
Copy link
Member

Come to think of it. This kind of thing might be better as something in globel settings that @elpaso implemented. We should use that for any defaults that we want. Main issue is that can't be overriden by the user because it's in the install folder. @nirvn thoughts on that? initProfile or shipped as default in the package folder.

@alexbruy
Copy link
Contributor

Why not use same approach as in WMS/WFS dialogs? I mean "Add default servers" button which will add to the connection list some default servers.

@Gustry
Copy link
Contributor

Gustry commented Apr 11, 2017

I would not hard code the server URL in the code but rather having a resource file on qgis.org with a list of default providers. (They would need internet anyway to use providers).

  1. If the load coming from QGIS should be unexpectedly high and impact our service performance, there might come a time where we'd have to throttle or even switch off this access. You should have some mechanism or plan that deals with that to avoid frustration among your user base - maybe a mechanism where QGIS installations request updated tile sources from a central service so you could notify them of the OSM tiles not being available (or being available elsewhere) should the need arise.

Gnome was using MapQuest provider last year in their map application. Then suddenly, MapQuest switched off his service. The application on Gnome Desktop was out of service too with a big 404 tile. We need to avoid this case in QGIS.

@nyalldawson
Copy link
Collaborator

Why not use same approach as in WMS/WFS dialogs? I mean "Add default servers" button which will add to the connection list some default servers.

I think the intention here is that these are always available (unless a user explicitly removes them), as opposed to the "opt-in" approach used by WMS/WFS dialogs.

@nirvn
Copy link
Contributor

nirvn commented Apr 11, 2017

@alexbruy , what (I think) we agreed on in the mailing list is for OSM tiles to be available by default; The "add default servers" button method still requires someone to actually take action before those show up in the browser panel / source selection window. My suggestion here would actually be to remove this "add default servers" button once @NathanW2 's profile work lands, and add default WMS/WFS servers when creating a profile.

@Gustry , IMO a list of XYZ tile servers on qgis.org can be useful (or even redirect to OSM's wiki page on that), but can't replace having a default server in the browser panel when users first launch QGIS. Using the main OSM tile server greatly reduces the chance of a mapquest situation happening. But this eventual "server going down" is yet another reason why we can't enforce an hardcoded name + url upon eveyr QGIS launch 😄

@jgrocha
Copy link
Member Author

jgrocha commented Apr 11, 2017

Thanks for the comments @nirvn, @nyalldawson, @NathanW2, @alexbruy and @Gustry.
This PR can hold, for sure. I'll try both the initProfile approach related with QEP#82 and the global settings @elpaso is doing (presented on this gist). Both approaches gives us better support to integrate this default OpenStreetMap connection and several others without hard coding.

@nirvn
Copy link
Contributor

nirvn commented Apr 11, 2017

@NathanW2 , I would opt for the option that gives users the most freedom, including freedom to delete the server off his/her list of saved servers.

@elpaso
Copy link
Contributor

elpaso commented Apr 11, 2017

@jgrocha the QgsSettings class and is now in core and it's the QGIS recommended replacement of QSettings, so you can have configurable pre-defined default settings.
This should allow you to avoid hardcoding any URL or any other default setting.
What we (as QGIS) should probably do is to ship a default settings configuration file that contains the example and pre-defined URLs.
The advantage of doing this way would be doublefold: you do not need to rebuild QGIS and make a new release for a small URL change or addition and you can customize a QGIS installation package to include a particular settings configuration.

@elpaso
Copy link
Contributor

elpaso commented Apr 11, 2017

I forgot the link: https://github.com/qgis/QGIS/blob/master/src/core/qgssettings.h
In the python tests you can find some extended examples of its usage.

@jgrocha
Copy link
Member Author

jgrocha commented Apr 11, 2017

@elpaso Sorry for this basic question, but where do I add these kind of settings, out of the code?
I see that QgsSettings gets data from ~/.config/QGIS/QGIS3.conf in my Ubuntu OS based machine. But there is no default QGIS3.conf distributed with QGIS.
I supposed that QGIS 3 will be distributed with some kind of ini file (or equivalent). Which file/container is that?

@elpaso
Copy link
Contributor

elpaso commented Apr 12, 2017

@jgrocha I think that @NathanW2 knows more about the new profiles support he's working on, I can speak for the QgsSettings.

QgsSettings introduces the concept of global and user settings.

Global settings are optional and read-only, and are stored in an .ini file that can be loaded:

The purpose of the global settings is to provide default values to the QgsSettings.
With this in mind, there is no need to store any URL literal (or any other default settings) in the C++ code anymore.

When a user saves a new setting value, it is stored in ~/.config/QGIS/QGIS3.conf and the values in this file override (shadow completely) the values specified in the global settings.

So, I think that the times are mature to create a qgis_global_settings.ini where we can start to place our global defaults for the settings.

On installation, the file should be placed in the appropriate folder in order to be automatically loaded by QGIS: QgsApplication::pkgDataPath( ) + "/qgis_global_settings.ini"

@jgrocha
Copy link
Member Author

jgrocha commented Apr 12, 2017

Thanks @elpaso for the detailed explanation. I'll create this qgis_global_settings.ini under resources folder. I'll also check how this file can be moved to the proper place at installation time.

@m-kuhn
Copy link
Member

m-kuhn commented Apr 12, 2017

@jgrocha @elpaso would be great to use http://doc.qt.io/qt-5/qstandardpaths.html instead of (or between the env var and) pkgDataPath() - I think that's a perfect example for a file that could be shipped by a sysadmin or locally overwritten by a user. QStandardPaths allows doing that without messing with env variables or the QGIS3.conf file.

@dakcarto
Copy link
Member

@m-kuhn wrote:

@jgrocha @elpaso would be great to use http://doc.qt.io/qt-5/qstandardpaths.html instead of (or between the env var and) pkgDataPath() - I think that's a perfect example for a file that could be shipped by a sysadmin or locally overwritten by a user. QStandardPaths allows doing that without messing with env variables or the QGIS3.conf file.

Not sure I agree with using QStandardPaths for the packaged global settings. I do think that the following is appropriate for settings relative to what the QGIS project expects to be present with an install, i.e. similar to having the URLs, etc. still inline with source code:

QgsApplication::pkgDataPath( ) + "/qgis_global_settings.ini"

After that, any further overrides, like on QStandardPaths, etc. can override those global settings and persist between reinstalls of QGIS. We certainly don't want to be clobbering any settings outside of the base QGIS install.

Likewise, it's important that the global settings be a standard fallback, and always present, for those support instances where one asks a user to blow away all their override settings.

Unless I misunderstand what you are saying about QStandardPaths.

@m-kuhn
Copy link
Member

m-kuhn commented Apr 13, 2017

@dakcarto that's why I added the "(or between env var and)" part, this approach is perfectly fine for me as well.

The one point which I'm still not sure how to handle it best is "Likewise, it's important that the global settings be a standard fallback, and always present, for those support instances where one asks a user to blow away all their override settings.".

I remember a couple of occasions where

  • I found myself looking on the internet for the standard configuration file after having tampered with the one shipped with the installation (and either because it was not possible or I didn't know how to manually override the settings - in the end it was easiest to just edit in place).
  • I had to spend a lot of time to find out, where to find/put configuration files (one of the few things I don't like too much about postgres)

I think we can do better by

  • Using system standard locations
  • Giving the user a qgis-settings.conf.template file that he needs to manually copy to qgis-settings.conf in the very same folder. I think this is a soft way to suggest keeping this file around as a backup for reference.
  • Writing down the paths where this file can/should be overridden in the template.
  • Then we can still have an internal fallback one around somewhere in our installation path.

@elpaso
Copy link
Contributor

elpaso commented Apr 13, 2017

@m-kuhn http://doc.qt.io/qt-5/qstandardpaths.html#details it seems there are only user locations (~) here (except for tmp and runtime location)

I believe that we should keep this simple, as implemented in QgsSettings, but this has definitely to be documented, I was waiting for @NathanW2 to complete his profile, because I suspect that it may impact with the settings implementation, before writing the docs.

@dakcarto
Copy link
Member

@m-kuhn wrote:

that's why I added the "(or between env var and)" part, this approach is perfectly fine for me as well.

Ah, now your comments make sense to me. Sorry about that.

I think we can do better by

Using system standard locations

This is the tricky one. On macOS, for example, that would be ~/Library/Preferences/, as per QStandardPaths, but this is only where QGIS.app settings have been stored. Pragmatically, ~/.qgis2|3/ is where many other config items have been stored. Technically speaking, that folder on macOS should be in ~/Library/Application Support/QGIS/<maybe version here>, though that's a separate discussion.

Ignoring what may be done with @NathanW2's profiles setup for a minute, what would be the advantage of using the above QStandardPaths over continuing to use ~/.qgis2|3/ at this point, other than automatic searching of those directories?

Giving the user a qgis-settings.conf.template file that he needs to manually copy to qgis-settings.conf in the very same folder. I think this is a soft way to suggest keeping this file around as a backup for reference.
Writing down the paths where this file can/should be overridden in the template.
Then we can still have an internal fallback one around somewhere in our installation path.

+1 on these points, especially the .template one.

@NathanW2
Copy link
Member

NathanW2 commented Apr 13, 2017 via email

@m-kuhn
Copy link
Member

m-kuhn commented Apr 21, 2017

My main points are

  • one should never need to set environment variables, they should only be available as alternatives to other means
  • the defaults should be based on standards. I don't know the FHS very well, I just assume that QStandardPaths is based on it
  • one should not only focus on individual users or admins. Instead sysadmins should be allowed to ship defaults (on the system path) which individual users can override (in their writable user dirs). And optionally one can resort to env vars.

@jgrocha
Copy link
Member Author

jgrocha commented Aug 8, 2017

This PR is deprecated in favour of the new #5000.

@jgrocha jgrocha closed this Aug 8, 2017
@qgib qgib mentioned this pull request May 25, 2019
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

9 participants