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.

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

Fix delayed update and weird output when typing fast in AztecRN #24

Merged
merged 3 commits into from Jun 29, 2018

Conversation

daniloercoli
Copy link
Contributor

This PR just check the already available mNativeEventCount variable, and decides if the Aztec instance needs to be updated or not by matching the value.

This was necessary since Native -> JS -> Native refresh was really downgrading performance on writing and also introducing errors on delete/write.

Not completely sure this is the correct way, but it's a way that seems to work fine.

@daniloercoli
Copy link
Contributor Author

Btw, this PR should go in tandem with wordpress-mobile/gutenberg-mobile#62

@hypest
Copy link
Contributor

hypest commented Jun 29, 2018

👋 @daniloercoli , tried to run the example app and it fails on my side with:

Error while updating property 'text' of a view managed by: RCTAztecView

java.lang.String cannot be cast to com.facebook.react.bridge.ReadableNativeMap

updateViewProp
    ViewManagersPropertyCache.java:92
setProperty
    ViewManagerPropertyUpdater.java:129
updateProps
    ViewManagerPropertyUpdater.java:48
updateProperties
    ViewManager.java:32
createView
    NativeViewHierarchyManager.java:232
execute
    UIViewOperationQueue.java:152
dispatchPendingNonBatchedOperations
    UIViewOperationQueue.java:1012
doFrameGuarded
    UIViewOperationQueue.java:983
doFrame
    GuardedFrameCallback.java:29
doFrame
    ReactChoreographer.java:134
doFrame
    ChoreographerCompat.java:105
run
    Choreographer.java:872
doCallbacks
    Choreographer.java:686
doFrame
    Choreographer.java:618
run
    Choreographer.java:860
handleCallback
    Handler.java:751
dispatchMessage
    Handler.java:95
loop
    Looper.java:154
main
    ActivityThread.java:6121
invoke
    Method.java
run
    ZygoteInit.java:889
main
    ZygoteInit.java:779

Is that expected?

@daniloercoli
Copy link
Contributor Author

Oopps! I forgot to update the demo app.

Should be fixed now.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @daniloercoli ! It works nicely while editing!

I only left one, not very important comment.

@@ -86,9 +88,18 @@ protected ReactAztecText createViewInstance(ThemedReactContext reactContext) {
}

@ReactProp(name = "text")
public void setText(ReactAztecText view, String text) {
public void setText(ReactAztecText view, ReadableMap inputMap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that important but, how about including the setIsSettingTextFromJS() call in the conditional as well to avoid calling them at all? Effectively, we could have a new method do the text setting dance and be called conditionally. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I've moved the common code in a new private routine so setIsSettingTextFromJS is only called when needed.

@hypest
Copy link
Contributor

hypest commented Jun 29, 2018

LGTM!

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.

None yet

2 participants