Niels I would really like this fix to make it into the next release, in
part because I am testing uDig which depends on having working events :)
Is there any chance of getting a test added to this one?
Jody Garnett
On Thu, May 8, 2014 at 7:17 AM, Jody Garnett <[email protected]> wrote:
> Unfortantly we always need a test case when accepting fixes. Still open up
> a bug report and it can wait until one of us has time.
>
> Jody Garnett
>
>
> On Thu, May 8, 2014 at 6:32 AM, Niels Charlier <[email protected]> wrote:
>
>> Hi Jody,
>>
>> I understand, cool.
>> I did test that the change didn't break anything, including online tests.
>>
>> I wanted to report what seems to be a bug to me.
>> Writing additional tests for this I think is out of scope of my current
>> project.
>> Perhaps in my own time I will do it.
>>
>> Kind Regards
>> Niels
>>
>>
>> On 08/05/14 05:31, Jody Garnett wrote:
>>
>> Your original bug report made sense, I was just going through the senario
>> to use to test if fix works.
>>
>> Jody Garnett
>>
>>
>> On Wed, May 7, 2014 at 10:52 AM, Niels Charlier <[email protected]> wrote:
>>
>>> Jody,
>>>
>>> If the events need to be sent to the listeners of 'source', then there
>>> is no need for a loop. While multiply the event sent to source by the
>>> amount of entries there are (apart from source itself)?
>>>
>>> If it does need to be sent to 'source', why the check if( entry ==
>>> source ) { continue; // no notificaiton required }
>>>
>>> Basically, the method as it exists today could be simplified as follows:
>>>
>>> for(int i=0 ; i < state.values().size()-1; i++ ){
>>>
>>> for( FeatureListener listener : source.listeners ){
>>> try {
>>> listener.changed( notification );
>>> }
>>> catch (Throwable t ){
>>> // problem issuing notification to an interested
>>> party
>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>> issuing feature event "+notification, t );
>>> }
>>> }
>>>
>>> Now does that make sense?
>>>
>>> The thing is, the events have already sent to source by source itself,
>>> it is source who calls this method after sending the events to its own
>>> listeners. It seems to me that is why the 'continue' check is in place...
>>> because it needs to be sent to entries *other* than source.
>>>
>>> Kind Regards
>>> Niels
>>>
>>>
>>>
>>> On 07/05/14 15:32, Jody Garnett wrote:
>>>
>>> I am not sure Niels, we do have some tricky logic around source vs
>>> entry, but I expect you are on to something.
>>>
>>> The test case to write (to confirm this) involves two transactions:
>>> a) One transaction issuing a bulk event, such as commit
>>> b) A second transaction, which should get this commit notification sent
>>> to its listeners
>>>
>>> The other case is transaction auto commit:
>>> a) event sent on transaction auto commit, such as feature add
>>> b) a second transaction, which should get this feature add event
>>> notification
>>>
>>> Jody
>>>
>>>
>>> Jody Garnett
>>>
>>>
>>> On Wed, May 7, 2014 at 7:15 AM, Niels Charlier <[email protected]> wrote:
>>>
>>>> Hi Everyone,
>>>>
>>>> while working on wfs-ng, I think I found a bug in the events system in
>>>> gt-data.
>>>> This is my suggested change:
>>>>
>>>> https://github.com/NielsCharlier/geotools/commit/af9df3e282d0ad70fbf463fa5e2b36225a53116a
>>>>
>>>> In
>>>> modules/library/data/src/main/java/org/geotools/data/store/ContentEntry.java
>>>> , now it is:
>>>>
>>>> for(ContentState entry : state.values() ){
>>>> if( entry == source ) {
>>>> continue; // no notificaiton required
>>>> }
>>>> for( FeatureListener listener : source.listeners ){
>>>> try {
>>>> listener.changed( notification );
>>>> }
>>>> catch (Throwable t ){
>>>> // problem issuing notification to an interested
>>>> party
>>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>>> issuing feature event "+notification, t );
>>>> }
>>>> }
>>>> }
>>>>
>>>> I would assume it should be this:
>>>>
>>>> for(ContentState entry : state.values() ){
>>>> if( entry == source ) {
>>>> continue; // no notificaiton required
>>>> }
>>>> for( FeatureListener listener : *entry*.listeners ){
>>>> try {
>>>> listener.changed( notification );
>>>> }
>>>> catch (Throwable t ){
>>>> // problem issuing notification to an interested
>>>> party
>>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>>> issuing feature event "+notification, t );
>>>> }
>>>> }
>>>> }
>>>>
>>>>
>>>> Why loop over the entries otherwise? It seems to me my change makes
>>>> more sense.
>>>>
>>>> Kind Regards
>>>> Niels
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Is your legacy SCM system holding you back? Join Perforce May 7 to find
>>>> out:
>>>> • 3 signs your SCM is hindering your productivity
>>>> • Requirements for releasing software faster
>>>> • Expert tips and advice for migrating your SCM now
>>>> http://p.sf.net/sfu/perforce
>>>> _______________________________________________
>>>> GeoTools-Devel mailing list
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>>>
>>>>
>>>
>>>
>>
>>
>
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
