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

[DB Manager] Fix delete raster layer in GeoPackage (Fix #57751) #57754

Merged

Conversation

agiudiceandrea
Copy link
Contributor

@agiudiceandrea agiudiceandrea commented Jun 13, 2024

Description

Fixes the "Delete..." functionality for raster layers in GeoPackage containers.

Fixes #57751.

Looking at the reported issue, I noticed that also the "Rename..." functionality doesn't work for raster layers in GeoPackage containers (it is not possible to rename a raster layer in a GeoPackage container even using the Browser panel #30907).
@elpaso it looks like a renameRasterTable function is missing in QgsGeoPackageProviderConnection.
@rouault, is there a "simple" way to rename a raster layer in a GeoPackage? (it seems to me ALTER TABLE ... RENAME TO ... doesn't correctly work for raster layers in GeoPackage)

UPDATE: I've also removed the Rename action for raster layer in a GeoPackage.
UPDATE: I'll submit a new PR to fix the Rename action for raster layer in a GeoPackage.

@github-actions github-actions bot added this to the 3.38.0 milestone Jun 13, 2024
@agiudiceandrea agiudiceandrea added DB Manager Relating to the DB Manager core plugin Bug Either a bug report, or a bug fix. Let's hope for the latter! backport queued_ltr_backports Queued Backports labels Jun 13, 2024
Copy link

github-actions bot commented Jun 13, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit a840ebf)

@agiudiceandrea agiudiceandrea changed the title [DB Manager] Fix delete raster layer in GeoPackage [DB Manager] Fix delete raster layer in GeoPackage (Fix #57751) Jun 13, 2024
@elpaso
Copy link
Contributor

elpaso commented Jun 13, 2024

See: #30907 (comment)

@agiudiceandrea
Copy link
Contributor Author

See: #30907 (comment)

This is not yet supported by GDAL.

I've also removed the Rename action for raster layer in a GeoPackage.

@rouault
Copy link
Contributor

rouault commented Jun 13, 2024

@rouault, is there a "simple" way to rename a raster layer in a GeoPackage?

no, it isn't supported currently. I've filed OSGeo/gdal#10201 about that

if isinstance(item, SchemaItem) or isinstance(item, TableItem):
if isinstance(item, SchemaItem) \
or (isinstance(item, TableItem)
and not (self.hasGPKGSupport and item.getItemData().type == Table.RasterType)):
Copy link
Contributor Author

@agiudiceandrea agiudiceandrea Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the best way to check if the item is a raster layer in a GeoPackage container...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB manager is ancient code, a more modern approach would be to use the connections API:

md = QgsProviderRegistry.instance().providerMetadata('postgres')
conn = md.createConnection("dbname='tinyows_demo' host=localhost port=5433 username='***' password='***'", {})
table = conn.table('public', 'demo3dlinestring')
bool(table.flags() & QgsAbstractDatabaseProviderConnection.TableFlag.Raster)
bool(table.flags() & QgsAbstractDatabaseProviderConnection.TableFlag.Vector)

It should work with all QGIS supported DB providers (GPKG included).

@elpaso
Copy link
Contributor

elpaso commented Jun 19, 2024

@agiudiceandrea see OSGeo/gdal#10213

Maybe you can add a branch to check for the next GDAL version.

@agiudiceandrea
Copy link
Contributor Author

@agiudiceandrea see OSGeo/gdal#10213

Maybe you can add a branch to check for the next GDAL version.

@elpaso thanks.
Do you plan to also add a renameRasterTable function in QgsGeoPackageProviderConnection? The DB Manager uses the renameVectorTable and renameRasterTable functions

if name in vector_table_names:
self.core_connection.renameVectorTable('', name, new_table)
else:
self.core_connection.renameRasterTable('', name, new_table)
and I think that will be also useful in order to fix the raster layer renaming in the Browser (#30907).

@elpaso
Copy link
Contributor

elpaso commented Jun 20, 2024

QgsGeoPackageProviderConnection

I will update QgsGeoPackageProviderConnection but a check for GDAL > 3.9.1 will be necessary.

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jun 20, 2024

I will update QgsGeoPackageProviderConnection but a check for GDAL > 3.9.1 will be necessary.

@elpaso, thanks again. I see that OSGeo/gdal#10213 has not been backported to the GDAL 3.9 branch so I it looks like it will be available in GDAL >= 3.10.

Anyway I think it would be better if I remove the last commit [db manager] Don't add a Rename action for raster layer in GeoPackage from this PR so this PR can be merged now.
I will then open a new PR to fix the renaming action for raster layer in GeoPackage after QgsGeoPackageProviderConnection will be updated.

elpaso added a commit to elpaso/QGIS that referenced this pull request Jun 20, 2024
@agiudiceandrea agiudiceandrea force-pushed the fix-57751-dbmanager-delete-raster-gpkg branch from 164ba02 to a840ebf Compare June 21, 2024 12:03
@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Jun 21, 2024

Hi @elpaso, I've removed the second commit from this PR. So it now only fixes the Delete functionality and can be merged and backported.
I'll open a new PR to also fix the Rename functionality with GDAL >= 3.10 thanks to your PRs OSGeo/gdal#10213 and #57812. The new PR cannot be backported to 3.34 since #57812 has not been backported to 3.34.

@elpaso elpaso merged commit 4b38aad into qgis:master Jun 24, 2024
104 of 105 checks passed
@agiudiceandrea
Copy link
Contributor Author

I'll open a new PR to also fix the Rename functionality with GDAL >= 3.10

PR: #57865.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! DB Manager Relating to the DB Manager core plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting raster from geopackage does nothing
4 participants