Move parts of QgsLocatorFilter to core

[3nids]

To create a locator item in QML without duplicating much of the code, part of QgsLocatorWidget was moved to core.
This PR also creates a QgsMapCanvasInterface to access core component of map canvas (namely map settings).

Sadly, I could not use multiple inheritance (you cannot inherit multiple QObject) since the core part needs some signal connections. Therefore, QgsLocatorWidget has a QgsLocatorWidgetCore member.

[3nids]

@nyalldawson I have succesfully tested this change both in QGIS core and QField. Do you want to review before I merge?

[nyalldawson]

Yes I'd like to review please

[nyalldawson]

I don't like the naming of this class -- it's dangerously bleeding gui specific classes into core. What about something more generic like "QgsMapDisplayInterface" or "QgsRenderedMapInterface"? QgsLayoutItemMap could implement this interface too.

[nyalldawson]

I also don't like the naming of this class. If it's widget specific it doesn't belong in core. How about "QgsLocatorModelBridge" ? (really should be QgsLocatorLocatorModelBridge, but that's horrible).

[3nids]

Perfect for me

[nyalldawson]

Add note about ownership/lifetime

[nyalldawson]

Better dox here -- is this blocking? If not, what happens to existing searches? Does it block until they are canceled?

[nyalldawson]

Does this need to be public?

[3nids]

This is used in the widget. If I could inherit not. Or I make a friend class?

[wonder-sk]

I am not sold on this approach and I wonder if we really need QgsMapCanvasInterface?

Alternatives:

• only keep pointer to QgsMapSettings from canvas - it is always the same object over map canvas lifetime
• rather than pulling map settings from an interface, why not push required bits of map settings when they change?

[3nids]

@wonder-sk having a pointer and returning a const pointer would be perfect. But is that an API break? Or we make a mapSettingsV2? If the approach is fine for @nyalldawson my vote is for this one

@nyalldawson thanks for the review and the naming proposals!

[wonder-sk]

having a pointer and returning a const pointer would be perfect. But is that an API break? Or we make a mapSettingsV2?

Maybe we mean different things... What I meant in the first alternative was really just to have setMapSettings( QgsMapSettings* mapSettings ) in the locator core class instead of setMapCanvasInterface(...). Then the locator would be initialized with mWidgetCore->setMapSettings( &canvas->mapSettings() ) ... no changes in QgsMapCanvas.

[PeterPetrik]

If the only reason we need new map canvas interface is to be able to access MapSettings, I will be more happy with the approach Martin suggested. That is what we do in almost all classes in QgsQuick, e.g.

class QUICK_EXPORT QgsQuickFeatureHighlight : public QQuickItem
{
...
    Q_PROPERTY( QgsQuickMapSettings *mapSettings MEMBER mMapSettings NOTIFY mapSettingsChanged )

or

class QUICK_EXPORT QgsQuickIdentifyKit : public QObject
{
    ...
    Q_PROPERTY( QgsQuickMapSettings *mapSettings READ mapSettings WRITE setMapSettings NOTIFY mapSettingsChanged )

[m-kuhn]

One of the tricky things here is the identity vs value disucssion. So far QgsMapSettings is treated as value, so it is copied around. Starting to pass around long living pointers to QgsMapSettings around adds some complexity to the current approach.

Some things to keep in mind:

• we now also have multiple map canvases, they can die before the app exits, and we need to make sure that a map settings pointer does not survive the lifetime of the underlying map canvas
• The approach to have this kind of property Q_PROPERTY( QgsQuickMapSettings *mapSettings MEMBER mMapSettings NOTIFY mapSettingsChanged ) looks dangerous too
  The pointer itself will not change (only the contents), to make it work, no change guard can be added (if ( mMapSettings == mapSettings ) return; and therefore we run the risk of binding loops.
  Normally pointers are only used as properties for QObject based classes (identities)
• It would be nice to be able to proxy change notifications of map settings through core (foremost but not only extentsChanged) which is currently done via the canvas.

[3nids]

@wonder-sk From the discussion, the interface seems the safest and most flexible approach. Why are you not sold to it? 💰?

[wonder-sk]

The interface is not really that flexible in my opinion. It can't have signals/slots in the future because it cannot be derived from QObject because QgsMapCanvas already derives from QObject. Plus generally I try to avoid inheritance, especially multiple inheritance because it often fixes API design too much (and IIRC the SIP casting bugs are related to multiple inheritance too).

I think @m-kuhn has good point with identity vs value - which one do we want here? If we need identity (i.e. QObject-based) with signals, then we should probably take QgsQuickMapSettings and put it into qgis_core (and rename it to something like QgsMapSettingsController) because that is essentially it. And QgsMapCanvas would just forward signals from it.

In my opinion the safest is the second alternative: "rather than pulling map settings from an interface, why not push required bits of map settings when they change"

[m-kuhn]

It can't have signals/slots in the future because it cannot be derived from QObject because QgsMapCanvas already derives from QObject.

Looks like it can, but it's relying on old style signals. I'm not sure we really want that https://stackoverflow.com/a/18113601/2319028 but I also can't think of a modern approach. If we are ok with that it would be my preferred approach. If...

Plus generally I try to avoid inheritance, especially multiple inheritance because it often fixes API design too much (and IIRC the SIP casting bugs are related to multiple inheritance too).

That statement sounds a bit too general to me ;) While certainly often abused I really like multiple inheritance when it's done in a (java) interface style with only virtual methods but no member variables.

I think @m-kuhn has good point with identity vs value - which one do we want here? If we need identity (i.e. QObject-based) with signals, then we should probably take QgsQuickMapSettings and put it into qgis_core (and rename it to something like QgsMapSettingsController) because that is essentially it. And QgsMapCanvas would just forward signals from it.

In my opinion the safest is the second alternative: "rather than pulling map settings from an interface, why not push required bits of map settings when they change"

They both work for me too. The latter one has a risk of a connection-orgy for cases where a lot of "required bits" are needed.
The first one sounds like something I had in mind since writing this class, so if there's a consensus to go this way, no objections.

[m-kuhn]

Thinking about it again, the proposal to "rather than pulling map settings from an interface, why not push required bits of map settings when they change" sounds actually good. It's just crs and extents that are under discussion from what I can see. the QgsMapSettingsController can still be implemented in the future.

[3nids]

this is ready for next round of reviews...
no more map canvas interface or controller.

[wonder-sk]

for me it is +1 now (a.k.a. I'm sold! 💰) but I haven't really looked into the locator stuff... thanks a lot for updating the PR!

[3nids]

@nyalldawson do you want to look at the locator part again or you're happy with this?

[nyalldawson]

I haven't looked yet sorry, give me 24 hours.

[nyalldawson]

Looks good to me