search

qgis / qgis4.0_api

Tracker for QGIS 4.0 API related issues and developer discussion
3 stars 1 forks source link

Singletons #42

Open m-kuhn opened 8 years ago

m-kuhn commented 8 years ago

Singletons have been discussed a lot as something to fix for 3.0.

A possible path forward may be to have one single singleton on top which owns all other "singletons" which would then explicitly be created and managed from this root singleton. This would give us full control over initialization order and allow instancing them from other points as well.

Feedback on and changes to the above structure are expected and encouraged!

nyalldawson commented 7 years ago

QgsCrsCache has been removed now

NathanW2 commented 7 years ago

I like this idea. It makes sense to just have a single place for everything and not singletons all over the place.

@wonder-sk thoughts?

vmora commented 7 years ago

Since QgisApp is not used in core/, and since I guess you don't intend to introduce the dependency, I understand this change implies that QgsProject::instance() will have to go out of core... do I understand well? If it's the case +1 for me.

m-kuhn commented 7 years ago

I think so. My favorite approach would be to drop QgsProject::instance() at some point in the future and instead have a QgisApp::activeProject property. But it will require a lot of time to properly address every usage of QgsProject::instance(), meanwhile with https://github.com/qgis/QGIS-Enhancement-Proposals/issues/76#issuecomment-257201342 at least we can have clearly separated projects and temporarily keep QgsProject::instance() in place.

luipir commented 7 years ago
  • QgisApp
    • QgsAuthManager / QgsCredentials (I guess unlocking should not be controlled globally? Is there an internal dependency between them

Observation: QgsAuthManager uses QgsCredentials just to interface pure virtual method to get MasterPassword: https://qgis.org/api/qgsauthmanager_8cpp_source.html#l02856

Question: I'm missing something, please someone can explain me why QgsAuthManager whould be initialised in QgisApp and not in the main singleton manager in QgsApplication?

m-kuhn commented 7 years ago

I wasn't quite sure how Credentials and AuthManager work exactly, my proposal is likely wrong. Question: is there always just one QgsAuthManager? Or can there be multiple different ones, for example in a shared server scenario?

luipir commented 7 years ago

Hmmmm... @m-kuhn good question about a shared served scenario. Architecturally is a really pure singleton, but I can't find concurrent access management. Would be necessary enhancement to use it in a server context. Any opinion @elpaso?

luipir commented 7 years ago

IMHO AuthManager in concurrent server context can be simplified stopping and starting server during permission management. BTW this depend if qgis server will evolve to a full featured map server instead of focusing more on the rendering task.

nyalldawson commented 7 years ago

https://github.com/qgis/QGIS/pull/3914 fixes:

elpaso commented 7 years ago

@luipir not sure if that's what you were asking for, but I think that there should not be any issue with concurrent servers on a single (SQLlite) auth DB: locking is handled by the DB engine itself. @m-kuhn, I don't see the need of multiple QgsAuthManager instances in the server: even when we are serving multiple project from a single instance (process) of QgsServer, unless we create system to bind a particular project to a particular auth DB, we can stick to the singleton model.

luipir commented 7 years ago

@elpaso, AuthManager does not only manage sqlite queries but also internal state and variables... here we can have concurrence problems. Btw I agree that singleton still continue to work in qgis server context. The context premise is where qgis server does not evolve with a web managing interface with multiple managers at the same time. I'm not used in QGIS server, but IMHO A common use case is where qgis desktop is used to manage configuration for the server => single manager. Am I wrong?

dakcarto commented 7 years ago

@m-kuhn wrote:

I wasn't quite sure how Credentials and AuthManager work exactly, my proposal is likely wrong.

They only communicate via a signal slot connection at the instance level between singletons. QgisApp is the parent of QgsCredentialDialog::instance. For Server, the master password and database are defined via env variables, so no QgsCredential::instance (or any of its subclasses) is used.

Question: is there always just one QgsAuthManager? Or can there be multiple different ones, for example in a shared server scenario?

The design is for only one, as it is app-relative. Shared server instances or concurrency was never designed for.

I'm looking into a QEP and API refactoring for the auth system, so would like to figure out the best design approach going forward. Can you explain further what you envision a 'shared server scenario' as? Currently, the SQLite qgis-auth.db is app- not project-relative. So, unless multiple projects are served simultaneously by the same QGIS Server FCGI spawned instance, I don't see the issue.

m-kuhn commented 7 years ago

Can you explain further what you envision a 'shared server scenario' as?

No vision at all, maybe the server team does have one? I'm happy to keep one per process (and not project) if that is fine for server.

haubourg commented 7 years ago

@pblottiere @rldhont Concerning the last singletons listed here, what is the current status in most recent code ?

QgisApp (application specific "singleton") QgsProject QgsMapLayerRegistry QgsAuthManager / QgsCredentials (I guess unlocking should not be controlled globally? Is there an internal dependency between them?) Server QgsConfigCache

Thanks for helping in cleaning up here !

nyalldawson commented 7 years ago

Here's the current list

App - should become members of QgisApp::instance():

Auth:

Core:

Gui:

Server:

The 4 GUI singletons are handled by https://github.com/qgis/QGIS/pull/4514

nyalldawson commented 7 years ago

No more GUI singletons -

Can be removed from the list

elpaso commented 6 years ago

QgsAuthManager can be removed from the list

nyalldawson commented 6 years ago

https://github.com/qgis/QGIS/pull/5535 removes QgsCoordinateTransformCache singleton