Skip to content

Commit

Permalink
Add breadcrumb for invalid selection indices
Browse files Browse the repository at this point in the history
  • Loading branch information
@mchowning
mchowning committed May 22, 2020
1 parent 32d722f commit 999672f
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 11 deletions.
2 changes: 1 addition & 1 deletion android/app/src/main/java/com/gutenberg/MainApplication.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ protected List<ReactPackage> getPackages() {
new ReactSliderPackage(),
new ReactVideoPackage(),
new SvgPackage(),
new ReactAztecPackage(null),
new ReactAztecPackage(null, null),

This comment has been minimized.

Sign in to view

Copy link
@hypest

hypest May 25, 2020 •
edited
Loading

Contributor

At this point, those 2 nulls probably need a line comment for quickly understanding what they are about. Something like "null for the exception and breadcrumb loggers" or the like.

This comment has been minimized.

Sign in to view

Copy link
@mchowning

mchowning May 25, 2020

Author Contributor

I don't feel strongly against this change, but I'm curious about your thinking behind it @hypest . It feels to me like it's more of a describing "what the code is doing" type of comment (which I generally try to avoid) as compared to a "describe why the code is doing something". I can definitely see how that comment would be useful when looking at code outside of an IDE, but inside an IDE it seems that clicking through to the constructor definition is almost as fast as reading a comment and conveys the same information (possibly better information since it can never get out of date). Like I said, I'm fine with making this change, but I'm curious to hear if you have any additional thoughts.

This comment has been minimized.

Sign in to view

Copy link
@hypest

hypest May 25, 2020 •
edited
Loading

Contributor

It feels to me like it's more of a describing "what the code is doing" type of comment (which I generally try to avoid) as compared to a "describe why the code is doing something"

Ah, going with "why the code is doing something" suits me equally well, if not better, here. Indeed, it was reading the code outside of an IDE that triggered my comment. I had the same "feeling" the first time I saw the (then single parameter) null but thought it wasn't hard for the reader to figure it out. With the second null, it will beg for some looking into and since those parameters are not very important (they are just refs to loggers) I thought a line comment would save the dev mental trouble to dive into it.

I can leave it to you actually, no strong feelings here either. "No line comment" is OK too.

This comment has been minimized.

Sign in to view

Copy link
@mchowning

mchowning May 25, 2020

Author Contributor

Thanks for explaining. I went ahead and added a brief comment about not needing log handlers in the demo app.
new LinearGradientPackage(),
mRnReactNativeGutenbergBridgePackage);
}
Expand Down
11 changes: 6 additions & 5 deletions ...-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,10 @@
import org.wordpress.aztec.plugins.wpcomments.HiddenGutenbergPlugin;
import org.wordpress.aztec.plugins.wpcomments.WordPressCommentsPlugin;
import org.wordpress.aztec.plugins.wpcomments.toolbar.MoreToolbarButton;
import org.wordpress.aztec.spans.UnknownHtmlSpan;

import java.util.List;
import java.util.Map;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class ReactAztecManager extends BaseViewManager<ReactAztecText, LayoutShadowNode> {

Expand All @@ -82,9 +78,11 @@ public class ReactAztecManager extends BaseViewManager<ReactAztecText, LayoutSha
private static final String BLOCK_TYPE_TAG_KEY = "tag";

@Nullable private final Consumer<Exception> exceptionLogger;
@Nullable private final Consumer<String> breadcrumbLogger;

public ReactAztecManager(@Nullable Consumer<Exception> exceptionLogger) {
public ReactAztecManager(@Nullable Consumer<Exception> exceptionLogger, @Nullable Consumer<String> breadcrumbLogger) {
this.exceptionLogger = exceptionLogger;
this.breadcrumbLogger = breadcrumbLogger;
initializeFocusAndBlurCommandCodes();
}

Expand Down Expand Up @@ -247,6 +245,9 @@ private void updateSelectionIfNeeded(ReactAztecText view, @Nullable ReadableMap
if (exceptionLogger != null) {
exceptionLogger.accept(customException);
}
if (breadcrumbLogger != null) {
breadcrumbLogger.accept(customException.getMessage());
}
} catch (Exception e) {
// Should never happen, but if it does don't let logging cause a crash
e.printStackTrace();
Expand Down
6 changes: 4 additions & 2 deletions ...-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecPackage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
public class ReactAztecPackage implements ReactPackage {

private final Consumer<Exception> exceptionLogger;
private final Consumer<String> breadcrumbLogger;

public ReactAztecPackage(Consumer<Exception> exceptionLogger) {
public ReactAztecPackage(Consumer<Exception> exceptionLogger, Consumer<String> breadcrumbLogger) {
this.exceptionLogger = exceptionLogger;
this.breadcrumbLogger = breadcrumbLogger;
}

@Override
public List<ViewManager> createViewManagers(ReactApplicationContext reactContext) {
List<ViewManager> views = new ArrayList<>();
views.add(new ReactAztecManager(exceptionLogger));
views.add(new ReactAztecManager(exceptionLogger, breadcrumbLogger));
return views;
}

Expand Down
9 changes: 6 additions & 3 deletions ...rg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public class WPAndroidGlueCode {
private static OkHttpClient sOkHttpClient = new OkHttpClient.Builder().addInterceptor(sAddCookiesInterceptor).build();
private boolean mIsDarkMode;
private Consumer<Exception> mExceptionLogger;
private Consumer<String> mBreadcrumbLogger;

public void onCreate(Context context) {
SoLoader.init(context, /* native exopackage */ false);
Expand Down Expand Up @@ -337,7 +338,7 @@ public void logUserEvent(GutenbergUserEvent event, ReadableMap eventProperties)
new MainReactPackage(getMainPackageConfig(getImagePipelineConfig(sOkHttpClient))),
new SvgPackage(),
new LinearGradientPackage(),
new ReactAztecPackage(mExceptionLogger),
new ReactAztecPackage(mExceptionLogger, mBreadcrumbLogger),
new ReactVideoPackage(),
new ReactSliderPackage(),
mRnReactNativeGutenbergBridgePackage);
Expand All @@ -363,9 +364,11 @@ public void onCreateView(Context initContext,
Bundle translations,
int colorBackground,
boolean isDarkMode,
Consumer<Exception> exceptionLogger) {
mExceptionLogger = exceptionLogger;
Consumer<Exception> exceptionLogger,
Consumer<String> breadcrumbLogger) {
mIsDarkMode = isDarkMode;
mExceptionLogger = exceptionLogger;
mBreadcrumbLogger = breadcrumbLogger;
mReactRootView = new ReactRootView(new MutableContextWrapper(initContext));
mReactRootView.setBackgroundColor(colorBackground);

Expand Down

0 comments on commit 999672f

Please sign in to comment.