WMS layers added with the browser panel ignore image format preferences and default to PNG

miceg commented 1 month ago

What is the bug or the crash?

When adding a WMS layer using the Data Source Manager dialog, you can choose between all image encoding formats that the WMS server and QGIS supports. Layers added with the Data Source Manager also default to either using:

the first image <Format> which appears in the <Capabilities> -> <Request> -> <GetMap> of the server's GetCapabilities response
if the hidden qgis/WMSDefaultFormat is set and the server supports it, it defaults to that format instead

By comparison, layers added with the Browser panel:

do not appear to choose the first image <Format> in the GetCapabilities response
explicitly setting the (hidden) qgis/WMSDefaultFormat preference has no effect

Instead, layers appear to default to PNG format. I could not locate any preference (hidden or otherwise) which would change this.

WMS layers added with either the Data Source Manager dialog or the Browser panel both cannot change image formats after being added.

Steps to reproduce the issue

Create a connection to a WMS server which offers JPEG and PNG image formats, in that order. It should have a GetCapabilities response like this:

<Capability>
 <Request>
   <!-- snipped -->
   <GetMap>
     <Format>image/jpeg</Format>
     <Format>image/png</Format>
     <!-- snipped -->

Add a layer using QGIS Data Source Manager dialog (Layer -> Add layer -> Add WMS/WMTS layer...).

You should see the PNG and JPEG image encoding options, with it defaulting to JPEG, and this results in a JPEG WMS layer being added to the project.
Close the QGIS Data Source Manager dialog.
Add the same layer using the Browser panel.

This will add a PNG WMS layer to the project.

Versions

QGIS version	3.34.7-Prizren	QGIS code revision	6f7d735cae3
Qt version	5.15.2
Python version	3.9.5
GDAL/OGR version	3.3.2
PROJ version	8.1.1
EPSG Registry database version	v10.028 (2021-07-07)
GEOS version	3.9.1-CAPI-1.14.2
SQLite version	3.35.2
PDAL version	2.3.0
PostgreSQL client version	unknown
SpatiaLite version	5.0.1
QWT version	6.1.6
QScintilla2 version	2.11.5
OS version	macOS 14.4

Active Python plugins processing | 2.12.99 grassprovider | 2.12.99 db_manager | 0.1.20 MetaSearch | 0.3.6

Supported QGIS version

[X] I'm running a supported QGIS version according to the roadmap.

New profile

[X] I tried with a new QGIS profile

Additional context

Ideally there'd be a connection-level default for this – but even changing the Browser pane so that it adds layers following the same rules/preferences as the Data Source Manager dialog would be an improvement.

I think that the browser panel's format selection comes from here:

https://github.com/qgis/QGIS/blob/2621d95279aee1088defdf3269584cacc65aabb0/src/providers/wms/qgswmsdataitems.cpp#L435-L446

This iterates over the image formats reported by QgsWmsProvider::supportedFormats() (a static list), and chooses the first one in that list which the server reports supporting, and the first one checked is PNG.

I think that the data source manager dialog's format selection comes from here:

https://github.com/qgis/QGIS/blob/2621d95279aee1088defdf3269584cacc65aabb0/src/providers/wms/qgswmssourceselect.cpp#L303-L330

This iterates over the image formats in QgsWmsCapabilities::supportedImageEncodings() (those reported by the WMS server), and chooses the first one (or one in qgis/WMSDefaultFormat) which appears in QgsWmsProvider::supportedFormats(), so the first one checked is the first one in that capabilities element.

agiudiceandrea commented 1 month ago

@miceg, thanks for reporting and spending time to search and find the root cause of the issue. Are you going to propose a PR to fix the issue?

miceg commented 1 month ago

Probably not: while if my assumptions were all correct, I think I could make a fix (swap the comparison in QgsWMSItemBase::createUri() so it iterates over mCapabilitiesProperty.capability.request.getMap.format instead), the two issues that block me writing a PR are:

I think that having a test case is important, but I have no idea where to start with that – this looks like code which is very UI-adjacent.
While I haven't yet tried to build QGIS from source, I've found it exceptionally difficult to successfully build other Qt apps on non-Linux OSes (due to Qt's installer). This makes it hard to even manually test a fix – I'd either need to fix that or setup a Linux desktop development environment.

If I ignored those two issues, I'd end up with 50% of a PR and relying on CI breaking to tell me what is wrong – which doesn't sit well with me 😓

elpaso commented 1 month ago

Probably not: while if my assumptions were all correct, I think I could make a fix (swap the comparison in QgsWMSItemBase::createUri() so it iterates over mCapabilitiesProperty.capability.request.getMap.format instead), the two issues that block me writing a PR are:
* I think that having a test case is important, but I have no idea where to start with that – this looks like code which is very UI-adjacent.

* While I haven't yet tried to build QGIS from source, I've found it exceptionally difficult to successfully build other Qt apps on non-Linux OSes (due to Qt's installer). This makes it hard to even manually test a fix – I'd either need to fix that or setup a Linux desktop development environment.
If I ignored those two issues, I'd end up with 50% of a PR and relying on CI breaking to tell me what is wrong – which doesn't sit well with me 😓

I'm not saying a test wouldn't be nice but strictly speaking it is not required for such a change (deep into a provider code and UI/UX related).

I think that if it doesn't become an habit relying on CI to test an occasional small patch is acceptable.

If you feel like trying an old unmaintained attempt to low the barriers to new developers there is this recipe https://github.com/elpaso/qgis-dev-vagrant

qgis / QGIS