2:25 p.m.
Hello all,
I am currently doing some cleanup in the EventListenerRegister used in Search,
but I'm needing some clarifications (and please don't make any changes to it).
the code checks for the presence of a FullTextIndexEventListener on each
listener type, but doesn't want to use "isAssignableFrom" to let the user
subclass (this is said by a comment there). BUT it checks for
a listener of type FullTextIndexEventListener.class OR any direct extension,
to recognize also the now deprecated FullTextIndexCollectionEventListener.
Also the check is not done on both the class and it's subclass in each case.
What is the purpose exactly?
if a user creates an extension and registers it, shouldn't
we have to skip registration? so IMHO we should actually use
"isAssignableFrom".
If you prefer to register our own listener anyway, maybe we should check
for the exact type FullTextIndexEventListener.class and
FullTextIndexCollectionEventListener.class (not any subtype).
regards,
Sanne
11:48 a.m.
Hi Sanne,
I have a few comments, since I was the last one working on the code. I
hope you might find this helpful.
First of all, EventListenerRegister has its origin in the Annotations
project and just recently got moved from Annotations to Search (where it
actually belongs). I applied some refactorings, but the biggest
outstanding refactoring would be to generalise the registration of the
listeners. I tried one apporach, but the code became very hard to read. So
I decided to leave this part as is for now.
Initially I wanted to get rid of the old registration code completely and
if you check the history of the class you will see that I did not have
'searchEventListenerClass == eventListener.getClass().getSuperclass();' at
all. It worked fine for the latest versions of Annotations and Search.
However, I think there were problems when using different combinations of
Annotations and Search. For that reason Emmanuel added this additional
check. It evades me right now what the actual reason was.
Regarding "isAssignableFrom" - you might be right. Let me have another
look at the code and come back to you.
--Hardy
12:25 p.m.
Hello Hardy,
thanks for your attention, I'll attach my current version (absolutely
untested) just to give the idea.
If you look at the current trunk the last listener handling
(PostCollectionUpdateEventListener)
is not consistent with the others; I thought it was "suspect",
but forgot to mention it in my previous post.
Sanne
2008/8/23 Hardy Ferentschik <hibernate(a)ferentschik.de>:
Hi Sanne,
I have a few comments, since I was the last one working on the code. I hope
you might find this helpful.
First of all, EventListenerRegister has its origin in the Annotations
project and just recently got moved from Annotations to Search (where it
actually belongs). I applied some refactorings, but the biggest outstanding
refactoring would be to generalise the registration of the listeners. I
tried one apporach, but the code became very hard to read. So I decided to
leave this part as is for now.
Initially I wanted to get rid of the old registration code completely and if
you check the history of the class you will see that I did not have
'searchEventListenerClass == eventListener.getClass().getSuperclass();' at
all. It worked fine for the latest versions of Annotations and Search.
However, I think there were problems when using different combinations of
Annotations and Search. For that reason Emmanuel added this additional
check. It evades me right now what the actual reason was.
Regarding "isAssignableFrom" - you might be right. Let me have another look
at the code and come back to you.
--Hardy
3:41 a.m.
On Sat, 23 Aug 2008 19:25:35 +0200, Sanne Grinovero
<sanne.grinovero(a)gmail.com> wrote:
Hello Hardy,
thanks for your attention, I'll attach my current version (absolutely
untested) just to give the idea.
If you look at the current trunk the last listener handling
(PostCollectionUpdateEventListener)
is not consistent with the others; I thought it was "suspect",
but forgot to mention it in my previous post.
Your code seems to be a step into the right direction and a good exaple
for the use of generics. Personally, I would use explicit if statements
insead of the ?: operator, but that's a matter of taste.
--Hardy
6:48 a.m.
2008/8/25 Hardy Ferentschik <hibernate(a)ferentschik.de>:
On Sat, 23 Aug 2008 19:25:35 +0200, Sanne Grinovero
<sanne.grinovero(a)gmail.com> wrote:
> Hello Hardy,
> thanks for your attention, I'll attach my current version (absolutely
> untested) just to give the idea.
> If you look at the current trunk the last listener handling
> (PostCollectionUpdateEventListener)
> is not consistent with the others; I thought it was "suspect",
> but forgot to mention it in my previous post.
Your code seems to be a step into the right direction and a good exaple for
the use of generics. Personally, I would use explicit if statements insead
of the ?: operator, but that's a matter of taste.
--Hardy
yes I agree, I'll simplify the ?: operator away; this was just an
intermediate step for cleanup.
Still I'm needing your opinion about
A) the latest method "isPresentInListeners";
especially because the current code isn't checking
all listeners the same way, I suppose because you merged the now deprecated
FullTextIndexCollectionEventListener with the FullTextIndexEventListener.
I'd use isAssignableFrom, but there's a comment there explicitly telling me
not to do.
B) I'm writing a trivial test, is there any special configuration to test for?
Sanne
11:52 a.m.
On Mon, 25 Aug 2008 13:48:13 +0200, Sanne Grinovero
<sanne.grinovero(a)gmail.com> wrote:
A) the latest method "isPresentInListeners";
especially because the current code isn't checking
all listeners the same way, I suppose because you merged the now
deprecated
FullTextIndexCollectionEventListener with the FullTextIndexEventListener.
I'd use isAssignableFrom, but there's a comment there explicitly telling
me not to do.
Regarding the inconsistent check I would say that this is just a
mistake.
The checks should be consistent and I don't see a reason why
PostCollectionUpdateEventListener should be treated differently to the
others. I also think 'isAssignableFrom' is the way to go. Emmanunel is
online tomorrow. Maybe he can enlighten us regarding this comment.
B) I'm writing a trivial test, is there any special configuration
to
test for?
Of course the latest configuration - with the latest annotations
depedency
- has to work. Other tests might be harder since they depend on different
versions of Annotations and Search. You can test whether an older version
of Annotations still works with the latest of Search and vice versa.
--Hardy
4:57 p.m.
I don't quite fully remember the reasoning for not using
isAssignableFrom unfortunately. I guess I envisioned people
subclassing the regular event listener to do whatever and I did not
wanted to be impacted by their work. But then it would make sense to
consider them as registered as they are subclasses... Anyway, it's
better to prevent subclassing in the first place (final). We will be
able to do that when the collection listener will be removed (let's
set a TODO).
PostCollectionUpdateEventListener is an oversight (I swear I fixed
that at some point).
Your code looks good.
--
Emmanuel Bernard
http://in.relation.to/Bloggers/Emmanuel | http://blog.emmanuelbernard.com
| http://twitter.com/emmanuelbernard
Hibernate Search in Action (http://is.gd/Dl1)
On Aug 25, 2008, at 13:48, Sanne Grinovero wrote:
2008/8/25 Hardy Ferentschik <hibernate(a)ferentschik.de>:
> On Sat, 23 Aug 2008 19:25:35 +0200, Sanne Grinovero
> <sanne.grinovero(a)gmail.com> wrote:
>
>> Hello Hardy,
>> thanks for your attention, I'll attach my current version
>> (absolutely
>> untested) just to give the idea.
>> If you look at the current trunk the last listener handling
>> (PostCollectionUpdateEventListener)
>> is not consistent with the others; I thought it was "suspect",
>> but forgot to mention it in my previous post.
>
> Your code seems to be a step into the right direction and a good
> exaple for
> the use of generics. Personally, I would use explicit if statements
> insead
> of the ?: operator, but that's a matter of taste.
>
> --Hardy
yes I agree, I'll simplify the ?: operator away; this was just an
intermediate step for cleanup.
Still I'm needing your opinion about
A) the latest method "isPresentInListeners";
especially because the current code isn't checking
all listeners the same way, I suppose because you merged the now
deprecated
FullTextIndexCollectionEventListener with the
FullTextIndexEventListener.
I'd use isAssignableFrom, but there's a comment there explicitly
telling me
not to do.
B) I'm writing a trivial test, is there any special configuration to
test for?
Sanne
6:43 a.m.
I closed the issue using class equality, it just seemed the best
solution basing on your feedback.
People making their own extensions should know how to configure
listeners correctly anyway.
If you don't agree, reopen it: HSEARCH-253
thanks for all the help,
Sanne
2008/8/27 Emmanuel Bernard <emmanuel(a)hibernate.org>:
I don't quite fully remember the reasoning for not using
isAssignableFrom
unfortunately. I guess I envisioned people subclassing the regular event
listener to do whatever and I did not wanted to be impacted by their work.
But then it would make sense to consider them as registered as they are
subclasses... Anyway, it's better to prevent subclassing in the first place
(final). We will be able to do that when the collection listener will be
removed (let's set a TODO).
PostCollectionUpdateEventListener is an oversight (I swear I fixed that at
some point).
Your code looks good.
--
Emmanuel Bernard
http://in.relation.to/Bloggers/Emmanuel | http://blog.emmanuelbernard.com |
http://twitter.com/emmanuelbernard
Hibernate Search in Action (http://is.gd/Dl1)
On Aug 25, 2008, at 13:48, Sanne Grinovero wrote:
> 2008/8/25 Hardy Ferentschik <hibernate(a)ferentschik.de>:
>>
>> On Sat, 23 Aug 2008 19:25:35 +0200, Sanne Grinovero
>> <sanne.grinovero(a)gmail.com> wrote:
>>
>>> Hello Hardy,
>>> thanks for your attention, I'll attach my current version (absolutely
>>> untested) just to give the idea.
>>> If you look at the current trunk the last listener handling
>>> (PostCollectionUpdateEventListener)
>>> is not consistent with the others; I thought it was "suspect",
>>> but forgot to mention it in my previous post.
>>
>> Your code seems to be a step into the right direction and a good exaple
>> for
>> the use of generics. Personally, I would use explicit if statements
>> insead
>> of the ?: operator, but that's a matter of taste.
>>
>> --Hardy
>
> yes I agree, I'll simplify the ?: operator away; this was just an
> intermediate step for cleanup.
> Still I'm needing your opinion about
>
> A) the latest method "isPresentInListeners";
> especially because the current code isn't checking
> all listeners the same way, I suppose because you merged the now
> deprecated
> FullTextIndexCollectionEventListener with the FullTextIndexEventListener.
> I'd use isAssignableFrom, but there's a comment there explicitly telling
> me
> not to do.
>
> B) I'm writing a trivial test, is there any special configuration to test
> for?
>
> Sanne
5964
days inactive
5970
days old
7 comments
3 participants
participants (3)
-
Emmanuel Bernard -
Hardy Ferentschik -
Sanne Grinovero