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.

Sign up for GitHub

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

Jump to bottom

add a completer for the file widget when creating a new GPKG #35072

Merged
merged 5 commits into from
Mar 15, 2020
Merged

add a completer for the file widget when creating a new GPKG #35072

merged 5 commits into from
Mar 15, 2020

Conversation

3nids
Copy link
Member

@3nids 3nids commented Mar 13, 2020

image

@3nids 3nids requested a review from nirvn March 13, 2020 23:50
@github-actions github-actions bot added this to the 3.14.0 milestone Mar 13, 2020
nyalldawson
nyalldawson reviewed Mar 14, 2020
View reviewed changes
src/gui/qgsnewgeopackagelayerdialog.cpp Outdated
@@ -129,6 +130,17 @@ QgsNewGeoPackageLayerDialog::QgsNewGeoPackageLayerDialog( QWidget *parent, Qt::W
}
checkOk();
} );

settings.beginGroup( "ogr/GPKG/connections", QgsSettings::Providers );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use the connections API instead of raw setting traversal?

Copy link
Member Author

Choose a reason for hiding this comment

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

do you have a pointer to the API? which class is it? cheers

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://qgis.org/api/classQgsProviderMetadata.html#af560b01841d55eb53b430e92bb7aaac7

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually - you could probably just create the completer directly using QgsProviderConnectionModel.

@nirvn
Copy link
Contributor

nirvn commented Mar 14, 2020

Nice move. Would be great to see the same applied to the new spatialite layer dialog.

@3nids
Copy link
Member Author

3nids commented Mar 14, 2020

here you go
image

@3nids
Copy link
Member Author

3nids commented Mar 14, 2020

I am just not sure if this needs filtering to gpkg.
I have an sqlite in my browser and it's not listed. Are now the spatialite always loaded using QGIS provider and not the OGR one?

@3nids 3nids requested a review from nyalldawson March 14, 2020 09:07
nyalldawson
nyalldawson reviewed Mar 14, 2020
View reviewed changes
src/gui/qgsnewgeopackagelayerdialog.cpp Outdated
@@ -129,6 +131,15 @@ QgsNewGeoPackageLayerDialog::QgsNewGeoPackageLayerDialog( QWidget *parent, Qt::W
}
checkOk();
} );

QgsProviderConnectionModel *ogrProviderModel = new QgsProviderConnectionModel( "ogr" );
Copy link
Collaborator

Choose a reason for hiding this comment

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

QStringLiteral

Copy link
Collaborator

Choose a reason for hiding this comment

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

Parent to "this", same with completer

@3nids 3nids requested a review from nyalldawson March 14, 2020 09:57
@elpaso
Copy link
Contributor

elpaso commented Mar 14, 2020

I am just not sure if this needs filtering to gpkg.

I think yes.

I have an sqlite in my browser and it's not listed. Are now the spatialite always loaded using QGIS provider and not the OGR one?

For spatialite? No, you can use both ogr and spatialite providers, when using the file browser you load them with ogr and when you use the connections or the data source dialog you load them with spatialite provider (yes, I know it's silly, we should probably just drop the spatialite provider and use only OGR for it).

For GPKGs we always use OGR provider.

@nirvn
Copy link
Contributor

nirvn commented Mar 14, 2020

@elpaso , -1 to drop spatialite driver prior to going through cost of doing so. A little over a year ago, that cost included a speed drop that would arguably not be acceptable.

We'd never consider dropping postgresql provider in favor of ogr either :)

@elpaso
Copy link
Contributor

elpaso commented Mar 14, 2020

@elpaso , -1 to drop spatialite driver prior to going through cost of doing so. A little over a year ago, that cost included a speed drop that would arguably not be acceptable.

Interesting, is there any metric available?

We'd never consider dropping postgresql provider in favor of ogr either :)

That's not a fair comparison :)

BTW look at who's maintaining it: I'm not dropping anything (even if I'd like to).

@nirvn
Copy link
Contributor

nirvn commented Mar 14, 2020

@elpaso , yeah , I know you've given your fair share of sweat on this provider.

The comparison to postgresql isn't fair but the basic argument remains: we have those providers for reasons and advantages that needs to be weighed seriously prior to taking decisions :)

No fresh metrics but easy enough to come up with using large datasets.

@elpaso
Copy link
Contributor

elpaso commented Mar 14, 2020

What really bothers me is that we are not running spatialite tests in travis.

@3nids
Copy link
Member Author

3nids commented Mar 14, 2020

@elpaso how can I filter using QgsProviderConnectionModel? I need to create the provider or is there a way to find out (without looking at the filename extension)?

@elpaso
Copy link
Contributor

elpaso commented Mar 14, 2020

@3nids I don't know, Nyall wrote it, but I see there is a provider (key) in the constructor.

@uclaros
Copy link
Contributor

uclaros commented Mar 14, 2020

Could also be applied to qgsnewspatialitelayerdialog.cpp, qgsnewvectorlayerdialog.cpp and qgsvectorlayersaveasdialog.cpp?
See also my related comment on having different dialogs for similar actions #34817 (comment)

@nyalldawson
Copy link
Collaborator

@3nids

how can I filter using QgsProviderConnectionModel

Use a QSortFilterProxyModel

@3nids
Copy link
Member Author

3nids commented Mar 15, 2020

@nyalldawson you wrote that ogr will strictly be gpgk, so no need to filter more, correct? if yes, ok to merge?
if not, my question was rather, am I forced to create the provider to know if it's a gpkg or a shapefile?
cheers

nyalldawson
nyalldawson approved these changes Mar 15, 2020
View reviewed changes
Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Merge away!

@3nids 3nids merged commit a93db7d into qgis:master Mar 15, 2020
@3nids 3nids deleted the gpgk-completer branch March 15, 2020 21:27
@nirvn
Copy link
Contributor

nirvn commented Mar 16, 2020

@3nids , just to avoid benevolent coding overlap, I'm handling the new spatialite dialog now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson approved these changes

@nirvn nirvn Awaiting requested review from nirvn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.14 (Feature)
Development

Successfully merging this pull request may close these issues.
5 participants
@3nids @nirvn @elpaso @uclaros @nyalldawson