[Mono.Android] Hide "internal" members of JLO from IDEs. #4583

jpobst · 2020-04-20T16:49:13Z

Java.Lang.Object contains various public and protected members that are required by our bindings, such as J.L.Object.Handle and J.L.Object.PeerReference. We cannot make them internal because our bindings need them, but we can make them "invisible" to users using IDEs by applying [EditorBrowsable (Never)] and [DebuggerBrowsable (Never)]. This will prevent them from showing up in Intellisense and debugger displays, as they should not be used by users.

Note they are still usable if needed, they simply do not show up in Intellisense. You have to type them out manually.

Intellisense for a class deriving JLO before:

After:

jonathanpeppers · 2020-04-20T19:08:32Z

src/Mono.Android/Java.Lang/Object.cs


+		[DebuggerBrowsable (DebuggerBrowsableState.Never)]
+		[EditorBrowsable (EditorBrowsableState.Never)]
 		public IntPtr Handle {


Would this also hide usage of Handle like:

private Java.Lang.Object foo; //... if (foo != null && foo.Handle != IntPtr.Zero) { //... }

If so, maybe we shouldn't hide Handle as developers might often use it?

Xamarin.Forms uses Handle in these extension methods a lot:

https://github.com/xamarin/Xamarin.Forms/blob/954cc8b0f174f179fd3395b4b1f257201f9701af/Xamarin.Forms.Platform.Android/Extensions/JavaObjectExtensions.cs#L7-L31

Yes, it would hide Handle in that case (from Intellisense). My gut says users wouldn't be using Handle if they are writing applications or libraries. (The authors of XF itself are a special case.)

But that's just an assumption and it's certainly up for discussion.

You also might use Handle if you are working with JNI directly? Some folks might do that?

jonpryor · 2020-04-20T20:25:34Z

src/Mono.Android/Java.Lang/Object.cs

 #if JAVA_INTEROP
+		[DebuggerBrowsable (DebuggerBrowsableState.Never)]
+		[EditorBrowsable (EditorBrowsableState.Never)]
 		public int JniIdentityHashCode {


I would think that when debugging you would want something to designate "object identity", e.g. to easily answer "are these two variables referring to the same instance"?

Is this (still?) a desirable/useful feature? If so, what property would best be expose to the debugger? JniIdentityHashCode is useful if there are *separate Java.Lang.Object instances wrapping the same Java instance, while Handle and PeerReference are good for seeing distinct wrappers.

Agree, but just wondering whether that couldn't be implemented in VS debugger the same way it is for .NET objects? This way we wouldn't have to check complex ids and hashes and it would feel more native to devs imo.

I didn't know that was a feature that existed.

That said, I assume that Make Object ID only works for identical instances. If you have two different C# instances which wrap the same Java instance -- presumably a rare but still possible scenario -- then Make Object ID wouldn't help. JniIdentityHashCode could.

Well that's not exactly what I mean... I mean if there could be added a new similar functionality, probably another context menu option specialized for this usecase, something like Make Java object ID that wouldn't care about the .NET instance but about the native Java instance probably using the mentioned JniIdentityHashCode property. That would make looking for Java instance equality much easier compared to writing down some hash codes or so. I've heard something that in .NET 5 there should me a more native interoperability with Java (and I believe that Java.Interop is the thing to be used for that?) so if that's true, this change would benefit not only Xamarin.Android users.

I suspect the primary reason .NET has Make Object ID is because .NET objects do not inherently have an ID and thus you cannot tell if 2 objects are the same. We already have that information easily available with these fields. In essence, they had to build a workaround for something we already have.

Adding Make Java object ID for both VS2019 and VSMac would be a decent amount of work for a different team (Android IDE team). I suspect it will be placed in their backlog at much lower priority than the other tasks they have and probably never implemented.

As such, let's keep these properties visible to the debugger. Overall this is still a good win by cleaning up the IntelliSense dropdown and some of the Debugger window.

You're right, the effort wouldn't be worth the outcome especially since it's imo something devs don't do really often.

jonpryor · 2020-04-20T20:28:50Z

We should give Java.Lang.Throwable the same treatment.

jonpryor · 2020-04-20T20:59:35Z

src/Mono.Android/Java.Lang/Object.cs

 		}

 #if JAVA_INTEROP
+		[DebuggerBrowsable (DebuggerBrowsableState.Never)]


This property should be shown to the debugger.

jonpryor · 2020-04-20T20:59:51Z

src/Mono.Android/Java.Lang/Object.cs

 		}
 #endif  // JAVA_INTEROP

+		[DebuggerBrowsable (DebuggerBrowsableState.Never)]


This property should be shown to the debugger.

bramborman · 2020-04-21T00:12:43Z

Just remembered about the Android.Runtime.IJavaObject iface which could get the same treatment too

jpobst · 2020-04-21T15:08:54Z

Addressed feedback to let the debugger see a couple of instance identifiers, and added Java.Lang.Throwable.

The only member on IJavaObject is Handle. Presumably if you've casted your object to an IJavaObject it's probably ok to see Handle since there's no other reason to cast to IJavaObject.

brendanzagaeski

Draft release notes

This looks like a nice user-facing behavior improvement. After the full set of types to adjust is settled, when you get a chance, you can add a draft release note for it to Documentation/release-notes/4583.md based on the PR description. I think the screenshots from the PR description could be nice to include in the note too. They can go in Documentation/release-notes/images/ as 4583-before.png and 4583-after.png or any other names you like. Thanks much!

The note can optionally include a separate section for the issue fixed, like:

### Tidier IntelliSense suggestions for Java.Lang.Object subclasses

`Java.Lang.Object` contains...

### Issues fixed

#### IDE compatibility

  * [GitHub 4582](https://github.com/xamarin/xamarin-android/issues/4582):
    Some `Java.Lang.Object` properties and methods like `Handle` and
    `PeerReference` appeared in IntelliSense suggestions even though they were
    primarily intended for use only in generated code.

Fixes: #4582 `Java.Lang.Object` and `Java.Lang.Throwable` contain various `public` and `protected` members that are required by our bindings, such as the `Handle` and `PeerReference` properties. We cannot make them `internal` because our bindings need them, but we can make them "invisible" to users using IDEs by applying the attributes: * [`[EditorBrowsable (EditorBrowsableState.Never)]`][0] * [`[DebuggerBrowsable (DebuggerBrowsableState.Never)]`][1] This will prevent them from showing up in IntelliSense and debugger displays, as they should not be used by users. Note they are still usable if needed, they simply do not show up in IntelliSense. You have to type them out manually. Intellisense for a class deriving JLO before: ![before](https://user-images.githubusercontent.com/179295/79778416-4fc78000-82fe-11ea-840a-fb0b4688d6aa.png) After: ![after](https://user-images.githubusercontent.com/179295/79778430-535b0700-82fe-11ea-9ff6-d6e41eabcf9a.png) [0]: https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.editorbrowsableattribute [1]: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.debuggerbrowsableattribute

[Mono.Android] Hide "internal" members of JLO from IDEs.

36a246a

jpobst force-pushed the hide-object-members branch from 2188fc6 to 36a246a Compare April 20, 2020 16:52

jpobst marked this pull request as ready for review April 20, 2020 17:11

jpobst requested a review from jonpryor as a code owner April 20, 2020 17:11

jonathanpeppers reviewed Apr 20, 2020

View reviewed changes

jonpryor reviewed Apr 20, 2020

View reviewed changes

rolfbjarne mentioned this pull request Apr 21, 2020

Hide advanced NSObject members to follow Xamarin.Android dotnet/macios#8439

Closed

Address feedback, add Java.Lang.Throwable.

bd43205

brendanzagaeski suggested changes Apr 22, 2020

View reviewed changes

Add release notes.

e9c9213

jonpryor merged commit 3c94f6f into master Apr 22, 2020

jonpryor deleted the hide-object-members branch April 22, 2020 19:38

brendanzagaeski added this to the In Preview: 10.4 (d16-7) milestone May 19, 2020

github-actions bot locked and limited conversation to collaborators Jan 27, 2024

[Mono.Android] Hide "internal" members of JLO from IDEs. #4583

[Mono.Android] Hide "internal" members of JLO from IDEs. #4583

Uh oh!

Conversation

jpobst commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpobst Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Apr 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramborman commented Apr 21, 2020

Uh oh!

jpobst commented Apr 21, 2020

Uh oh!

brendanzagaeski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jpobst commented Apr 20, 2020 •

edited

Loading

jpobst Apr 20, 2020 •

edited

Loading