getsentry / team-mobile

Meta issues for the Mobile team
4 stars 1 forks source link

Support View hierarchy #64

Closed marandaneto closed 1 year ago

marandaneto commented 2 years ago

Spec: User facing docs: RFC: https://github.com/getsentry/rfcs/blob/main/text/0033-view-hierarchy.md

### Platforms
- [X] https://github.com/getsentry/sentry-java/pull/2440
- [X] https://github.com/getsentry/sentry-cocoa/issues/2467
- [x] https://github.com/getsentry/sentry-react-native/pull/2708
- [X] https://github.com/getsentry/sentry-dart/pull/1189
- [ ] https://github.com/getsentry/sentry-unity/pull/1169

@markushi DRI

Old issue: https://github.com/getsentry/team-mobile/issues/9

ueman commented 2 years ago

Feel free to just copy my code. It's not really tested, neither via unit tests nor in production. So make sure to do some throughout testing 😅 But as far as I can tell it's really promising.

ueman commented 2 years ago

Some more thoughts on my code:

The view hierarchy in Flutter is way deeper than those of Android and iOS. Classes of views (views are called widgets in Flutter) prefixed with a _ are private (that's how it works in Dart). It might be worth to ignore private classes since you most probably can't look them up in docs and so on. Public children of private classes should be shown again, though. Private classes are often used to improve the structuring of a widget, so they are kinda an implementation detail. Classes not prefixed with an underscore are public and one should be able to look them up in docs. Ignoring private classes is currently not done in my code. Also, you don't know whether a class is from user code, 3-party code or framework code, since there's no reflection/introspection in Dart. When enabling obfuscation in Flutter, the class names are of course not really human readable, but I guess that's similar for Android/iOS.

In addition to the names of the widgets, you could also add the content of well known widgets like Text and *Buttons, as well as the on screen size and coordinates. That's pretty easily added, but also currently not done.

markushi commented 2 years ago

@bitsandfoxes since we already have a good idea how a View hierarchy looks on the mobile platforms could you paste an example for Unity as well? It would be interesting to see if it makes sense to have a format which works for both 2D and 3D

krystofwoldrich commented 2 years ago

After a short research, I've not found a way to retrieve the React Components hierarchy. The main reason is the absence of DOM as it is on the web. There is a nice description of what is happening in RN here https://stackoverflow.com/a/48916954, but I don't know how/if possible to access the rendered nodes tree. (There might be this information in UIManager somewhere, but it would require a lot of research, great presentation on how shadow tree works)

The RN implementation will rely on the native implementations.

On the Legacy RN Arch the mapping will be one-to-one. On the New Arch Fabric Rendere is doing View Flattening which means layout-only components get squashed. Therefore the mapping won't be one-to-one, but the native hierarchy still should be useful and clear.

RN will need a method to get the view hierarchy, something like captureViewHierarchy. We will enhance the event with this content and send it back to the native layers as we do now.

bitsandfoxes commented 2 years ago

The primary motivation for Unity would be irrespective of 2D vs 3D but about the additional data and the context stored within the hierarchy. Unity uses scenes (which are basically trees) to manage everything that happens in a game. The whole workflow within the Unity editor is based on the hierarchy and components are added/subtracted on that tree.

Screenshot 2022-10-25 at 16 22 59
marandaneto commented 1 year ago

@markushi file name is mandatory but not part of the RFC, should we agree on view_hierarchy.json?

markushi commented 1 year ago

@marandaneto good point! We should definitely align that. As far as I can see iOS is using view-hierarchy.json right now (- instead of _). I guess we don't have any convention here right? cc @brustolin

brustolin commented 1 year ago

Yeap, I don't see why not. The name should not matter either way, because we have an special attachment_type to use.

marandaneto commented 1 year ago

That's fine, happy with view-hierarchy.json. Can we amend the RFC?

marandaneto commented 1 year ago

Something yet not discussed, the name of the flag to enable/disable the feature and the default state. I'd recommend attachViewHierarchy and disabled for now, wdyt?

Another point is when to attach the view hierarchy, I suggest similar to screenshots, only when the event has an error, wdyt?

markushi commented 1 year ago

Alright, let's stick with view-hierarchy.json - I'll open a PR for the RFCs.