[QGIS-Developer] QBrowser, QgsGdalLayerItems, QgsOgrLayerItems and non valid layers

[Even Rouault]

On vendredi 7 juillet 2017 13:35:49 CEST Richard Duivenvoorde wrote:
> On 07-07-17 13:11, Even Rouault wrote:
> > So what I suspect here is that your QGIS is setup to only look at
> > extensions,
> > 
> > which is the default setting I think.
> > 
> > See
> > 
> > https://github.com/qgis/QGIS/blob/master/src/providers/gdal/qgsgdaldataite
> > ms.cpp#L145
> > 
> > and then
> > 
> > https://github.com/qgis/QGIS/blob/master/src/providers/gdal/qgsgdaldataite
> > ms.cpp#L239
> > 
> > And so for drivers that have both raster&vector capabilities, a file
> > will appear twice. Probably
> > 
> > this logic should be amended for dual capabiiity drivers like GPKG and
> > PDF. Actually L246 shows
> > 
> > a particular case for VRT. But I think this logic is now invalid since
> > GDAL 2.0, since
> 
> Hi Even,
> 
> Thanks for your prompt answer.
> 
> Yep, you are right. The default setting is apparently (to save
> resources?) to only look at the extension. If I use 'Check file
> contents' in "Options/Data Sources/Data source handling/Scan for valid
> items in the browser dock" all is fine.
> 
> Should we not make this default then?

Not all GDAL/OGR drivers have an implementation of the Identify() method (which just look 
at the first bytes of the file and/or extension typically), so sometimes this can involve a 
regular Open() method which can be costly. Although the situation is improving regularly 
over time...
But there are cases where there is a Identify() method but no conclusive results can be 
returnd. Consider all the SQLite based formats. Looking at the header is not enough, so 
basically the Identify() method of the SQLite/Spatialite, GPKG, MBTiles, Rasterlite driver 
returns either FALSE = "no, this is not a SQLite file", or  -1 = "I can't tell if this file matches this 
driver with just the information I have. So you have to Open() it to know".

> 
> And still: debugging this with the 'only look at extension' option you
> go through:
> 
> https://github.com/qgis/QGIS/blob/master/src/providers/gdal/qgsgdaldataitems
> .cpp#L43
> 
> where hDS then is a NULL... And all those items in the browser then show
> 'error' in their properties.

Ah indeed you're right, even if we only check by extension, then we end up still opening the 
file with GDALOpen :-)
So we should have either :
- do only extension checking and defer the call to 
https://github.com/qgis/QGIS/blob/master/src/providers/gdal/qgsgdaldataitems.cpp#L43 as 
late as possible. But I'm not sure if that's doable. I am merely discovering this piece of code
- or do extension checking as a pre-filter, but try GDALOpen() to ensure then that the file is a 
valid candidate. Would seem a more reasonable approach.

> So only looking only to the extention, the codes still tries to open
> them using gdal/ogr anyway.
> 
> Isn't it better then to add a flag that this item is not to be opened?
> And probably not to be shown in the tree?

Well, the dataItem() should return a nullptr instead of a QgsDataItem that isn't going to work

> 
> Same with ALL .txt files by the way (both in a zip and as file), they
> also appear as 'geodata' in the browser, and will fail upon opening.

The GeoConcept driver can open .txt files, hence what you observe

> 
> So currently I see a couple of special cases for which QGIS should check:
> 
> - .img files (which are not gis image files)
> - .pdf files
> - .gpkg files
> - .sqlite files
> - .txt files
> - .tab files (people tend to save tab-delimited files as .tab files)
> 
> Should I create an issue for this?
> 
> About the VRT / GDAL 2.0
> 
> Should I create an issue for that?

Yeah, both issues are valid, although it's not clear for the first one, what a solution that is 
both reliable (doesn't return false positive matches) and CPU & I/O efficient is...

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/qgis-developer/attachments/20170707/668b74ac/attachment-0001.html>