FWD » Core Development » Base Language

Code Review Task Branch 2917a Revision 10956

1. Is it intended that EventList.remove() operate on multiple EventDefinition instances if their widget IDs or event IDs match? It seems to me that one might remove widget/resource IDs from non-intersecting definitions as well as intersecting ones (e.g. different events but same widget IDs).

2. EventList.remove() is package private, so it should be moved to below the public methods.

Greg Shah wrote:

Code Review Task Branch 2917a Revision 10956

1. Is it intended that EventList.remove() operate on multiple EventDefinition instances if their widget IDs or event IDs match? It seems to me that one might remove widget/resource IDs from non-intersecting definitions as well as intersecting ones (e.g. different events but same widget IDs).

On server-side, although the trigger ID is set at the EventDefinition, each EventList matches only one ON statement. Even if the same widget is set more than once for this trigger, it doesn't affect the ON ... REVERT: what it matters is if is the only trigger defined or not.

I'll change the EventList.removE(EventDefinition) to include the intersect check and return a boolean if something was removed or not (currently there is a javadoc stating that a EventList.intersect() check is required before invoking this).

2. EventList.remove() is package private, so it should be moved to below the public methods.

OK.

I've rebased 2917a from trunk rev 10956.

See revision 10959 for the issues in note 2.

Code Review Task Branch 2917a Revision 10959

The code looks good.

What standalone testcases have you checked?

Greg Shah wrote:

What standalone testcases have you checked?

I've tested with uast/revert_trigger/revert_trigger.p, but the existing revert1.p and revert2.p tests fail ... I missed to check the case when multiple events/widgets are defined for the same trigger. I'm working on a fix.

2917a rev 10960 fixes EventList.remove - use intersection/difference to remove an event list.

Code Review Task Branch 2917a Revision 10960

The changes are good. It is easier to read/understand and I like the reuse of our other code for handling the removal.

Please rebase and put it into runtime testing, if you are happy with the current state.

Greg Shah wrote:

Code Review Task Branch 2917a Revision 10960

The changes are good. It is easier to read/understand and I like the reuse of our other code for handling the removal.

Please rebase and put it into runtime testing, if you are happy with the current state.

2917a was rebased from trunk rev 10957 - new rev 10961.

Runtime testing also passed. I think is OK to merge either to 1881t or to trunk.

Please merge to trunk.

Greg Shah wrote:

Please merge to trunk.

2917a was merged to trunk rev 10958 and archived.