[Proposal] Change ObservationManager behaviour with CancelableEvents

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Proposal] Change ObservationManager behaviour with CancelableEvents

Simon Urli
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
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Thomas Mortagne
Administrator
+1 to stopping event propagation when it's cancelled
On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <[hidden email]> 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
> [hidden email]
> More about us at http://www.xwiki.com



--
Thomas Mortagne
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Simon Urli
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 <[hidden email]> 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
>> [hidden email]
>> More about us at http://www.xwiki.com
>
>
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Guillaume Delhumeau
In reply to this post by Thomas Mortagne
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 <[hidden email]> a
écrit :

> +1 to stopping event propagation when it's cancelled
> On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <[hidden email]> 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
> > [hidden email]
> > More about us at http://www.xwiki.com
>
>
>
> --
> Thomas Mortagne
>


--
Guillaume Delhumeau ([hidden email])
Research & Development Engineer at XWiki SAS
Committer on the XWiki.org project
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Simon Urli


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 <[hidden email]> a
> écrit :
>
>> +1 to stopping event propagation when it's cancelled
>> On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <[hidden email]> 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
>>> [hidden email]
>>> More about us at http://www.xwiki.com
>>
>>
>>
>> --
>> Thomas Mortagne
>>
>
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Guillaume Delhumeau
Le mer. 17 oct. 2018 à 10:47, Simon Urli <[hidden email]> 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 <[hidden email]>
> a
> > écrit :
> >
> >> +1 to stopping event propagation when it's cancelled
> >> On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <[hidden email]>
> 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
> >>> [hidden email]
> >>> More about us at http://www.xwiki.com
> >>
> >>
> >>
> >> --
> >> Thomas Mortagne
> >>
> >
> >
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com
>


--
Guillaume Delhumeau ([hidden email])
Research & Development Engineer at XWiki SAS
Committer on the XWiki.org project
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Simon Urli


On 10/17/18 11:29 AM, Guillaume Delhumeau wrote:

> Le mer. 17 oct. 2018 à 10:47, Simon Urli <[hidden email]
> <mailto:[hidden email]>> 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
>     <[hidden email] <mailto:[hidden email]>> a
>      > écrit :
>      >
>      >> +1 to stopping event propagation when it's cancelled
>      >> On Tue, Oct 16, 2018 at 6:07 PM Simon Urli <[hidden email]
>     <mailto:[hidden email]>> 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
>      >>> [hidden email] <mailto:[hidden email]>
>      >>> More about us at http://www.xwiki.com
>      >>
>      >>
>      >>
>      >> --
>      >> Thomas Mortagne
>      >>
>      >
>      >
>
>     --
>     Simon Urli
>     Software Engineer at XWiki SAS
>     [hidden email] <mailto:[hidden email]>
>     More about us at http://www.xwiki.com
>
>
>
> --
> Guillaume Delhumeau ([hidden email]
> <mailto:[hidden email]>)
> Research & Development Engineer at XWiki SAS
> Committer on the XWiki.org project

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Marius Dumitru Florea
In reply to this post by Simon Urli
-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 <[hidden email]> 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
> [hidden email]
> More about us at http://www.xwiki.com
>
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Change ObservationManager behaviour with CancelableEvents

Marius Dumitru Florea
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 <
[hidden email]> 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 <[hidden email]> 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
>> [hidden email]
>> More about us at http://www.xwiki.com
>>
>