Skip to content

Conversation

@jsarman
Copy link
Contributor

@jsarman jsarman commented Jul 16, 2013

Major changes to internals. Now supports @Conversational(auto,propagation) annotation to allow pages to override global configuration. Added WicketCdiFilter to allow the Cdi functionality to be set before the apps are loaded. This allows for configuration of global settings in web.xml and for Injection at Application initialization. One can now put @Inject in the WebApplication class and use the variable in the init function or anywhere else. Added several tests for ConversationPropagation use cases. Reintroduced the original Configuration option as deprecated methods to allow easier transition from cdi 1.0 implementation. The Propagation and Auto can be programmatically changed at runtime if the calling code is in a non-transient conversation. Any changes to the global defaults are stored in a ConversationManager which is ConversationScoped. Adding the test helped to weed out many potential problems.

martin-g and others added 30 commits June 27, 2013 06:37
fix typo
java 7 diamonds
…ge isn't visible on the page

WICKET-5128 Allow for Testing Component relative Feedback-Messages in Wicket-Tester
CedricGatay and others added 15 commits June 27, 2013 06:38
We now correctly detect if the resource already ends with .min to avoid further unnecessary compression.
Package is split up into wicket-cdi-1.1-core and wicket-cdi-1.1-weld.
Refactored some classes to use cdi injection instead of constructors.
Worked with tomcat and glassfish in my limited testing.
CdiConfiguration is now built totally on @Inject dependencies.
Replaced with Producer method in cdiConfiguration.
Adds unique global setting for each app in a war file.  Adds extension
to WicketFilter to allow for cdi inject at App deployment.  Added
several tests.
@martin-g
Copy link
Member

martin-g commented Nov 8, 2013

@papegaaij Just merged CDI 1.1 support from Wicket 7 to Wicket 6.
This PR may be reviewed.

@ivaynberg ping.

@papegaaij
Copy link
Contributor

I've checked the code in Wicket 7, and did make some changes (see the git log on wicket-6.x). Most of it seems ok and with the small modifications, it works fine on our application.

@martin-g
Copy link
Member

martin-g commented Nov 8, 2013

@papegaaij I see no commits in master from you today. All your commits are in wicket-6.x branch.
Maybe I didn't understand you.
I'm not sure though whether your commits include this PR or not, I haven't checked the details yet.

@papegaaij
Copy link
Contributor

I only backported wicket-cdi-1.1 from Wicket 7 to Wicket 6. Once I've got everything working, I'll forward port my changes to Wicket 7. I did not use this PR. It's impossible to verify the diffs in this PR: all kinds of things are mixed, it contains merges, formatting changes, etc.

@ivaynberg
Copy link
Contributor

but wicket-cdi-1.1 in master was based on that PR...

-igor

On Fri, Nov 8, 2013 at 6:48 AM, Emond Papegaaij notifications@github.comwrote:

I only backported wicket-cdi-1.1 from Wicket 7 to Wicket 6. Once I've got
everything working, I'll forward port my changes to Wicket 7. I did not use
this PR. It's impossible to verify the diffs in this PR: all kinds of
things are mixed, it contains merges, formatting changes, etc.


Reply to this email directly or view it on GitHubhttps://github.com//pull/50#issuecomment-28067540
.

@martin-g
Copy link
Member

martin-g commented Nov 8, 2013

wicket-cdi-1.1 was introduced by earlier PRs (47 and 48, I think).
This PR was too complex/radical for me as non-CDI user, so I asked for help with its review.

@jsarman
Copy link
Contributor Author

jsarman commented Nov 8, 2013

Igor,
In our conversations throughout the day I thought it was about my early
pulls, because my latest pull request could not properly merged. After
rereading that papegaaij had pulled and fixed my latest code, I will go
into defense mode on what you do not agree with. Not to start some
flame-war, but to fix what you do not like and the community agrees is the
wrong approach. The code before that should not be in a release. As for
why I refactored the original cdi-1.0 code to use cdi for injection rather
than constructor instantiate is because once the dependency is there, I
leveraged it. Was it broke before, NO. Was it essential to refactor it,
of course not, but now it is easier to create test cases. Did I break
wicket just to add cdi-1.1, not at all. So even though I updated your code
base and refactored to use cdi injection even in the low level
implementation classes, I really didn't alter the core functionality. It
still supports cdi-1.0 code. It now allows injection at the instantiation
of the App, it supports the Auto with a finer granularity. To me it is
still your code with some assistance from me to get it to 1.1.

John

On Fri, Nov 8, 2013 at 4:22 PM, Igor Vaynberg notifications@github.comwrote:

but wicket-cdi-1.1 in master was based on that PR...

-igor

On Fri, Nov 8, 2013 at 6:48 AM, Emond Papegaaij notifications@github.comwrote:

I only backported wicket-cdi-1.1 from Wicket 7 to Wicket 6. Once I've
got
everything working, I'll forward port my changes to Wicket 7. I did not
use
this PR. It's impossible to verify the diffs in this PR: all kinds of
things are mixed, it contains merges, formatting changes, etc.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28067540>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/50#issuecomment-28099108
.

@ivaynberg
Copy link
Contributor

my primary concern is the removal of features like auto conversations. i
know you are still working on a fix, but its not done yet. i do not want to
lose any functionality.

my second concern is that a lot of code has changed and no one went through
it to see if it aligns with how we do things. i found a lot of instances
where it does not.

my third concern is that the api has somewhat changed. i dont really see a
reason for that unless its really necessary.

ideally the pull requests wouldve gone like this (had they i mightve had
time to review them a lot sooner):

  • clone wicket-cdi into wicket-cdi-1.1+weld. minimal changes to remove seam
    module conversation management and replace with weld-dependent interfaces.
  • refactor auto-conversation management api, i noticed you were doing some
    of that in your changes, but they are too messy (code formatting tweaks)
    and too mixed in with everything else to clearly see what you were trying
    to accomplish
  • refactor to allow management of application using cdi lifecycle.

if they were separated like that we can easily follow and
apply/reject/refine your changes individually. as it stands now its more of
a all or nothing approach...

-igor

On Fri, Nov 8, 2013 at 1:35 PM, John Sarman notifications@github.comwrote:

Igor,
In our conversations throughout the day I thought it was about my early
pulls, because my latest pull request could not properly merged. After
rereading that papegaaij had pulled and fixed my latest code, I will go
into defense mode on what you do not agree with. Not to start some
flame-war, but to fix what you do not like and the community agrees is the
wrong approach. The code before that should not be in a release. As for
why I refactored the original cdi-1.0 code to use cdi for injection rather
than constructor instantiate is because once the dependency is there, I
leveraged it. Was it broke before, NO. Was it essential to refactor it,
of course not, but now it is easier to create test cases. Did I break
wicket just to add cdi-1.1, not at all. So even though I updated your code
base and refactored to use cdi injection even in the low level
implementation classes, I really didn't alter the core functionality. It
still supports cdi-1.0 code. It now allows injection at the instantiation
of the App, it supports the Auto with a finer granularity. To me it is
still your code with some assistance from me to get it to 1.1.

John

On Fri, Nov 8, 2013 at 4:22 PM, Igor Vaynberg notifications@github.comwrote:

but wicket-cdi-1.1 in master was based on that PR...

-igor

On Fri, Nov 8, 2013 at 6:48 AM, Emond Papegaaij <
notifications@github.com>wrote:

I only backported wicket-cdi-1.1 from Wicket 7 to Wicket 6. Once I've
got
everything working, I'll forward port my changes to Wicket 7. I did
not
use
this PR. It's impossible to verify the diffs in this PR: all kinds of
things are mixed, it contains merges, formatting changes, etc.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28067540>
.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28099108>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/50#issuecomment-28099893
.

@ivaynberg
Copy link
Contributor

To me it is
still your code with some assistance from me to get it to 1.1.

i write this code using ASL2 license, so i dont mind when other people take it, i give up my ownership at commit. its not about whose code it is or how much youve changed it, its a question of whether its changed for the better. at some point i will be using this code so i just want to make sure that i have a good experience using it. it may seem selfish, but its also why i contribute this code - so others can make it better.

@jsarman
Copy link
Contributor Author

jsarman commented Nov 8, 2013

In the latest revision, the AutoConversation is supported. Just add
@Conversational to the class. If you want to do it the cdi-1.0 way then
implement ConversationalComponent as , which was modified to have the
@Conversational annotation. The Conversational annotation also adds some
properties, prop and auto. prop allows the reconfiguration of the
ConversationPropagation for the particular Page without affecting the
global ConversationPropagation set at the Application Instantiation. The
auto property allows for the Page to override the auto property set
globally by the Application for the Session. Nothing broke just allows for
page level reconfiguration.

On Fri, Nov 8, 2013 at 4:56 PM, Igor Vaynberg notifications@github.comwrote:

my primary concern is the removal of features like auto conversations. i
know you are still working on a fix, but its not done yet. i do not want
to
lose any functionality.

my second concern is that a lot of code has changed and no one went
through
it to see if it aligns with how we do things. i found a lot of instances
where it does not.

my third concern is that the api has somewhat changed. i dont really see a
reason for that unless its really necessary.

ideally the pull requests wouldve gone like this (had they i mightve had
time to review them a lot sooner):

  • clone wicket-cdi into wicket-cdi-1.1+weld. minimal changes to remove
    seam
    module conversation management and replace with weld-dependent interfaces.
  • refactor auto-conversation management api, i noticed you were doing some
    of that in your changes, but they are too messy (code formatting tweaks)
    and too mixed in with everything else to clearly see what you were trying
    to accomplish
  • refactor to allow management of application using cdi lifecycle.

if they were separated like that we can easily follow and
apply/reject/refine your changes individually. as it stands now its more
of
a all or nothing approach...

-igor

On Fri, Nov 8, 2013 at 1:35 PM, John Sarman notifications@github.comwrote:

Igor,
In our conversations throughout the day I thought it was about my early
pulls, because my latest pull request could not properly merged. After
rereading that papegaaij had pulled and fixed my latest code, I will go
into defense mode on what you do not agree with. Not to start some
flame-war, but to fix what you do not like and the community agrees is
the
wrong approach. The code before that should not be in a release. As for
why I refactored the original cdi-1.0 code to use cdi for injection
rather
than constructor instantiate is because once the dependency is there, I
leveraged it. Was it broke before, NO. Was it essential to refactor it,
of course not, but now it is easier to create test cases. Did I break
wicket just to add cdi-1.1, not at all. So even though I updated your
code
base and refactored to use cdi injection even in the low level
implementation classes, I really didn't alter the core functionality. It
still supports cdi-1.0 code. It now allows injection at the
instantiation
of the App, it supports the Auto with a finer granularity. To me it is
still your code with some assistance from me to get it to 1.1.

John

On Fri, Nov 8, 2013 at 4:22 PM, Igor Vaynberg notifications@github.comwrote:

but wicket-cdi-1.1 in master was based on that PR...

-igor

On Fri, Nov 8, 2013 at 6:48 AM, Emond Papegaaij <
notifications@github.com>wrote:

I only backported wicket-cdi-1.1 from Wicket 7 to Wicket 6. Once
I've
got
everything working, I'll forward port my changes to Wicket 7. I did
not
use
this PR. It's impossible to verify the diffs in this PR: all kinds
of
things are mixed, it contains merges, formatting changes, etc.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28067540>
.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28099108>
.


Reply to this email directly or view it on GitHub<
https://github.com/apache/wicket/pull/50#issuecomment-28099893>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/50#issuecomment-28101325
.

asfgit pushed a commit that referenced this pull request Nov 11, 2013
@martin-g
Copy link
Member

Please close this PR.
We don't have the karma to do it.
Thanks!

@Humbedooh Humbedooh closed this Feb 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants