Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WICKET-7080] 1) make default events delivery machinery pluggable 2) … #682

Merged
merged 1 commit into from
Nov 1, 2023
Merged

[WICKET-7080] 1) make default events delivery machinery pluggable 2) … #682

merged 1 commit into from
Nov 1, 2023

Conversation

reiern70
Copy link
Contributor

@reiern70 reiern70 commented Oct 16, 2023 •
edited
Loading

  1. make default events delivery machinery plug-gable
  2. allow to disabled sending events
  3. defines a reflection based machinery to deliver events

@reiern70 reiern70 force-pushed the reiern70/WICKET-7080 branch from 6fd7ce9 to e0066f2 Compare October 16, 2023 17:25
@reiern70 reiern70 requested review from martin-g and bitstorm October 16, 2023 17:25
reiern70
reiern70 commented Oct 17, 2023
View reviewed changes
wicket-core/src/main/java/org/apache/wicket/event/ReflectionEventDispatcher.java Outdated
/**
* IEventDispatcher that uses reflection in order to locate events methods.
*/
public class ReflectionEventDispatcher implements IEventDispatcher {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add here a class to method map cache. As a similar project on wicket stuff have. One main difference is we don't have a component instantiation listener=> this will only happen at runtime when event is delivered.

reiern70
reiern70 commented Oct 17, 2023
View reviewed changes
wicket-core/src/main/java/org/apache/wicket/event/ReflectionEventDispatcher.java Outdated
* Probably, this should be set to true in most applications and enforce the use of either @{@link EventAwareObject}
* annotation or the marker interface @{@link IEventAwareObject}
*/
private final boolean restrictToEventAware;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag should make event's processing faster and only marked classes will be scanned.

@reiern70
Copy link
Contributor Author

This PR still needs some more work and care

@reiern70 reiern70 force-pushed the reiern70/WICKET-7080 branch 2 times, most recently from 44d559e to 3f2fb29 Compare October 17, 2023 20:44
@reiern70
Copy link
Contributor Author

I have added more tests and allow to use more that one eventType

@reiern70
Copy link
Contributor Author

Any volunteers to review this PR?

@martin-g
Copy link
Member

https://github.com/wicketstuff/core/tree/master/annotationeventdispatcher-parent

@reiern70
Copy link
Contributor Author

https://github.com/wicketstuff/core/tree/master/annotationeventdispatcher-parent

@martin-g I'm aware of that work. This PR is 2 things

  1. Make "default" event delivery plug-able. This means replacing it by something else (or disabling it)
  2. Some simple event annotation based events delivery. Where you can get protected or private or public methods injected with an event,
@OnEvent(XXX.class)
protected onXXX(IEvent<XXX>  event) {
}

This allows to still work with event and programmatically cancel it.

or

@OnEvent(XXX.class)
protected onXXX(XXX  payload) {
}

will both work. No restrictions on XXX are imposed.

IMHO, 1. would be nice to have in wicket. As for 2. I can understand can be implemented in many different ways.

  1. This approach does not require a component listener and only marked classes are scanned (by annotation or implementing an interface)

@reiern70
Copy link
Contributor Author

@martin-g My last commits adds caching

@reiern70 reiern70 force-pushed the reiern70/WICKET-7080 branch 2 times, most recently from eadd46b to 756ff28 Compare November 1, 2023 14:51
@reiern70
Copy link
Contributor Author

reiern70 commented Nov 1, 2023

Ok, I have reverted the part related to adding a reflection based machinery. What remains is:

  1. Allow to plug your own default "event handler"
  2. As a side effect it is possible to disable events delivery

martin-g
martin-g reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case with disabled default event dispatcher.

wicket-core/src/main/java/org/apache/wicket/settings/FrameworkSettings.java Outdated
@@ -45,8 +45,28 @@
*/
public class FrameworkSettings implements IEventDispatcher
{
/**
* Does the standard delivery of events. Override and do nothing if you want to disable it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is private it is not possible to override it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
* Does the standard delivery of events. Events can be disabled at application level by setting a null
* {@link IEventDispatcher} via {@link #setDefaultEventDispatcher(IEventDispatcher)} and setting no additional
* {@link IEventDispatcher}s (via {@link #add(IEventDispatcher)}).
*/

wicket-core/src/main/java/org/apache/wicket/settings/FrameworkSettings.java Outdated
@@ -68,7 +88,7 @@ public FrameworkSettings(final Application application)
* Gets the Wicket version. The Wicket version is in the same format as the version element in
* the pom.xml file (project descriptor). The version is generated by maven in the build/release
* cycle and put in the /META-INF/MANIFEST.MF file located in the root folder of the Wicket jar.
*
* <p></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most probably you want the closing </p> to be at the end of the paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or. create an empty paragraph to separate from next IJ complains here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

wicket-core/src/main/java/org/apache/wicket/settings/FrameworkSettings.java Outdated
/**
* @return Returns <code>true</code> if there is at least one event dispatcher
*/
public final boolean hasAnyAnyEventDispatchers()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one too many any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@reiern70 reiern70 force-pushed the reiern70/WICKET-7080 branch from a1d0228 to d55432f Compare November 1, 2023 18:38
@reiern70 reiern70 requested a review from martin-g November 1, 2023 18:52
@reiern70 reiern70 force-pushed the reiern70/WICKET-7080 branch from d55432f to 7d40ce7 Compare November 1, 2023 18:54
@reiern70 reiern70 merged commit 7d40ce7 into wicket-9.x Nov 1, 2023
2 checks passed
@reiern70 reiern70 deleted the reiern70/WICKET-7080 branch November 1, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitstorm bitstorm Awaiting requested review from bitstorm

@martin-g martin-g Awaiting requested review from martin-g

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@reiern70 @martin-g