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.

Sign up for GitHub

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

Jump to bottom

[Android] Allows to set individual (left,top,right,bottom) dotted/dashed borders #29099

Closed
wants to merge 4 commits into from
+45 −11
Closed

[Android] Allows to set individual (left,top,right,bottom) dotted/dashed borders #29099

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cec8ac9
updating example
fabOnReact Jun 9, 2020
40e940a
updating ReactViewBackgroundDrawable to support single dashed borders
fabOnReact Jun 9, 2020
cbc9f45
changes required in code-review
fabOnReact Jul 9, 2020
8f446a7
Merge branch 'master' into fix-dotted-borderLeft
fabOnReact Aug 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions RNTester/js/examples/Border/BorderExample.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const styles = StyleSheet.create({
border1: {
borderWidth: 10,
borderColor: 'brown',
borderStyle: 'dotted',
},
borderRadius: {
borderWidth: 10,
Expand All @@ -38,10 +39,10 @@ const styles = StyleSheet.create({
},
border3: {
borderColor: 'purple',
borderTopWidth: 10,
borderTopWidth: 7,
borderRightWidth: 20,
borderBottomWidth: 30,
borderLeftWidth: 40,
borderBottomWidth: 10,
borderLeftWidth: 5,
},
border4: {
borderTopWidth: 10,
Expand Down Expand Up @@ -99,12 +100,14 @@ const styles = StyleSheet.create({
},
border8Left: {
borderLeftWidth: 5,
borderStyle: 'dotted',
},
border8Bottom: {
borderBottomWidth: 5,
},
border8Right: {
borderRightWidth: 5,
borderStyle: 'dashed',
},
border9: {
borderWidth: 10,
Expand Down
44 changes: 37 additions & 7 deletions ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,14 @@ private void updatePathEffect() {
mPaint.setPathEffect(mPathEffectForBorderStyle);
}

private void updatePathEffect(int borderWidth) {
PathEffect pathEffectForBorderStyle = null;
if(mBorderStyle != null) {
pathEffectForBorderStyle = BorderStyle.getPathEffect(mBorderStyle, borderWidth);
}
mPaint.setPathEffect(pathEffectForBorderStyle);
}

/** For rounded borders we use default "borderWidth" property. */
public float getFullBorderWidth() {
return (mBorderWidth != null && !YogaConstants.isUndefined(mBorderWidth.getRaw(Spacing.ALL)))
Expand Down Expand Up @@ -1099,21 +1107,43 @@ private void drawRectangularBackgroundWithBorders(Canvas canvas) {
int bottom = bounds.bottom;

mPaint.setColor(fastBorderColor);
mPaint.setStyle(Paint.Style.STROKE);
if (borderLeft > 0) {
int leftInset = left + borderLeft;
canvas.drawRect(left, top, leftInset, bottom - borderBottom, mPaint);
Path path = new Path();
Copy link
Contributor

Choose a reason for hiding this comment

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

It is generally recommended against creating new instances in functions called in onDraw. You may want to consider creating the Path where mPaint is created and reset it before using it here.

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitdion I will try to implement your suggestions and improve this solution. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @benoitdion

It is generally recommended against creating new instances in functions called in onDraw. You may want to consider creating the Path where mPaint is created and reset it before using it here.

I just noticed that the changes required in your message are included in commit cbc9f45

  1. creating the Path where mPaint is created

The Path mPathForSingleBorder is created where mPaint is created

react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java

Line 92 in cbc9f45

private Path mPathForSingleBorder = new Path();

react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java

Line 106 in cbc9f45

private final Paint mPaint = new Paint(Paint.ANTI_ALIAS_FLAG);

  1. and reset it before using it here.

mPathForSingleBorder is resetted before drawing the border

react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java

Line 1114 in cbc9f45

mPathForSingleBorder.reset();

  1. I tried to replace moveTo and lineTo with drawLine, but the dotted style is not applied
  2. I tried moving this logic to a separate function as displayed in fabOnReact@ed3230c and then apply recursion using invoke and lambda.

I decided to not commit those changes as they do not make the code more readable, more clear and easier to search. Moving the logic to a separate function requires other developers to read/memorize an additional function.

The only reason to have a drawSide function below would be:

  1. It would be easier to write a unit test. I would unit test this function with a mocking library and I would invoke the function drawSide(width, start, end, spy) and set an expectation on the spy canvas parameter.

I would expect that spy received method :drawPath with parameters mPathForSingleBorder and mPaint (test-spy). I would verify mPathForSingleBorder and that mPaint is dotted.

  private void drawSide(int width, PointF start, PointF end, Canvas canvas) {
    mPathForSingleBorder.reset();
    updatePathEffect(width);
    mPaint.setStrokeWidth(width);
    mPathForSingleBorder.moveTo(start.x, start.y);
    mPathForSingleBorder.lineTo(end.x, end.y);
    canvas.drawPath(mPathForSingleBorder, mPaint);
  }
  1. The below logic is repeated 4 times (left, top, right, bottom). The below code repetition could be avoided with a fifth parameter in drawSide. The fifth parameter would be a lambda, the lambda would use invoke to send a message (:left, :top, :right, :bottom) to borderWidth and return from the drawSide method if the result is 0. The same could be done to calculate start and end Point.
          if (borderWidth.left > 0) {
            int width = Math.round(borderWidth.left);
            PointF start = new PointF(left, top - borderWidth.top/2);
            PointF end = new PointF(left, bottom + borderWidth.bottom/2);
            drawSide(width, start, end, canvas);
          }

But I would not use reflection with this class as it would make the code less readable and harder to search.
I would prefer to write those changes in a separate Pull Request and give clear explanation of those changes.

I also believe that I should focus on fixing existing issues in react-native, for this reason I did not persist on the above changes.

This afternoon I will test further this functionality to detect any other potential issues
I remain available for further changes
Thanks a lot
Fabrizio Bertoglio

int width = Math.round(borderWidth.left);
updatePathEffect(width);
mPaint.setStrokeWidth(width);
path.moveTo(left, top - borderWidth.top/2);
path.lineTo(left, bottom + borderWidth.bottom/2);
canvas.drawPath(path, mPaint);
}
if (borderTop > 0) {
int topInset = top + borderTop;
canvas.drawRect(left + borderLeft, top, right, topInset, mPaint);
Path path = new Path();
int width = Math.round(borderWidth.top);
updatePathEffect(width);
mPaint.setStrokeWidth(width);
path.moveTo(left, top);
path.lineTo(right, top);
canvas.drawPath(path, mPaint);
}
if (borderRight > 0) {
int rightInset = right - borderRight;
canvas.drawRect(rightInset, top + borderTop, right, bottom, mPaint);
Path path = new Path();
int width = Math.round(borderWidth.right);
updatePathEffect(width);
mPaint.setStrokeWidth(width);
path.moveTo(right, top - borderWidth.top/2);
path.lineTo(right, bottom + borderWidth.bottom/2);
canvas.drawPath(path, mPaint);
}
if (borderBottom > 0) {
int bottomInset = bottom - borderBottom;
canvas.drawRect(left, bottomInset, right - borderRight, bottom, mPaint);
Path path = new Path();
int width = Math.round(borderWidth.bottom);
updatePathEffect(width);
mPaint.setStrokeWidth(width);
path.moveTo(left, bottom);
path.lineTo(right, bottom);
canvas.drawPath(path, mPaint);
}
}
} else {
Expand Down