[NotifyDescriptor] Refactor return values to not rely on Integer identity

[mbien]

extracted from https://github.com/apache/netbeans/pull/8391

• return values must mimic Integer instances and their equals contracts
• unit test used the wrong return value (int instead of Object)
• added unit test to demonstrate the problem

part of https://github.com/apache/netbeans/issues/8257

[mbien]

we could also move this to NB 27, since the large project-wide replacements are in, the smaller but more risky changes like this one here and the WeakSet PR could wait for more feedback.

[mbien]

OK, I'm struggling to understand the requirement for the complexity here?

compatibility. I have been accused of over engineering already (which I don't take lightly :P) but the first attempt was obviously to simply use Integer.valueOf(). This creates the situation that two return options having the same identity (which is not compatible) - this failed multiple tests only indirectly related to the module - this is never a good sign.

You can also easily come up with code where someone uses == if chains and would get a different result after such change.

But I am open for suggestions of course.

edit: moving this to NB 27

[neilcsmith-net]

Well, there's behavioural compatibility and API compatibility. I'd be curious to see which tests failed? The return options that would have the same identity should not appear together. Unless someone did some really weird things with custom options.

The harder part would be internally differentiating at places like https://github.com/apache/netbeans/blob/master/platform/core.windows/src/org/netbeans/core/windows/services/NbPresenter.java#L664 which would have to then take option type into account, slightly changing the existing semantics. Or never accepting Yes or No, which matches the API anyway! https://bits.netbeans.org/25/javadoc/org-openide-dialogs/org/openide/DialogDescriptor.html#DialogDescriptor-java.lang.Object-java.lang.String-boolean-java.lang.Object:A-java.lang.Object-int-org.openide.util.HelpCtx-java.awt.event.ActionListener-

If we're happy to lose the integer relationship, which is already a minor behavioural incompatibility, surely five distinct Strings would be enough? Is comparability also important somewhere?

[mbien]

comparing strings with == opens a different can of worms. The second attempt after https://github.com/apache/netbeans/pull/8391#discussion_r2027026494 used an enum - enums were made for this.

If we go only for API compatibility -> n times new Object() is sufficient since that is exactly what the API says. But this will break client code.

[neilcsmith-net]

Yes, although the can of worms was opened using == with Object here in the first place! I'm not sure I saw why the enum approach failed. As I said, I'm still slightly inclined to keeping integers and fixing the displayers. But you decide. Some client code will break somewhere, but if it's not written in the docs ... :smile: And I was specifically looking at the Javadoc of that constructor btw, which is somewhat different to the current behaviour.

[neilcsmith-net]

I'm not sure where this one is at. Removing milestone - retarget when ready.

[mbien]

not sure what to do here, Changing a value is also something to consider. This may break applications which compare it with equals.

    /** Return value if YES is chosen. */
    public static final Object YES_OPTION = JOptionPane.YES_OPTION;

    /** Return value if NO is chosen. */
    public static final Object NO_OPTION = JOptionPane.NO_OPTION;

    /** Return value if CANCEL is chosen. */
    public static final Object CANCEL_OPTION = JOptionPane.CANCEL_OPTION;

    /** Return value if OK is chosen. */
    public static final Object OK_OPTION = 3; // was new Integer(JOptionPane.OK_OPTION) but must be distinct from YES_OPTION

    /** Return value if user closes the window without pressing any button. */
    public static final Object CLOSED_OPTION = JOptionPane.CLOSED_OPTION;

It has similar issues to using an Enum or new Object(), but would allow Integer casts.

If we want to assertEquals(YES_OPTION, OK_OPTION) while both having distinct identity for == to not break (NbPresenter and possibly apps), we have to go the ReturnValue with custom equals etc.

[neilcsmith-net]

I'm still of the opinion that YES_OPTION == OK_OPTION is the least worst fix, and aligns with JOptionPane.

It might require some tweaking of code and/or checking on presence of NO_OPTION to decide which to display. Using Yes without No seems odd anyway?!

[mbien]

the fact that YES_OPTION == OK_OPTION requires code change within the NB impl is a bad sign regarding compatibility - I would rather want to break the API than letting apps silently open the wrong dialog or OK buttons not work.

Aligning it with JOptionPane is a minor (impl) detail IMO because the keys are of type Object. In fact JOptionPane.YES_OPTION == NotifyDescriptor.YES_OPTION is false today. Not sure if it is a good argument for alignment with JOptionPane.

[neilcsmith-net]

I'd rather break compatibility inside the NB impl than outside it!

And I didn't mean align with JOptionPane in terms of ==, but in terms of having no real need to differentiate between OK and YES.

[mbien]

I'd rather break compatibility inside the NB impl than outside it!

As if this would be so simple. From the API perspective, the most compatible approach is likely the ReturnValue record, unless someone looked at the code and saw that those keys were Integers (but that is the impl perspective and technically no longer API). All explored options so far will break something.

[neilcsmith-net]

As if this would be so simple.

Well, probably!

From the API perspective, the most compatible approach is likely the ReturnValue record, unless someone looked at the code and saw that those keys were Integers ...

Which we know is happening!

As far as I can tell the problem is only removal of the ability to differentiate YES and OK? Or is there another overlap I'm missing?

What is the likelihood of someone needing to differentiate those two return values from a single dialog? There is a way to have both in one dialog, but not specified in the API as far as I can tell? Nor does the API state which values are unique?

This solution seems overly complex for an issue that seems hypothetical. Have you an example of actual usage that demonstrates where just using the integers will fail?

[mbien]

What is the likelihood of someone needing to differentiate those two return values from a single dialog? There is a way to have both in one dialog, but not specified in the API as far as I can tell? Nor does the API state which values are unique? This solution seems overly complex for an issue that seems hypothetical. Have you an example of actual usage that demonstrates where just using the integers will fail?

thinking about hypothetical apps was too big of an unknown for me given how much usage this API has (I did also grep the usage within NB). What if someone puts those things into a Set, calls contains()? I don't know.

I approached this completely differently: I tried to update it in a way to not break API, NB, and the tests. Then I wrote an extra test purely from the object identity perspective and made it pass before and after this change. From a API perspective this has the highest compatibility I could come up with, unless someone takes impl details for granted, e.g that the Object is an Integer as previously mentioned.

ReturnValue is the only key I could come up with where everything was green.

I tried: new Object() or enum, Integer.valueOf(), and YES=OK, possibly other versions in https://github.com/apache/netbeans/pull/8391. I intentionally did not go around and rewrite NB code since this would indicate that there is a certain chance that an app would break too.

This solution seems overly complex ...

I would take the internal complexity hit of a record as key if it reduces the risk of obscure failures at application runtime.

As if this would be so simple.

Well, probably!

I am going to close this so that someone else can take this over, ideally before the build fails due to JDK api removal.

[neilcsmith-net]

thinking about hypothetical apps was too big of an unknown for me given how much usage this API has

Yes, but there are non-hypothetical examples where this approach fails.

Have opened #8799 so at least there's something to compare approaches with. It would be useful to know exactly which tests failed when you tried this? Let's at least look at why and what the failures mean.

What if someone puts those things into a Set, calls contains()? I don't know.

API compliant Set or identity Set? :smile:

[mbien]

Yes, but there are non-hypothetical examples where this approach fails.

serialization was one of my concerns, I don't know what yours was since you didn't mention it yet.

API compliant Set or identity Set? 😄

any collection. We do not know where those keys are used.

[neilcsmith-net]

serialization was one of my concerns, I don't know what yours was since you didn't mention it yet.

I've mentioned a few, one of which I think Eirik also mentioned, here and in the Slack conversation on this, as well as my PR.

I'm not sure how serialization is going to be any more or less broken by int values. And anything relying on serialized dialog values is probably a little suspect already.

API compliant Set or identity Set? 😄

any collection. We do not know where those keys are used.

They're already equal, so any collection based on equality should behave the same.