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
[FEATURE] layer refresh and trigger actions on provider notification #5179
Conversation
@haubourg can you try this out please ? |
src/core/qgsvectordataprovider.h
Outdated
* | ||
* \since QGIS 3.0 | ||
*/ | ||
void notify( QString msg ) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't msg
be const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's "pass by value". But I'm not sure what is the best for signals since it's more a return value than anything else, the signal interface is misleading IMO.
Apparently Qt does some magic such that a "const &" is actually transformed into a "pass by value" https://stackoverflow.com/questions/8455887/stack-object-qt-signal-and-parameter-as-reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be const ref, even in signals.
The Qt magic related to old style connections - but we don't use them anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Done.
Tested now, I like it :) |
Nice feature, I didn't know that! |
@vmora Is there a reason to reduce action to only refresh instead of registered layer actions? |
btw useful feature :) |
@vmora have you planned to add tests for this new feature? |
Nope, it was just the first thing to do that came to mind. There is no interface yet to connect the signal to an action. But that's a good idea, both simple and powerful.
Of course it's on my todo list, should be simple enough based on an existing postgres test. |
great, tnx |
Interesting feature. It would be interesting to extend this feature to QGIS server. There are push notifications (https://developers.google.com/web/updates/2015/03/push-notifications-on-the-open-web) that allow a registered clients to receive data from the server on the servers demand. That way QGIS server could tell a web client that new data is available and that the client should reload the map image. Would be interesting for dynamic data ... Just thinking out loud. Not asking for this to be implemented, because I don't have such dynamic data anyway ... but may be a useful improvement for others who have such data. |
@andreasneumann interesting use case. In a web server context that would be somewhat different. The web client is not directly discussing with postgres in a transaction. So we would need the client to be able to receive "push" notifications (That is what web sockets do, if I get it well). |
src/core/qgsvectordataprovider.h
Outdated
@@ -533,6 +543,13 @@ class CORE_EXPORT QgsVectorDataProvider : public QgsDataProvider, public QgsFeat | |||
*/ | |||
void raiseError( const QString &msg ) const; | |||
|
|||
/** | |||
* Notification coming from provider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Emitted when notification..."
src/core/qgsvectordataprovider.h
Outdated
@@ -524,6 +524,16 @@ class CORE_EXPORT QgsVectorDataProvider : public QgsDataProvider, public QgsFeat | |||
*/ | |||
virtual bool hasMetadata() const { return true; }; | |||
|
|||
/** | |||
* Listen to provider notification and transforms them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dox should mention explicitly when something is a setter, e.g. "Sets whether the provider will listen to provider notifications..."
src/core/qgsvectorlayer.cpp
Outdated
@@ -147,6 +147,7 @@ QgsVectorLayer::QgsVectorLayer( const QString &vectorLayerPath, | |||
, mSymbolFeatureCounted( false ) | |||
, mEditCommandActive( false ) | |||
, mReadExtentFromXml( readExtentFromXml ) | |||
, mIsRefreshOnNofifyEnabled( false ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this initialization to the declaration in the header (c++11 style)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind, should I also move all other flags the same way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if you feel like it! There's a clang tidy tool I was trying to use to do all this automatically, but it refused to work at all for me. Manually doing it is the next best thing ;)
src/core/qgsvectorlayer.cpp
Outdated
if ( !dataProvider() ) | ||
return; | ||
|
||
if ( enabled and not isRefreshOnNotifyEnabled() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use c++ operators && ! (don't use the and
and not
keywords) (also in the else condition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, ther is too much python in my life.
src/core/qgsvectorlayer.cpp
Outdated
|
||
void QgsVectorLayer::onNotified( const QString &message ) | ||
{ | ||
if ( refreshOnNotifyMessage().isEmpty() or refreshOnNotifyMessage() == message ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'or' -> '||'
src/core/qgsvectorlayer.h
Outdated
* | ||
* \since QGIS 3.0 | ||
*/ | ||
void setRefreshOnNotifyEnabled( bool enabled ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering - should these changes be moved from QgsVectorLayer to QgsMapLayer? Potentially a raster data provider could also have some form of notification signal (such as when the file changes on disk).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. BTW, if not too expensive, the "watch file" could be enabled by default.
void notify( QString message ); | ||
|
||
private: | ||
volatile bool mStop; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize in the header, not source
src/core/qgsvectorlayer.cpp
Outdated
void QgsVectorLayer::onNotified( const QString &message ) | ||
{ | ||
if ( refreshOnNotifyMessage().isEmpty() or refreshOnNotifyMessage() == message ) | ||
triggerRepaint(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that emit a dataChanged()
signal so other parts than only the canvas can also benefit from the signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the same should be done with the mRefreshTimer. What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly... Although not sure since the refresh timer doesn't actually check for any changes, I'm not sure I want to attribute table reloaded every 5 seconds "just because".
Nice work @vmora. @andreasneumann The webserver use case certainly sounds interesting. The main thing to do there is to have some sort of session management where user(s) connect to a project which is kept alive (not only for a single request but for the duration of the session). I guess this is a bigger topic in which the whole server team should be involved (and which could magically resolve all the caching issues with which we had troubles in the past ;) ). |
@m-kuhn to me the notification is something coming from outside, but I don't think I understand what you have in mind. |
Which ones have been caused by an update, insert or delete triggered by our local QGIS instance. And which ones by "the outside world™" |
To me, outside transaction groups, each layers lives in its own world. Maybe you can achieve something with the postgres connection process id |
@luipir for your idea of connecting the notification to an action: the interface with the two checkboxes plus the message is a bit clumsy to add in the actions list. Moreover, for actions, I think it's a bit dangerous to react on any notification (e.g. if action triggers a notification, we have a nice infinite loop). What I'd go for is an additional column for actions with a string to filter messages (could be a regex) if there is a specified string, we enable listening and trigger action if the notification matches. What do you think ? |
Reading your statement I realize that the main use-case you have in mind is probably side-effects caused by triggers etc - and not concurrent editing by different users. That's actually another very interesting scenario, but I guess there it's even harder to know which of those changes are already known to our own qgis instance (because it's a result of directly editing and saving a layer) and which ones are side-effects. So in the end it's up to the database side to emit notifications for the right layers at the right time? |
Well, for me, a different user or a different layer (no transaction group) is the same thing a far a the database is concerned: it's a different connection (or am I mistaken?) The refresh is the first use case for the notification, so side-effects (triggers) are somewhat related, but also insertion by sensors connected to the database, or updates by another users... But the notification mechanism has a lot more to offer. As you say "in the end it's up to the database side to emit notifications for the right layers at the right time". |
@vmora I really like the implementation and I agree with your proposal to avoid infinite loop. Speculating more (and probably outside the scope of this PR), probably we need a base class able to implement a generic Publisher/Subscriber cross network protocol, like ZeroMQ (https://github.com/HBPVIS/ZeroEQ). I use PubSub python lib when I've to add async behaviour in functions that does not use QObject/event infrastrucure. |
Oh nice!
…On Wed, Sep 13, 2017 at 9:18 PM, Luigi Pirelli ***@***.***> wrote:
@vmora <https://github.com/vmora> I really like the implementation and I
agree with your proposal to avoid infinite loop.
Speculating more (and probably outside the scope of this PR), probably we
need a base class able to implement a generic Publisher/Subscriber cross
network protocol, like ZeroMQ (https://github.com/HBPVIS/ZeroEQ). I use
PubSub python lib when I've to add async behaviour in functions that does
not use QObject/event infrastrucure.
There are many possible usecases and one is the telemetry plugin or an
application to monitor underwater or flying UAV telemetry.
Just a note: https://github.com/SpiderOak/skeeter is an example to bridge
PG listen/notify event to ZeroMQ. The advantage is that we can aslo publish
events listened by the DB.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5179 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3DZ9flKh1F6g74K9Adqq1L7t3ue5ks5sh7oGgaJpZM4PUbzP>
.
|
The question I wanted to ask is, if we can avoid cache invalidation (which happens unconditionally if |
OK, got it now. Does the triggerRepaint invalidate cache ? Because I kinda copied the behavior from the timer refresh (triggerRepaint, not dataChanged as pointed out by @nyalldawson). With the connection on actions I'm about to add, you can do whatever you want with the signal, does this address your concerns ? |
What do you think how should be dealt with
I think there's a general need for a signal that says "hey, something on the layer changed but we don't have enough information to tell you exactly what, if you want to stay on the safe side reload whatever you have" (that's what the I can't offer a complete solution, I guess it's just not a trivial topic. Just bringing a braindump to the table here. |
What could be done is to add a map with extended information to the
Other information groups like these may be better. However, I think some kind of "hints" about what (potentially could have) changed will be useful for big projects with many users working in parallel to allow slots to filter what they are interested in. |
src/core/qgsdataprovider.h
Outdated
* | ||
* \since QGIS 3.0 | ||
*/ | ||
virtual void setListening( bool isListening ) { Q_UNUSED( isListening ); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General though: it's better to keep such things in the cpp file. Less code in the header makes compiling faster and virtual methods cannot be inlined anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to argue (https://stackoverflow.com/questions/733737/are-inline-virtual-functions-really-a-non-sense)... just for the fun of arguing ;)
@m-kuhn to be on the safe side, we would be better of with a dataChanged signal in this case (same holds for timer refresh I guess). We could also have a switch to chose which signal is to be emitted... but that is not user-friendly IMO Custom signaling could also be achieved in the layer actions. I like the added checkboxes in the rendering tab, because it advertises the notification from provider feature. The "actions on notify" will be more flexible, bit less discoverable, so it's nice to have both options. Concerning the granularity of "dataChanged"... it's a good idea, but beyond the scope of what I'm trying to do here. |
@luipir you have the GUI part for "notification triggered action" in the last commit. Here is what it looks like If someone can give me an OK for the addition of notification message to QgsAction, I'll finish implementation. |
great @vmora, but if actions can be triggered, it still make sense to have a refresh config parameter instead of a standard repaint/refresh action? I really like the fine granularity that can be managed creating specific topics... e.g. an action can listen a specific record field modification if a specific NOTFY topic is set. |
@luipir with the latest commits the "not only need to match exactly but match some regexp" is not satisfied, but I think the current activation with "not only need to match exactly but begins with" is easier to use and sufficient to filter notifications. @3nids do you agree ? @m-kuhn if @luipir and @3nids reeeeeeeealy want regex, I'd love an answer to the "which regex engine" question. If they are ok with the current "begins with", then all is well. about size limitation: that was just a remark related to sending something else after the "header", the something_else must satisfy |
@m-kuhn, as mentioned earlier, the refresh layer on notification (triggerRepaint) is now here to "advertise" the "trigger action on notification" feature. With the later, you can choose whatever cache you want invalidated as long as the invalidation as a python API. |
For business logic, yes. |
Nooooo this one: http://doc.qt.io/qt-5/qregularexpression.html Let's save ourselves SOME work when Qt upstream drops the next round of classes they dislike! |
Of course, who proposed the other one? 😜 |
and also
@m-kuhn Sorry, I don't get what you want me to do here on that topic. I do feel stupid for that. Please elaborate.
OK, thanks @nyalldawson. Waiting for @3nids and @luipir to confirm that they reeeeeeally want regex there. Should be a piece of cake, but I don't feels it's necessary here. |
and
I just want to be clear on what the notification mechanism does: the postgres NOTIFY command syntax is I you wonder "Why not specifying/filtering on the channel too?": it's to keep the provider interface "minimal". A file-based provider can issue a notification message "file changed" but there is no channel involved there, and the concept is not necessary IMO. If someone out there has a database running that already "NOTIFY" on another channel and wants those signals listened to, it's doable, but that's a lot of changes and should really be worth the effort. |
@vmora yes I suppose it's necessary only to change here: |
@luipir as opposed to the comment I just made about "channel" selection, I know it's a trivial change to take regex into account. My only concern was the clarity of the interface and the actual need for for this flexibility. |
@vmora ok, no problem to leave it simple... btw I don't know how much is more complex expressing: |
Beware ! Awesome regex included. But watch out for regex behavior since |
hum, @vmora We need to add the tag [needs-doc]to the PR. Can you write one or two examples of possible payloads more explicit to illustrate |
@haubourg I will rebase/squash and improve the commit message. |
a6e071a
to
d0771d2
Compare
all done, I think the commit message is explicit enough for documentation now |
@nyalldawson, @m-kuhn there is one failing test (PyQgsLocator timeout) that doesn't fail on my machine. |
d0771d2
to
b5a2c69
Compare
Last commit is a useless rebase on master to trigger rebuild. |
I wonder if we just need to send (an initially empty) struct/class parameter that could contain some flags for internal usage containing more details about what changed to determine if some caches should be invalidated. E.g. a Does that make any sense to you? I'm not completely convinced myself, but I think it's an important question to consider before the 3.0 API is frozen. (CC @wonder-sk @nyalldawson). |
Not sure yet, sorry.
To who, from who ? Right know, you can There is no class/struct involved there, nor is the message standardized in any way. Now, once you receive the notification in qgis, you could have a special handler (could be the provider "listener" itself) that understand a "standard" syntax and takes "standard" actions accordingly. Then we may require some additional slots to invalidate some cache an not some other. So if I understand you: this PR opens the possibility for a datasource to notify qgis with only one kind of notifications (message containing anything) and you'd like more granularity, like (e.g. attributeValuesChanged or geometryChanged) ? |
I was thinking about the internal signalling (something like How the information is put into this structure is yet a different question. |
[needs-docs] In vector layer properties (only usefull for postgres datasources) **in the rendering tab** A "Refresh layer on notification" checkbox has been added to refresh layer on provider notification. For a postgres datasource, if a `NOTIFY qgis;` command is issued by one of the database clients, a refresh of the layer will occur. If the "Only if message is" checkbox is checked, the notification will trigger the refresh only if the message contend is the one specified, e.g. if the user enters "refresh" in the box right next to the "Only if message is" checkbox, then a `NOTIFY qgis, 'refresh';` command in the datatabase will trigger a layer refresh, but `NOTIFY qgis;` or `NOTIFY qgis, 'something else';` won't. **in the actions tab** A column "On notification" has been added, the action editor widget a has text field "Execute if notification message matches" to specify a filter for notification from the provider. The filter is a Perl-type regex. Note that, as opposed to the "layer refresh" that Exemple: - QGIS side "Execute if notification message matches" `^trigger my action` - Postgres side: `NOTIFY qgis, 'trigger my action'` will trigger the action - Postgres side: `NOTIFY qgis, 'trigger my action some additional data'` will trigger the action - Postgres side: `NOTIFY qgis, 'do not trigger my action some additional data'` will NOT trigger the action Please note that if the `^`, which means "starts with", in `^trigger my action` had been ommited, the last notification would have triggered the action because the notification message contains the `trigger my action` A new qgis variable `notification_message` is available for use in actions, it holds the contend of the notification message. To continue with the previous exemple, if the action is of python type with the code: ```python print('[% @notification_message %]') ``` The three notifictions above will result in two printed lines ``` trigger my action trigger my action some additional data ``` User Warning: For postgres providers, if the "Refresh layer on notification" is checked, or if one layer action has "On notification" specified, a new connection to the database is made to listen to postgres notifications. This olds even if transaction groups are enabled at the project level. Note that once the notification mechanism is started in a QGIS session, it will not stop, even if there is no more need for it (Refresh layer on notification" unchecked and no "On notification" in any action). Consequently the connection listening to notification will remain open. IMPLEMENTATION DETAILS: A notify signal has been added to the abstract QgsVectorDataProvider along with a setListening function that enables/disble the notification mechanism. For the moment only the postgres provider implements the notification. QgsAction has a notificationMessage member function that holds the regex to match to trigger action QgsActionManager becomes a QObject and is doing the filtering and execute actions on notifications. The notification notion extends beyond SRGBD servers (postgres and oracle at least have the notify) and the "watch file" in the delimitedtext provider could also benefit from this interface. For the postgres provider a thread is created with a second connection to the database. This thread is responsible for listening postgres notifications. It would be nice to avoid the creation of one listening chanel per provider in the case transaction groups are enabled. Please note that when listening starts (a thread and connection is created in the postgres provider) it cannot be stopped by removing the connected actions or unchecking the refresh check box. Indeed, since we don't know who needs the signals, we dont't want to stop the service. The service will not restart in the next qgis session though. If this behavior is not deemed appropriate, we could use ``` int QObject::receivers ( const char * signal ) const ``` and have QgsDataProvider::setListening return a bool to tell the caller if the signal has actually been closed.
b5a2c69
to
02e3916
Compare
@m-kuhn can the PR be accepted or are you waiting for something ? If the former, can you be specific ? If the later, can @nyalldawson or yourself merge that please ? Resolving .ui conflicts is not my cup of tea. |
Let's just merge this for testing soon. It's an awesome feature which I really like to see land! |
Hi, looks awesome. I guess we could have applications such as live monitoring of movements (bus; trains; trucks;safety vehicles...) such as in https://traintimes.org.uk/map/london-buses/#73 |
@mayeulk yes exactly! You could already use a timer to refresh the canvas, but now, you can trigger the refresh only when needed, from a database perspective. |
[needs-docs]
In vector layer properties, in the rendering tab, a
"Refresh layer on notification" checkbox has been added to refresh layer
on provider notification.
For a postgres datasource, if a "NOTIFY qgis;" command is issued by one of the database clients,
a refresh of the layer will occur.
If the "Only if message is" checkbox is checked and the notification will trigger the refresh only
if the message contend is the one specified, e.g. if the user enters
"refresh" in the box right next to the "Only if message is" checkbox,
then a "NOTIFY qgis, 'refresh';" command in the datatabase will trigger
a layer refresh, but "NOTIFY qgis;" or "NOTIFY qgis, 'something else';"
won't.
IMPLEMENTATION DETAILS:
A notify signal has been added to the abstract QgsVectorDataProvider
along with a setListening function that enables/disble the notification
mechanism.
For the moment, this notification is only used to refresh/redraw a vector
layer.
For the moment only the postgres provider implements the notification.
The notification notion extends beyond SRGBD servers (postgres and oracle at
least have the notify) and the "watch file" in the delimitedtext
provider could also benefit from this interface.
For the postgres provider a thread is created with a second connection
to the database. This thread is responsible for listening postgres
notifications.
It would be nice to avoid the creation of one listening chanel per
provider in the case transaction groups are enabled.