Prevent inadvertently repeated callbacks in ProtocolProxy #384

Merged
merged 2 commits into from Mar 17, 2016

Projects

None yet

3 participants

@chrismayer
Contributor

This PR is supposed to fix the FeatureStore issues (ExtJS 5) documented in #378.

As discussed in #378 this fixes issue 1.) and can be used as base for further development to fix issue 2.)

@chrismayer chrismayer Ensure ProtocolProxy loads features with Ext 5
This changes the execution order of setSuccessful and setCompleted
functions within the loading process of a ProtocolProxy. This avoids that the
reading of the features is aborted with ExtJS 5.
9dca7e2
@chrismayer
Contributor

Is it possible that switching the call order of setCompleted and setSuccessful will already fix this?
Without the optional true to call setCompleted (Ext 5) from setSuccessful it should be safe.

Hey @bentrm thanks for this suggestion. I gave it a shot and it works. Correcting the execution order also solves the problem and should be more safe. I corrected the commit.

@marcjansen
Member

Now that's a nice commit/diff.

Thanks to both of you. Please merge, @chrismayer

@chrismayer
Contributor

Thanks for your review @marcjansen but this is still work in progress since the second part of the fix, which is more tricky, is still to do. As said in #378 I fear that this is deeply settled somewhere in the ExtJS 'proxy-/reader-world'.

@marcjansen
Member

You decide, @chrismayer

@bentrm
Member
bentrm commented Mar 17, 2016

@chrismayer, I think this should be merged as it solves the isolated issue of inadvertently repeated callbacks. It would make it easier for others to tackle the remaining issue described in #378 in a separate PR as well.

@chrismayer chrismayer changed the title from [WIP] Make FeatureStore with Proxy working with ExtJS 5 to Prevent inadvertently repeated callbacks in ProtocolProxy Mar 17, 2016
@chrismayer
Contributor

Hi @bentrm,
I agree that we can interpret the inadvertently repeated callbacks as a single issue. I adapted the title of this PR and I will merge this now.

Thank you all for your input.

@chrismayer chrismayer merged commit fd29082 into geoext:master Mar 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment