fix #1592 : add marker support on Rect, Line, Ellipse, Circle and Group #1594

mlecoq · 2021-05-26T14:47:29Z

Summary

Explain the motivation for making this change: here are some points to help you:

What issues does the pull request solve? Please tag them so that they will get automatically closed once the PR is merged
Marker - display issues on android #1592
What is the feature? (if applicable)

Markers on Line, Rect, Ellipse, Circle and Group
How did you implement the solution?

Markers were only displayed on Path on Android. I have looked at the code and Markers are based on elements attribute which was only filled via PathParser. I have modified getPath method on other shapes in order to fill elements attribute. For groups I only copied data from children.
What areas of the library does it impact?

Only Android is impacted, markers are now displayed on Rect, Line, Ellipse, Circle and Group

Test Plan

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.

What's required for testing (prerequisites)?

none

What are the steps to reproduce (after prerequisites)?

Create a svg with a marker, for example on a rectangle

<Svg width="400" height="200" viewBox="0 0 4000 2000">
  <Defs>
    <Marker
      id="selection"
      markerUnits={'userSpaceOnUse' as MarkerUnits}
      refX="0"
      refY="0"
       orient="auto"
    >
     <Circle
            fill="#3a6cff"
            r={5}
            cx={0}
            cy={0}
            strokeWidth={1}
            stroke="white"
          />
    </Marker>
  </Defs>
  <Rect
    x="10"
    y="10"
    width="3980"
    height="1980"
    fill="none"
    stroke="blue"
    strokeWidth="10"
   marker="url(#selection)"
  />
</Svg>

Compatibility

OS	Implemented
iOS	❌
Android	✅

Checklist

I have tested this on a device and a simulator
I added documentation in README.md
I updated the typed files (typescript)
I added a test for the API in the __tests__ folder

somasekharkakarla · 2022-04-15T11:27:43Z

@mlecoq marker still does not work. please test it once your end.
Platfrom: Android
Lib.version: 12.3.0

Please resolve this as soon as possible.

mlecoq · 2022-04-15T11:33:14Z

the PR is not yet merged, can you test with "react-native-svg": "mlecoq/react-native-svg#hotfix/displayMarkersOnAndroid", ?

somasekharkakarla · 2022-04-16T09:39:11Z

@mlecoq yeah sure

somasekharkakarla · 2022-04-16T10:06:31Z

@mlecoq npm i git+https://github.com/mlecoq/react-native-svg.git#hotfix/displayMarkersOnAndroid
unable to install.
Please provide me steps to install

somasekharkakarla · 2022-04-16T10:39:56Z

i tried to change same files in original libs folder for line and group. Still marker didn't work

mlecoq · 2022-04-16T12:02:35Z

Can you share your example ?

somasekharkakarla · 2022-04-17T05:52:29Z

hi @mlecoq ,
here is the example https://github.com/somasekharkakarla/svg_test_react_native
By the way i am trying to implement same as (https://www.npmjs.com/package/@archireport/react-native-svg-draw).

mlecoq · 2022-04-17T08:22:03Z

Thanks !

somasekharkakarla · 2022-04-17T16:25:08Z

Hi @mlecoq any update?

mlecoq · 2022-04-17T17:06:45Z

I'm on vacation 😄, next week !

somasekharkakarla · 2022-04-17T18:18:45Z

@mlecoq enjoy vacation. Your solution worked. i just reset my cache and re started my laptop it worked. Thank you bro.
Why still PR not merge with main library?

WoLewicki · 2022-04-19T16:07:51Z

I believe it was not added since the svg standard does not support markers for those elements: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/marker. Or am I missing something? On the other hand, if it is available on iOS, maybe it would be good to have it on both platforms.

mlecoq · 2022-04-22T09:05:57Z

Hi @WoLewicki

Thanks for your feedback

If we look at the documentation, markers should be available at least for Line tags.

And on iOS with react-native-svg it works for all of them

mlecoq · 2022-05-10T13:10:20Z

There is an issue on android on marker size (which are bigger compared to iOS). I've fixed it through this commit : 845cb88

jake-nz · 2022-08-01T01:46:20Z

I think this should be merged. True, the SVG spec only has markers for <path>, <line>, <polyline> and <polygon> but it's better to merge this PR and support extra elements than the current state where some elements that should be supported are not.

I'm currently facing this issue where my <Line>s have no markers
I can use <Path> instead but the markers are huge on Android
This PR fixes both issues

tmeiss · 2022-10-24T19:23:52Z

Any plans to merge this?

…, Circle and Group

narliecholler · 2023-03-13T18:03:30Z

merging? O.o

WoLewicki · 2023-03-28T14:45:33Z

I tried using it with percentages instead of numbers for props, and the points seem to be calculated in a wrong way then. Am I missing something?

WoLewicki · 2023-03-28T15:37:42Z

android/src/main/java/com/horcrux/svg/CircleView.java

@@ -79,6 +81,14 @@ Path getPath(Canvas canvas, Paint paint) {
    double r = relativeOnOther(mR);

    path.addCircle((float) cx, (float) cy, (float) r, Path.Direction.CW);
+
+    elements = new ArrayList<>();
+    elements.add(new PathElement(ElementType.kCGPathElementMoveToPoint, new Point[]{new Point(mCx.value,mCy.value - mR.value )}));


I think that in order for it to work correctly, you should use the relative values, so in this example cx, cy, r. And for some reason they should be divided by scale attribute, which you can get like this: float scale = DisplayMetricsHolder.getScreenDisplayMetrics().density;

WoLewicki · 2023-03-28T16:58:28Z

@mlecoq I pushed a fix of those mentioned problems to your fork. Could you merge it to your branch so they are available here?

mlecoq · 2023-03-28T18:50:46Z

@WoLewicki done !

WoLewicki

LGTM!

WoLewicki · 2023-03-29T13:23:54Z

I spotted some problems with different kinds of svgs for marker elements, and I will address them in another PR to keep them small.

syedquadri1982 approved these changes Sep 21, 2021

View reviewed changes

mlecoq force-pushed the hotfix/displayMarkersOnAndroid branch 2 times, most recently from d1a1845 to f451eb2 Compare May 10, 2022 14:04

mlecoq force-pushed the hotfix/displayMarkersOnAndroid branch 3 times, most recently from 1214046 to 85a0f36 Compare November 9, 2022 11:28

mlecoq changed the base branch from develop to main November 9, 2022 11:34

mlecoq added 2 commits November 9, 2022 12:50

fix software-mansion#1592 : add marker support on Rect, Line, Ellipse…

5dd50cf

…, Circle and Group

fix: wrong scale of markers on android

c791696

mlecoq force-pushed the hotfix/displayMarkersOnAndroid branch from 014f4d7 to c791696 Compare November 9, 2022 11:51

WoLewicki reviewed Mar 28, 2023

View reviewed changes

fix: marker points calculation for percentages and add markers example

f19b2fd

WoLewicki approved these changes Mar 29, 2023

View reviewed changes

WoLewicki linked an issue Mar 29, 2023 that may be closed by this pull request

Marker - display issues on android #1592

Closed

WoLewicki merged commit a7af70c into software-mansion:main Mar 29, 2023

WoLewicki mentioned this pull request Mar 29, 2023

Line markerEnd doesn't work on Android #1739

Closed

mlecoq deleted the hotfix/displayMarkersOnAndroid branch March 30, 2023 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #1592 : add marker support on Rect, Line, Ellipse, Circle and Group #1594

fix #1592 : add marker support on Rect, Line, Ellipse, Circle and Group #1594

mlecoq commented May 26, 2021

somasekharkakarla commented Apr 15, 2022

mlecoq commented Apr 15, 2022

somasekharkakarla commented Apr 16, 2022

somasekharkakarla commented Apr 16, 2022 •

edited

Loading

somasekharkakarla commented Apr 16, 2022

mlecoq commented Apr 16, 2022

somasekharkakarla commented Apr 17, 2022 •

edited

Loading

mlecoq commented Apr 17, 2022

somasekharkakarla commented Apr 17, 2022

mlecoq commented Apr 17, 2022

somasekharkakarla commented Apr 17, 2022

WoLewicki commented Apr 19, 2022 •

edited

Loading

mlecoq commented Apr 22, 2022

mlecoq commented May 10, 2022 •

edited

Loading

jake-nz commented Aug 1, 2022

tmeiss commented Oct 24, 2022

narliecholler commented Mar 13, 2023

WoLewicki commented Mar 28, 2023

WoLewicki Mar 28, 2023

WoLewicki commented Mar 28, 2023

mlecoq commented Mar 28, 2023

WoLewicki left a comment

WoLewicki commented Mar 29, 2023

fix #1592 : add marker support on Rect, Line, Ellipse, Circle and Group #1594

fix #1592 : add marker support on Rect, Line, Ellipse, Circle and Group #1594

Conversation

mlecoq commented May 26, 2021

Summary

Test Plan

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

Checklist

somasekharkakarla commented Apr 15, 2022

mlecoq commented Apr 15, 2022

somasekharkakarla commented Apr 16, 2022

somasekharkakarla commented Apr 16, 2022 • edited Loading

somasekharkakarla commented Apr 16, 2022

mlecoq commented Apr 16, 2022

somasekharkakarla commented Apr 17, 2022 • edited Loading

mlecoq commented Apr 17, 2022

somasekharkakarla commented Apr 17, 2022

mlecoq commented Apr 17, 2022

somasekharkakarla commented Apr 17, 2022

WoLewicki commented Apr 19, 2022 • edited Loading

mlecoq commented Apr 22, 2022

mlecoq commented May 10, 2022 • edited Loading

jake-nz commented Aug 1, 2022

tmeiss commented Oct 24, 2022

narliecholler commented Mar 13, 2023

WoLewicki commented Mar 28, 2023

WoLewicki Mar 28, 2023

Choose a reason for hiding this comment

WoLewicki commented Mar 28, 2023

mlecoq commented Mar 28, 2023

WoLewicki left a comment

Choose a reason for hiding this comment

WoLewicki commented Mar 29, 2023

somasekharkakarla commented Apr 16, 2022 •

edited

Loading

somasekharkakarla commented Apr 17, 2022 •

edited

Loading

WoLewicki commented Apr 19, 2022 •

edited

Loading

mlecoq commented May 10, 2022 •

edited

Loading