[xwiki-devs] [Proposal] Change ObservationManager behaviour with CancelableEvents
Hi everyone, the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered. I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener. My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user. Do you know if the current behaviour is required at some places? Else do you agree on changing it? Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
+1 to stopping event propagation when it's cancelled On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Thomas Mortagne
Hi all, I took the liberty to already create the PR, then you can see the impact on the code: https://github.com/xwiki/xwiki-commons/pull/49 Simon. On 10/17/18 9:09 AM, Thomas Mortagne wrote:
+1 to stopping event propagation when it's cancelled On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
I'm OK. <OFF TOPIC> I'm just thinking about an other particular case: Imagine you have 3 event listeners (A, B, C): - A receives the event and perform some actions (saving something in the database). - B receives the event and cancels it - C don't receive the event because it had been canceled However, we may want to resend some infos to listener A so it can rollback its actions (otherwise we end up with bad info in the database). Do we have a strategy for this? </OFF TOPIC> Le mer. 17 oct. 2018 à 09:09, Thomas Mortagne <thomas.mortagne@xwiki.com> a écrit :
+1 to stopping event propagation when it's cancelled On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Thomas Mortagne
-- Guillaume Delhumeau (guillaume.delhumeau@xwiki.com) Research & Development Engineer at XWiki SAS Committer on the XWiki.org project
On 10/17/18 10:35 AM, Guillaume Delhumeau wrote:
I'm OK.
<OFF TOPIC> I'm just thinking about an other particular case: Imagine you have 3 event listeners (A, B, C): - A receives the event and perform some actions (saving something in the database). - B receives the event and cancels it - C don't receive the event because it had been canceled
However, we may want to resend some infos to listener A so it can rollback its actions (otherwise we end up with bad info in the database).
Do we have a strategy for this?
I don't think we have a strategy for that, but we might add a new method in EventListener: onRollback(CancelableEvent canceledEvent, Object source, Object data) and store somewhere the list of called listener to be able to call their rollback method if the event has been cancelled. Should do the trick, WDYT?
</OFF TOPIC>
Le mer. 17 oct. 2018 à 09:09, Thomas Mortagne <thomas.mortagne@xwiki.com> a écrit :
+1 to stopping event propagation when it's cancelled On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Thomas Mortagne
-- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
Le mer. 17 oct. 2018 à 10:47, Simon Urli <simon.urli@xwiki.com> a écrit :
On 10/17/18 10:35 AM, Guillaume Delhumeau wrote:
I'm OK.
<OFF TOPIC> I'm just thinking about an other particular case: Imagine you have 3 event listeners (A, B, C): - A receives the event and perform some actions (saving something in the database). - B receives the event and cancels it - C don't receive the event because it had been canceled
However, we may want to resend some infos to listener A so it can rollback its actions (otherwise we end up with bad info in the database).
Do we have a strategy for this?
I don't think we have a strategy for that, but we might add a new method in EventListener:
onRollback(CancelableEvent canceledEvent, Object source, Object data)
and store somewhere the list of called listener to be able to call their rollback method if the event has been cancelled. Should do the trick, WDYT?
Sounds like a good idea, indeed. Or it could be called onCanceled() instead of onRollback, since the rollback is what we expect, not the event that is triggered.
</OFF TOPIC>
Le mer. 17 oct. 2018 à 09:09, Thomas Mortagne <thomas.mortagne@xwiki.com> a écrit :
+1 to stopping event propagation when it's cancelled On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event
wouldn't
match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Thomas Mortagne
-- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-- Guillaume Delhumeau (guillaume.delhumeau@xwiki.com) Research & Development Engineer at XWiki SAS Committer on the XWiki.org project
On 10/17/18 11:29 AM, Guillaume Delhumeau wrote:
Le mer. 17 oct. 2018 à 10:47, Simon Urli <simon.urli@xwiki.com <mailto:simon.urli@xwiki.com>> a écrit :
On 10/17/18 10:35 AM, Guillaume Delhumeau wrote: > I'm OK. > > <OFF TOPIC> > I'm just thinking about an other particular case: > Imagine you have 3 event listeners (A, B, C): > - A receives the event and perform some actions (saving something in the > database). > - B receives the event and cancels it > - C don't receive the event because it had been canceled > > However, we may want to resend some infos to listener A so it can rollback > its actions (otherwise we end up with bad info in the database). > > Do we have a strategy for this?
I don't think we have a strategy for that, but we might add a new method in EventListener:
onRollback(CancelableEvent canceledEvent, Object source, Object data)
and store somewhere the list of called listener to be able to call their rollback method if the event has been cancelled. Should do the trick, WDYT?
Sounds like a good idea, indeed. Or it could be called onCanceled() instead of onRollback, since the rollback is what we expect, not the event that is triggered.
I'll create an issue in that way.
> </OFF TOPIC> >
Now Thomas just opens the debate about where to put the behaviour change: I proposed in my PR to put it in AbstractCancelableEvents to allow users redefine the behaviour if needed by overriding the method. Thomas proposes (https://github.com/xwiki/xwiki-commons/pull/49#issuecomment-430555665) that we put it in DefaultObservationManager as we want the behaviour for all CancelableEvents, not only those who inherits from AbstractCancelableEvents. My concern here is that this behaviour couldn't be changed easily if needed (unless using another kind of event). WDYT?
> Le mer. 17 oct. 2018 à 09:09, Thomas Mortagne <thomas.mortagne@xwiki.com <mailto:thomas.mortagne@xwiki.com>> a > écrit : > >> +1 to stopping event propagation when it's cancelled >> On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <simon.urli@xwiki.com <mailto:simon.urli@xwiki.com>> wrote: >>> >>> Hi everyone, >>> >>> the current behaviour of the ObservationManager is to always triggers >>> the listeners if it matches the events. >>> Now regarding the CancelableEvents, the match is only done on the type >>> of the event and some given filter rules, but never with its cancel >>> status: if an event is cancelled, the matching listeners are always >>> triggered. >>> >>> I propose to change that behaviour, to trigger listeners only if the >>> CancelableEvents are not canceled: basically, a cancelled event wouldn't >>> match any listener. >>> >>> My primary reason for wanting that change is that the current behaviour >>> led to a bad UX: if an event triggers multiple questions, no matter if >>> one is cancelled, all questions will be asked to the user. >>> >>> Do you know if the current behaviour is required at some places? >>> Else do you agree on changing it? >>> >>> Simon >>> -- >>> Simon Urli >>> Software Engineer at XWiki SAS >>> simon.urli@xwiki.com <mailto:simon.urli@xwiki.com> >>> More about us at http://www.xwiki.com >> >> >> >> -- >> Thomas Mortagne >> > >
-- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com <mailto:simon.urli@xwiki.com> More about us at http://www.xwiki.com
-- Guillaume Delhumeau (guillaume.delhumeau@xwiki.com <mailto:guillaume.delhumeau@xwiki.com>) Research & Development Engineer at XWiki SAS Committer on the XWiki.org project
-- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
-0. I would be OK to stop the event propagation if the list of event listeners were *ordered* but AFAIK there's no order between event listeners ATM. It should always be possible to catch an event, even if that event is going to be canceled by some other listener: * either by registering an event listener with higher priority than the one canceling the event * or by not stopping the event propagation and letting each listener decide what to do based on the canceled state of the event For instance, DOM events have this https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-flow-capture If the capturing EventListener
<https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener> wishes to prevent further processing of the event from occurring it may call the stopProgagation method of the Event <https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event> interface. This will prevent further dispatch of the event, although additional EventListeners <https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener> registered at the same hierarchy level will still receive the event.
So you can always catch the DOM event by registering the event listener on the element that triggers the event (rather than on one of its parents). Thanks, Marius On Tue, Oct 16, 2018 at 7:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
An use case is to be able to log all the events that happen in XWiki. It would be stupid to not be able to log an event because there's another event listener that cancels the event before. On Wed, Oct 17, 2018 at 3:21 PM Marius Dumitru Florea < mariusdumitru.florea@xwiki.com> wrote:
-0. I would be OK to stop the event propagation if the list of event listeners were *ordered* but AFAIK there's no order between event listeners ATM. It should always be possible to catch an event, even if that event is going to be canceled by some other listener:
* either by registering an event listener with higher priority than the one canceling the event * or by not stopping the event propagation and letting each listener decide what to do based on the canceled state of the event
For instance, DOM events have this https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-flow-capture
If the capturing EventListener
<https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener> wishes to prevent further processing of the event from occurring it may call the stopProgagation method of the Event <https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event> interface. This will prevent further dispatch of the event, although additional EventListeners <https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventListener> registered at the same hierarchy level will still receive the event.
So you can always catch the DOM event by registering the event listener on the element that triggers the event (rather than on one of its parents).
Thanks, Marius
On Tue, Oct 16, 2018 at 7:07 PM Simon Urli <simon.urli@xwiki.com> wrote:
Hi everyone,
the current behaviour of the ObservationManager is to always triggers the listeners if it matches the events. Now regarding the CancelableEvents, the match is only done on the type of the event and some given filter rules, but never with its cancel status: if an event is cancelled, the matching listeners are always triggered.
I propose to change that behaviour, to trigger listeners only if the CancelableEvents are not canceled: basically, a cancelled event wouldn't match any listener.
My primary reason for wanting that change is that the current behaviour led to a bad UX: if an event triggers multiple questions, no matter if one is cancelled, all questions will be asked to the user.
Do you know if the current behaviour is required at some places? Else do you agree on changing it?
Simon -- Simon Urli Software Engineer at XWiki SAS simon.urli@xwiki.com More about us at http://www.xwiki.com
participants (4)
-
Guillaume Delhumeau -
Marius Dumitru Florea -
Simon Urli -
Thomas Mortagne