[FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles #5000

jgrocha · 2017-08-07T21:20:07Z

Description

This PR adds a default XYZ connection, so users can add easily OpenStreetMap tiles.
Currently, no layer is added to the map. Later we can add an option to add it by default when starting a new project, but only if the user enables it.

With this connection available, users can add the OpenStreetMap as a new layer with just one double click on it, using the old brower panel or the new brower.

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

There is a previous PR #4352 that can be closed if this one is merged. The previous PR contains the discussion that supports the approach taken. The approach used is based on the new qgis_global_settings.ini. This file is read only and support initial settings. The contents write now are:

[qgis]
connections-xyz\OpenStreetMap\authcfg=
connections-xyz\OpenStreetMap\password=
connections-xyz\OpenStreetMap\referer=
connections-xyz\OpenStreetMap\url=http://a.tile.openstreetmap.org/{z}/{x}/{y}.png
connections-xyz\OpenStreetMap\username=
connections-xyz\OpenStreetMap\zmax=19
connections-xyz\OpenStreetMap\zmin=0

Removal of global settings

To delete a global settings connection, it is necessary to write something on the local settings.
So, if the user decide to remove the OpenStreetMap default connection, a new entry will be added to the local settings:

connections-xyz\OpenStreetMap\hidden=true

This way, the connection will not be visible. If the user resets his local settings, the global one will be visible again.

Test

To test this PR on a developing environment (QGIS not installed) it is necessary to manually copy or link qgis_global_settings.ini to the current development QgsApplication::pkgDataPath() folder, is is problably the root of the repository. Just in case, it can be checked on the Python console with: QgsApplication.pkgDataPath(). In my case, I just need to;

git fetch upstream
git checkout pr/5000
ln -s resources/qgis_global_settings.ini .

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

m-kuhn · 2017-08-07T21:24:09Z

tests/src/python/test_qgis_global_settings.py

+    def tearDown(self):
+        """Run after each test."""
+        qDebug('tearDown')
+    pass


Watch out for the indentation here

elpaso · 2017-08-12T14:51:39Z

tests/src/python/test_qgis_global_settings.py

+        qDebug('settings.allKeys(): {0}'.format(settings.allKeys()))
+        defaulturl = settings.value('qgis/connections-xyz/OpenStreetMap/url')
+
+        def testKey():


is this function necessary?
I think you should just assert in the main body, same for testLayer

Thanks @elpaso
Done!

It was a WIP. Now I think it is fully implemented.
Now we support the removal of default connections by users.

elpaso

LGTM thanks!
Can you please address the two minor issues that I've commented in the review?

elpaso · 2017-08-31T12:13:57Z

src/providers/wms/qgsxyzconnection.cpp

+  QStringList global = settings.globalChildGroups();
+  settings.endGroup();
+
+  for ( auto &s : global )


To avoid a detach you might want to try the new qgsAsConst 029f741 recently added by @nyalldawson or use Q_FOREACH

It also should be const auto &

To avoid a detach you might want to try the new qgsAsConst 029f741 recently added by @nyalldawson or use Q_FOREACH

... or simply use const QStringList global

I prefer to avoid the Q_FOREACH.

I've used the suggested pattern:

const QStringList global = settings.globalChildGroups(); for ( const auto &s : global )

elpaso · 2017-08-31T12:16:52Z

python/core/qgssettings.sip

@@ -156,6 +156,11 @@ Returns a list of all top-level keys that can be read using the QSettings object
 Returns a list of all key top-level groups that contain keys that can be read using the QSettings object.
 :rtype: list of str
 %End
+    QStringList globalChildGroups() const;


Would you mind adding a test case for globalChildGroups() to https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgssettings.py ?

Thanks @elpaso. I discovered another minor issue writing the test. The test now covers the settings.globalChildGroups().

Fix container iteration

m-kuhn · 2017-09-01T08:23:04Z

tests/src/python/test_qgis_global_settings.py

        qDebug('QgsApplication.pkgDataPath(): {0}'.format(QgsApplication.pkgDataPath()))
        # Path after deployment
        # QgsSettings.setGlobalSettingsPath(QgsApplication.pkgDataPath() + '/qgis_global_settings.ini')
        # Path before deployment
-        QgsSettings.setGlobalSettingsPath(QgsApplication.pkgDataPath() + '/resources/qgis_global_settings.ini')
+        assert QgsSettings.setGlobalSettingsPath(QgsApplication.pkgDataPath() + '/resources/qgis_global_settings.ini')


Better use self.assertTrue

@m-kuhn thanks! Fixed!

elpaso · 2017-09-01T08:34:06Z

@jgrocha sorry if I didn't mentioned it before, but maybe would be better if you move the tests from globaltest_qgis_global_settings.py into test_qgssettings.py.
I don't see a good reason for adding a new test file for a single test case on a method that it's part of the main QgsSettings class.

jgrocha · 2017-09-01T08:40:26Z

@elpaso I was separating all logic regarding to the presence/absence of the new qgis_global_settings.ini in this new test_qgis_global_settings.py. But your suggestion is equally good and I'll move all tests t the same file.

…ault-layers

jgrocha · 2017-09-05T12:19:39Z

@elpaso The new method settings.globalChildGroups() was included in test_qgssettings.py.

Add XYZ connection to provide default OpenStreetMap tiles

273243b

m-kuhn reviewed Aug 7, 2017

View reviewed changes

jgrocha mentioned this pull request Aug 8, 2017

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

Closed

8 tasks

jgrocha changed the title ~~WIP: [FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles~~ [WIP][FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles Aug 9, 2017

elpaso requested changes Aug 12, 2017

View reviewed changes

jgrocha added 2 commits August 22, 2017 23:11

Support removal of global defined XYZ connections

8486767

Fix TRAVIS qgis_global_settings path

59aa5d3

jgrocha changed the title ~~[WIP][FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles~~ [FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles Aug 23, 2017

elpaso mentioned this pull request Aug 31, 2017

Welcome page improvements qgis/QGIS-Enhancement-Proposals#102

Closed

elpaso approved these changes Aug 31, 2017

View reviewed changes

Include settings.globalChildGroups in test

ce01bc0

Fix container iteration

m-kuhn reviewed Sep 1, 2017

View reviewed changes

Improve test: assert → assertTrue

7af72f7

jgrocha added 6 commits September 2, 2017 23:24

settings.globalChildGroups() test moved to test_qgssettings.py

390a34f

settings.globalChildGroups() test moved to test_qgssettings.py

dc50a3e

Merge remote-tracking branch 'origin/add-default-layers' into add-def…

a82dd5b

…ault-layers

remove unwanted files

aa4213f

remove unnecessary test

e681547

Improve test: assert → assertTrue

858413e

elpaso merged commit f0e021c into qgis:master Sep 8, 2017

jgrocha mentioned this pull request Mar 6, 2018

allows to add default XYZ Tiles Servers #6546

Closed

8 tasks

This was referenced May 24, 2019

Impossible to change from old symbology to new symbology #14802

Closed

Cannot activate new symbology for OSM layers #14823

Closed

program will not allow selection of new symbology 1.9.90 windows xp #14863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles #5000

[FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles #5000

jgrocha commented Aug 7, 2017 •

edited

m-kuhn Aug 7, 2017

elpaso Aug 12, 2017

jgrocha Aug 23, 2017

jgrocha Aug 23, 2017

elpaso left a comment

elpaso Aug 31, 2017

nyalldawson Aug 31, 2017

m-kuhn Aug 31, 2017

jgrocha Sep 1, 2017

elpaso Aug 31, 2017

jgrocha Sep 1, 2017

m-kuhn Sep 1, 2017

jgrocha Sep 1, 2017 •

edited

elpaso commented Sep 1, 2017

jgrocha commented Sep 1, 2017

jgrocha commented Sep 5, 2017

[FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles #5000

[FEATURE][needs-docs] Add XYZ connection to provide default OpenStreetMap tiles #5000

Conversation

jgrocha commented Aug 7, 2017 • edited

Description

Removal of global settings

Test

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elpaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrocha Sep 1, 2017 • edited

Choose a reason for hiding this comment

elpaso commented Sep 1, 2017

jgrocha commented Sep 1, 2017

jgrocha commented Sep 5, 2017

jgrocha commented Aug 7, 2017 •

edited

jgrocha Sep 1, 2017 •

edited