Skip to content

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

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

Merged
merged 3 commits into from
Apr 22, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36a246a
[Mono.Android] Hide "internal" members of JLO from IDEs.
jpobst Apr 20, 2020
bd43205
Address feedback, add Java.Lang.Throwable.
jpobst Apr 21, 2020
e9c9213
Add release notes.
jpobst Apr 22, 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
17 changes: 17 additions & 0 deletions Documentation/release-notes/4583.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
### Tidier IntelliSense suggestions for Java.Lang.Object subclasses

`Java.Lang.Object` contains several properties and methods that are
required to be `public` to support bindings, but are not intended
to be called by users. Use `[EditorBrowser]` to hide them from
IntelliSense, making it easier to find useful members.

[IntelliSense cleanup)[images/4583.png]

### 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.
Binary file added Documentation/release-notes/images/4583.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 15 additions & 0 deletions src/Mono.Android/Java.Lang/Object.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using Java.Interop;

using Android.Runtime;
using System.ComponentModel;
using System.Diagnostics;

namespace Java.Lang {

Expand Down Expand Up @@ -91,21 +93,27 @@ internal void SetHandleOnDeserialized (StreamingContext context)
}

#if JAVA_INTEROP
[EditorBrowsable (EditorBrowsableState.Never)]
public int JniIdentityHashCode {
Copy link
Contributor

@jonpryor jonpryor Apr 20, 2020

Choose a reason for hiding this comment

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

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.

Copy link

@bramborman bramborman Apr 21, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jonpryor jonpryor Apr 22, 2020

Choose a reason for hiding this comment

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

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.

Copy link

@bramborman bramborman Apr 22, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jpobst jpobst Apr 22, 2020

Choose a reason for hiding this comment

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

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.

Copy link

@bramborman bramborman Apr 22, 2020

Choose a reason for hiding this comment

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

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

get {return (int) key_handle;}
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
public JniObjectReference PeerReference {
get {
return new JniObjectReference (handle, (JniObjectReferenceType) handle_type);
}
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
public virtual JniPeerMembers JniPeerMembers {
get { return _members; }
}
#endif // JAVA_INTEROP

[EditorBrowsable (EditorBrowsableState.Never)]
public IntPtr Handle {
Copy link
Member

@jonathanpeppers jonathanpeppers Apr 20, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jpobst jpobst Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jonathanpeppers jonathanpeppers Apr 20, 2020

Choose a reason for hiding this comment

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

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

get {
if (weak_handle != IntPtr.Zero)
Expand All @@ -115,10 +123,14 @@ public IntPtr Handle {
}
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
protected virtual IntPtr ThresholdClass {
get { return Class.Object; }
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
protected virtual System.Type ThresholdType {
get { return typeof (Java.Lang.Object); }
}
Expand Down Expand Up @@ -154,6 +166,7 @@ void IJavaPeerable.DisposeUnlessReferenced ()
}
}

[EditorBrowsable (EditorBrowsableState.Never)]
public void UnregisterFromRuntime ()
{
JNIEnv.AndroidValueManager.RemovePeer (this, key_handle);
Expand Down Expand Up @@ -231,6 +244,7 @@ internal static void Dispose (IJavaPeerable instance, ref IntPtr handle, IntPtr
}
}

[EditorBrowsable (EditorBrowsableState.Never)]
protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
{
JNIEnv.AndroidValueManager.AddPeer (this, value, transfer, out handle);
Expand Down Expand Up @@ -287,6 +301,7 @@ internal static IJavaPeerable GetObject (IntPtr handle, JniHandleOwnership trans
return Java.Interop.TypeManager.CreateInstance (handle, transfer, type);
}

[EditorBrowsable (EditorBrowsableState.Never)]
public T[] ToArray<T>()
{
return JNIEnv.GetArray<T>(Handle);
Expand Down
13 changes: 13 additions & 0 deletions src/Mono.Android/Java.Lang/Throwable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using Java.Interop;

using Android.Runtime;
using System.ComponentModel;
using System.Diagnostics;

namespace Java.Lang {

Expand Down Expand Up @@ -171,27 +173,37 @@ public override string ToString ()
}

#if JAVA_INTEROP
[EditorBrowsable (EditorBrowsableState.Never)]
public int JniIdentityHashCode {
get {return (int) key_handle;}
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
public JniObjectReference PeerReference {
get {
return new JniObjectReference (handle, (JniObjectReferenceType) handle_type);
}
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
public virtual JniPeerMembers JniPeerMembers {
get { return _members; }
}
#endif // JAVA_INTEROP

[EditorBrowsable (EditorBrowsableState.Never)]
public IntPtr Handle { get { return handle; } }

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
protected virtual IntPtr ThresholdClass {
get { return class_ref; }
}

[DebuggerBrowsable (DebuggerBrowsableState.Never)]
[EditorBrowsable (EditorBrowsableState.Never)]
protected virtual System.Type ThresholdType {
get { return typeof (Java.Lang.Throwable); }
}
Expand Down Expand Up @@ -226,6 +238,7 @@ public unsafe Java.Lang.Class Class {
}
}

[EditorBrowsable (EditorBrowsableState.Never)]
protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
{
JNIEnv.AndroidValueManager.AddPeer (this, value, transfer, out handle);
Expand Down