Add react-native-svg interface

[latekvo]

Description

This PR adds integration with the react-native-svg (RNSVG) to improve hitbox detection of the SVG elements they provide.

blocked by software-mansion/react-native-svg#2583
blocked by nested SvgViews with viewBox prop have invalid hit detection - No issue opened yet

Test plan

• use the newly added SVG integration example for testing this feature

[latekvo]

Integration works as of bd51ac7, but I have to test it a bit more before it's ready.

[latekvo]

// Using the latest version for now, todo: find the oldest working version

Will remove this comment, and replace the minimal version check with the latest version, as soon as this PR is released.

[m-bert]

Overall looks ok, left some comments

[m-bert]

As far as I can see this is function is called iff view is SvgElement. In that case, can't we simplify it?

Suggested change

	if (view is SvgView) {
	val childrenIds = view.children.map { it.id }
	val hasBeenPressed = view.id == pressedId
	val hasChildBeenPressed = pressedId in childrenIds
	val pressIsInBounds = 0 < posX && posX < view.width && 0 < posY && posY < view.height
	return (hasBeenPressed || hasChildBeenPressed) && pressIsInBounds
	}
	if (view is VirtualView) {
	return view.id == highestOrderSvgView.reactTagForTouch(rootX, rootY)
	}
	return false
	if (view is VirtualView) {
	return view.id == highestOrderSvgView.reactTagForTouch(rootX, rootY)
	}
	val childrenIds = view.children.map { it.id }
	val pressIsInBounds = 0 < posX && posX < view.width && 0 < posY && posY < view.height
	return (view.id == pressedId || pressedId in childrenIds) && pressIsInBounds

Also it would be nice to use .. instead of <= in bounds checks, but 0 might be a bit problematic.

Speaking of which, is it possible to detect click out of bounds, so that posX will be for example -1?

[latekvo]

Are you sure we want to do that?
To me it seems the simplified version, especially the return statement, is much less readable.
The if statement also has a double purpose of triggering the smart-cast of view from View to SvgView.
Without the if statement, I'll have to explicitly cast the view to SvgView, which will also contribute to making the code less readable.

[latekvo]

Also it would be nice to use .. instead of <= in bounds checks, but 0 might be a bit problematic.

Thankfully the ranges in Kotlin are inclusive, so 0 won't cause any issues.

Done in 543a8fe

[latekvo]

Speaking of which, is it possible to detect click out of bounds, so that posX will be for example -1?

I think that should be currently possible, the isWithinBounds call is executed for every gesture handler no matter it's position on the screen.

Are you asking about this in context of hitSlop?
Most SVG shapes aren't rectangular, so to implement hitSlop for them it'll likely require a lot more work, as the hitSlops will also have to be non-rectangular.

[m-bert]

To me it seems the simplified version, especially the return statement, is much less readable.

For me it doesn't make much of a difference if we use pressedId == view.id instead of hasBeenPressed, it is simple enough to understand and I wouldn't call it much less readable.

The if statement also has a double purpose of triggering the smart-cast of view from View to SvgView.

Okay, that complicates things a bit. Adding 4 more casts may not be the cleanest solution. I also don't like stuff like x = x as ... (at least not in TypeScript). As far as I can see Kotlin doesn't support union types, so I'm not sure if there's much that we can do (cc kotlin master @j-piasecki).

Are you asking about this in context of hitSlop?

No, I was just wondering whether we have to check if posX >= 0, since coordinates are view-relative.

[latekvo]

Adding 4 more casts may not be the cleanest solution

Only 1 cast will be necessary, but it's still not ideal.

No, I was just wondering whether we have to check if posX >= 0, since coordinates are view-relative.

I see. The received coordinates are often negative, so the posX >= 0 is indeed necessary.

For me it doesn't make much of a difference if we use pressedId == view.id instead of hasBeenPressed, it is simple enough to understand and I wouldn't call it much less readable.

It reduces the self-descriptiveness of both pressedId == view.id and pressedId in childrenIds, making it a bit harder to read, but i agree it's not that much of a difference.
If you prefer the simpler notation a lot, i'll change it right away.

[m-bert]

Only 1 cast will be necessary, but it's still not ideal.

Do you mean only in first occurrence of view? If it's only 1 cast than it sounds ok.

If you prefer the simpler notation a lot, i'll change it right away.

It is not a strong opinion, it is just a fact that I don't think it is necessary to add new variables for obvious stuff. If for you it becomes less readable than you can leave it.

[latekvo]

Do you mean only in first occurrence of view? If it's only 1 cast than it sounds ok.

Yes, only the first occurrence.

But as I said, just looking at hitTest() function, it's not in any way clear that view is limited to being VirtualView and SvgView without looking at both the usages of hitTest(), and the rest of RNSVGHitTester.
Having if (view is SvgView) and if (view is VirtualView) is the only clear indication within hitTest() that view can be only either one of those.
Removing that indication in favour of an early return for VirtualView, and then casting the view to SvgView in the rest of the code may seem arbitrary to someone unfamiliar with the code, so I would prefer to keep it the way it is now.

[m-bert]

(...) it's not in any way clear that view is limited to being VirtualView and SvgView (...)

I wouldn't say so since we are in the RNSVGHitTester file. Also, one line above hitTest there's isSvgElement method that also indicates which ones are from SVG.

The thing is, I don't like the idea of using if as cast if we can use cast directly (as you said, there's only one that is needed)

The if statement also has a double purpose of triggering the smart-cast of view from View to SvgView.

Now I'd say that it is its only purpose, am I right?

Removing that indication in favour of an early return for VirtualView, and then casting the view to SvgView in the rest of the code may seem arbitrary to someone unfamiliar with the code, so I would prefer to keep it the way it is now.

It is not really about early return, it is because this if is unnecessary. Also, even if it would be arbitrary, we can leave a comment explaining that these are the only two possibilities.

What's your stance on this, @j-piasecki?