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

[QEP 149] Introduce static data providers #30234

Merged
merged 25 commits into from
Jun 26, 2019
Merged

Conversation

PeterPetrik
Copy link
Contributor

@PeterPetrik PeterPetrik commented Jun 15, 2019

Implementation of QEP: qgis/QGIS-Enhancement-Proposals#149

Sorry for the massive PR, but unfortunately the changes are in handling data providers and it really affect the code all over the place. I tried to not change any logic in the implementations and keep the function in their original location/file where possible. Some exceptions are in QgsGdalUtils and QgsOgrUtils where the functions are copied from the providers.

This work does not make it possible to build data providers as static libraries yet - but we are quite close.

Next steps (followup PRs)

  • update code + cmakelists of providers to make it possible to do a static build of qgis_core (including all providers)
  • separate gui-related code in providers from non-gui code (no need for HAVE_GUI #ifdefs)
  • adapt data providers to stop using the deprecated API calls (e.g. QgsDataItem implementations should not do any gui-related calls - if that's necessary they should use QgsDataItemGuiProvider for that)

It would be good to think more about the browser model and its data items. Does it make sens to keep QgsBrowserModel in qgis_core? Shouldn't it be in qgis_gui because a lot of functionality of data items is GUI related? (showing message boxes, dialogs, ...) Currently QgsDataItemGuiProvider is there to implement GUI code for data items, but it makes the implementation of data items a bit awkward (split into different areas of code).


A summary of all the major changes follows...

qgis_core

QgsProviderMetadata

  • [deprecated] library()
  • [deprecated] createFunction()
  • [removed] constructor QgsProviderMetadata( key, description, createFunc ) - but for Python we patch QgsProviderMetadata to keep the existing code working
  • [new] virtual functions
    • initProvider()
    • cleanupProvider()
    • filters()
    • createProvider()
    • createRasterDataProvider()
    • pyramidResamplingMethods()
    • decodeUri()
    • dataItemProviders()
    • listStyles()
    • getStyleById()
    • deleteStyleById()
    • saveStyle()
    • loadStyle()
    • createDb()
    • createTransaction()

QgsProviderRegistry

  • [deprecated] library()
  • [deprecated] providerCapabilities()
  • [deprecated] createSelectionWidget()
  • [deprecated] function()
  • [deprecated] createProviderLibrary()
  • [depreacted] registerGuis()
  • [new] createRasterDataProvider()
  • [new] pyramidResamplingMethods()
  • [new] dataItemProviders()
  • [new] listStyles()
  • [new] getStyleById()
  • [new] deleteStyleById()
  • [new] saveStyle()
  • [new] loadStyle()
  • [new] createDb()
  • [new] createTransaction()

QgsDataItem

  • [deprecated] acceptDrop()
  • [deprecated] handleDrop()
  • [deprecated] rename()
  • [deprecated] deleteLayer()

QgsProjectStorage

  • [deprecated] visibleName()
  • [deprecated] showLoadGui()
  • [deprecated] showSaveGui()

qgis_gui

QgsProviderGuiMetadata [new]

New interface for data providers for any GUI-related stuff. Currently allows providers to return:

  • data item gui providers
  • source select widget providers
  • project storage gui providers

QgsProviderGuiRegistry [new]

New registry for QgsProviderGuiMetadata implementations. To be used through QgsGui::providerGuiRegistry()

QgsProjectStorageGuiProvider [new]

New interface for data providers to implement gui-related functionality related to custom project storage.

QgsProjectStorageGuiRegistry [new]

New registry for QgsProjectStorageGuiProvider implementations. To be used through QgsGui::projectStorageGuiRegistry()

QgsBrowserGuiModel [new]

New class derived from QgsBrowserModel that makes use of QgsDataItemGuiProvider implementations. This class should be used in any gui code instead of QgsBrowserModel (which does not have access to QgsDataItemGuiProvider implementations and thus may not be able to use drag'n'drop of items etc.)

QgsDataItemGuiProvider

  • [new] rename()
  • [new] deleteLayer()
  • [new] acceptDrop()
  • [new] handleDrop()

Data Provider Interface

Dynamic providers (dll/so) now C-export only 2 functions instead of 10s of functions (isProvider, classFactory, …) that have been replaced by virtual methods in QgsGdalProviderMetadata or QgsGdalGuiProviderMetadata.

QgsProviderMetadata* providerMetadataFactory()
QgsProviderGuiMetadata* providerGuiMetadataFactory()

gdal and ogr providers

They have been moved to qgis_core and qgis_gui (i.e. those two providers are always present), together with the move they have been properly split into core/gui parts (using data item gui providers, project storage gui providers).

@PeterPetrik PeterPetrik added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging API Break! Breaks stable API. Proceed with extreme caution!! Cleanup Code cleanup Frozen Feature freeze - Do not merge! PyQGIS Related to the PyQGIS API Data Provider Related to specific vector, raster or mesh data providers labels Jun 15, 2019
@PeterPetrik PeterPetrik added this to the 3.10 milestone Jun 15, 2019
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.

(partial review only)

@PeterPetrik
Copy link
Contributor Author

Can you please use an enum class?

tried and it looks like it is not that easy with combination with QFlags. Agreed that will postpone the change until after this PR is merged

- adds QgsProviderGuiRegistry and QgsProviderGuiMetadata
- adds QgsProjectStorageGuiRegistry
- requires providerMetadataFactory for dynamic data providers
- requires providerMetadataGuiFactory for dynamic data providers (GUI only)
- removes QgsProviderRegistry::WidgetMode
- grass data item provider fixes
- removed QgsProviderMetadata constructor (with std::function / PyObject) due to sip errors (api break)
- reverted DataCapability move to Qgis - back to QgsDataProvider (avoiding api breaks)
- WidgetMode enum documentation
- sipify monkey patching fix
- renamed WidgetMode's "None" to "Normal" value - in python None has special meaning (api break)
@3nids
Copy link
Member

3nids commented Jun 20, 2019

Regarding the enum, I would like that we use scoped enums since they are much better looking in the Python API.

Regarding the equality operator, the question of using an equality sign can be debated since it is quite error prone for flags.
I have asked the question on stackoverflow, and apparently the solution would be to implement the operator.
Another approach would be to add a static method bool hasCapability( DataCapabilites flags ).

@m-kuhn m-kuhn removed the Frozen Feature freeze - Do not merge! label Jun 21, 2019
In C++ we do not support QgsProviderMetadata(key, description, createFunc) anymore,
but for python code we can have a patched class that still supports createFunc.

Why not keep the variant with createFunc in C++ as well? Because... SIP!
With newly added virtual methods in QgsProviderMetadata, SIP started to generate
a sip-specific subclass to handle derived classes in Python, however due to the variant
with PyObject* and custom MethodCode it was getting confused about what constructors
are available in C++ and failing to compile.
@wonder-sk wonder-sk removed the API Break! Breaks stable API. Proceed with extreme caution!! label Jun 23, 2019
@wonder-sk wonder-sk changed the title WIP: [QEP 149] Introduce static data providers [API BREAK] [QEP 149] Introduce static data providers Jun 23, 2019
@wonder-sk
Copy link
Member

The pull request should be ready for review & merge.

I have done a detailed review of @PeterPetrik 's changes, added some fixes, documentation improvements, reverted some API breaks and polished things a bit. I am now confident the PR is good to go.

It would be good to merge this PR soon as the refactoring ended up being huge - much bigger than I anticipated.

@wonder-sk
Copy link
Member

I have forgotten to mention that I have also updated the PR description with a summary of changes in the pull request.

python/core/auto_generated/qgsdataitem.sip.in Show resolved Hide resolved
python/core/auto_generated/qgsprovidermetadata.sip.in Outdated Show resolved Hide resolved
python/core/auto_generated/qgsprovidermetadata.sip.in Outdated Show resolved Hide resolved
python/core/auto_generated/qgsprovidermetadata.sip.in Outdated Show resolved Hide resolved
src/app/qgshandlebadlayers.cpp Show resolved Hide resolved
src/core/providers/gdal/qgsgdalprovider.cpp Outdated Show resolved Hide resolved
src/core/providers/gdal/qgsgdalprovider.cpp Outdated Show resolved Hide resolved
src/core/qgsdataprovider.h Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator

Very impressive work! (Also almost completely un-reviewable ;) )

It's the right time of the cycle to merge this kind of change anyway...

@m-kuhn
Copy link
Member

m-kuhn commented Jun 25, 2019

Impressive work.
I hope I'll find some time to go through it. But I'm scared already :D

@wonder-sk
Copy link
Member

Thanks a lot for taking time to review the code!! ...and apologies again for such monster sized pull request, but it really contains just the minimal amount of changes :-)

@nyalldawson
Copy link
Collaborator

Merge away!!

@wonder-sk wonder-sk merged commit cf2d878 into qgis:master Jun 26, 2019
@nirvn
Copy link
Contributor

nirvn commented Jun 26, 2019

@wonder-sk , @PeterPetrik , I think this broke the AMS provider. I get the following error on any layer I attempt to add to a project:

CRITICAL Invalid Layer : Raster layer Cannot instantiate the 'arcgismapserver' data provider (file: src/core/raster/qgsrasterlayer.cpp row: 603function setDataProvider:)

@jef-n
Copy link
Member

jef-n commented Jun 26, 2019

And the oracle provider.

@wonder-sk
Copy link
Member

AMS provider is fixed 984194b

Let me have a look at oracle...

jef-n added a commit that referenced this pull request Jun 26, 2019
@jef-n
Copy link
Member

jef-n commented Jun 26, 2019

Let me have a look at oracle...

@wonder-sk I hope you noticed early enough…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code cleanup Data Provider Related to specific vector, raster or mesh data providers PyQGIS Related to the PyQGIS API Requires Tests! Waiting on the submitter to add unit tests before eligible for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants