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

[Android] Synchronize content retrieval over the bridge #36072

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import androidx.annotation.Nullable;
import androidx.core.util.Consumer;
import androidx.core.util.Pair;
import androidx.fragment.app.Fragment;

import com.brentvatne.react.ReactVideoPackage;
Expand Down Expand Up @@ -858,7 +859,7 @@ public interface OnGetContentTimeout {
void onGetContentTimeout(InterruptedException ie);
}

public CharSequence getContent(CharSequence originalContent, OnGetContentTimeout onGetContentTimeout) {
public synchronized CharSequence getContent(CharSequence originalContent, OnGetContentTimeout onGetContentTimeout) {
if (mReactContext != null) {
mGetContentCountDownLatch = new CountDownLatch(1);

Expand All @@ -878,7 +879,7 @@ public CharSequence getContent(CharSequence originalContent, OnGetContentTimeout
return originalContent;
}

public CharSequence getTitle(OnGetContentTimeout onGetContentTimeout) {
public synchronized CharSequence getTitle(OnGetContentTimeout onGetContentTimeout) {
if (mReactContext != null) {
mGetContentCountDownLatch = new CountDownLatch(1);

Expand All @@ -898,28 +899,65 @@ public CharSequence getTitle(OnGetContentTimeout onGetContentTimeout) {
return "";
}


/** This method retrieves both the title and the content from the Gutenberg editor by the emission of a single
* event. This is useful to avoid redundant events, since {@link #getTitle} and {@link #getContent} both share the
* same event anyway, and also share the same mechanism to suspend execution until a response is received (or a
* timeout is reached).
* @param originalContent fallback content to return in case the timeout is reached, or the thread is interrupted
* @param onGetContentTimeout callback to invoke if thread is interrupted before the timeout
* @return
*/
public synchronized Pair<CharSequence, CharSequence> getTitleAndContent(CharSequence originalContent,
OnGetContentTimeout onGetContentTimeout) {
if (mReactContext != null) {
mGetContentCountDownLatch = new CountDownLatch(1);

mRnReactNativeGutenbergBridgePackage.getRNReactNativeGutenbergBridgeModule().getHtmlFromJS();

try {
// TODO: we should consider logging when await returns false
mGetContentCountDownLatch.await(10, TimeUnit.SECONDS);
} catch (InterruptedException ie) {
// TODO: this should be either renamed, or invoked when the await returns false.
onGetContentTimeout.onGetContentTimeout(ie);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the intent of the get content timeout callback is not achieved by the current code. I think that onGetContentTimeout was meant to be called when the latch await timeout is exceeded, however, await does not throw in this case. Instead, it merely returns false. I believe this logic should be refactored, or at least renamed, since the current naming is misleading, imo. Also, it may be beneficial to log the cases where the timeout is indeed reached (when await returns false), which I think was the original intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this with bc2ab62 👍


return new Pair<>(
mTitle == null ? "" : mTitle,
mContentChanged ? (mContentHtml == null ? "" : mContentHtml) : originalContent
);
} else {
// TODO: Add app logging here
}

return new Pair<>("", originalContent);
}

public boolean triggerGetContentInfo(OnContentInfoReceivedListener onContentInfoReceivedListener) {
if (mReactContext != null && (mGetContentCountDownLatch == null || mGetContentCountDownLatch.getCount() == 0)) {
if (!mIsEditorMounted) {
onContentInfoReceivedListener.onEditorNotReady();
return false;
}

mGetContentCountDownLatch = new CountDownLatch(1);

mRnReactNativeGutenbergBridgePackage.getRNReactNativeGutenbergBridgeModule().getHtmlFromJS();

new Thread(new Runnable() {
@Override public void run() {
try {
mGetContentCountDownLatch.await(5, TimeUnit.SECONDS);
if (mContentInfo == null) {
// We need to synchronize access to (and overwriting of) the latch to avoid race conditions
synchronized (WPAndroidGlueCode.this) {
mGetContentCountDownLatch = new CountDownLatch(1);

mRnReactNativeGutenbergBridgePackage.getRNReactNativeGutenbergBridgeModule().getHtmlFromJS();

try {
mGetContentCountDownLatch.await(5, TimeUnit.SECONDS);
if (mContentInfo == null) {
onContentInfoReceivedListener.onContentInfoFailed();
} else {
onContentInfoReceivedListener.onContentInfoReceived(mContentInfo.toHashMap());
}
} catch (InterruptedException ie) {
onContentInfoReceivedListener.onContentInfoFailed();
} else {
onContentInfoReceivedListener.onContentInfoReceived(mContentInfo.toHashMap());
}
} catch (InterruptedException ie) {
onContentInfoReceivedListener.onContentInfoFailed();
}
}
}).start();
Expand Down