Remove use of deprecated Integer constructor in NotifyDescriptor

[neilcsmith-net]

Removes use of deprecated Integer constructors for NotifyDescriptor constants. Alternative to #8407 for testing and discussion.

This change keeps the existing value relationship with JOptionPane int constants, which involves managing YES and OK now having the same identity. It is no longer possible to maintain both aspects, just choose the one that offers best compatibility.

Various usages in the NetBeans codebase suggest that keeping the values might lead to less hidden issues in our and third-party usage. eg.

netbeans/php/php.dbgp/src/org/netbeans/modules/php/dbgp/ServerThread.java

Line 113 in 2c92300

if (choice.equals(JOptionPane.YES_OPTION)) {

or

netbeans/php/php.smarty/src/org/netbeans/modules/php/smarty/editor/TplEditorSupport.java

Line 137 in 2c92300

showEncodingWarnings = (Integer) dd.getValue();

There's a mix in code that checks return values by equals and by identity. Having these work the same might be desirable.

The key place that not being able to differentiate OK and YES might cause problems is in the specification of custom options. The API does not actually say YES is a valid argument, but that is currently supported in the main implementation. This change assumes YES is wanted if NO is also present. Return values should work as before.

fixes #8257 (yey!)

[neilcsmith-net]

Marking ready for review but do not merge. This would be my preferred option to #8407 for handling the necessary changes for NotifyDescriptor options with upcoming value type changes. Both options keep to the API definition. Both options could break usage relying on implementation details (eg. linked code above). This one seems safer to me.

The test change is required because it will no longer be possible to specify both OK and YES in custom options. Hopefully people are not actually doing something so strange in real-world usage! It's also an implementation detail as YES and NO are not included in the range of supported options here documented in the spec

[mbien]

this essentially boils down to the question what type of client code we want to break. The introduction of value based objects has the potential to break things for everyone who expected them to have object identity.

in this case here:

a) There is code which follows the rules, looks at the API, sees keys of type Object and treats them as keys of undefined type. In that case, both choice.equals(JOptionPane.YES_OPTION) and (Integer) dd.getValue() instances would rely on unspecified impl details which were not mentioned anywhere in the doc. What you could see however is code which relies that OK and YES option would have different key Object identities.

b) And there is code which looks at the NB impl, or looks at the keys in the debugger, sees that they are of type Integer and that they were derived from JOptionPane keys and assumes impl==spec.

if we want to keep code of kind a) working, but are ok to break b) then we have to replace the new Integer() box, with something of arbitrary type but future proof, which behaves exactly like the old Integer box. Object identity wise (==) and follows the POJO rules (.equals() etc). (serialization is where things would get ugly)

If we say that too many looked at the source code and took impl details for granted (b)), we break the object identity of the keys and let YES==OK which wasn't the case until this change is made.

I do gravitate most of the time to the simpler solution, so I am slightly in favor of simply mapping everything to JOptionPane keys as done here, this will avoid breaking code of kind b) but has the (hopefully low) chance to break code of kind a). It always hurts a bit to break good behaving code, but in this case it might be the lesser evil. (when I worked on #8391 and #8407 I obviously tried the simple options first too as seen in the PR history, I just saw the failing tests as bad omen for code assumptions and compatibility)

[eirikbakke]

What timeframe are we expecting these Integer constructors to be removed at? Will it happen sooner than, say, 5 years from now?

I'm wondering if it might be better to wait until JEP 401 is actually stable, since the removal of constructors might then come with alternatives such as a drop-in static constructor.

Looking at the Javadoc for Java 25 EA, the constructors are still there: https://download.java.net/java/early_access/jdk25/docs/api/java.base/java/lang/Integer.html#%3Cinit%3E(int)

[mbien]

the removal of the constructors is IMO a distraction in this discussion since the behavior of Integer objects will change (it is just a symptom and isn't even set in stone). So it is not really about how long NB will still compile without errors.

The deprecation and javadoc notes on value objects describe what applications should do to be future proof. We shouldn't wait with making changes and fix every deprecation we can before they pile up - waiting till the last moment is never a good idea.

(Thread.stop() is deprecated since Java 1.2 and throws exceptions now and we still have usage! #4952)

[eirikbakke]

The deprecation and javadoc notes on value objects describe what applications should do to be future proof.

That's good for new code, but for existing APIs, perhaps it's better to just leave them as-is? I'd be surprised if JDK changes behavior in a way that is likely to break client applications.

(The example they give of a possible future change is that "synchronization may fail", but that would be a much more obscure use of an Integer instance.)

[mbien]

doc: the class's methods treat instances as freely substitutable when equal, meaning that interchanging any two instances x and y that are equal according to equals() produces no visible change in the behavior of the class's methods;

once Integer is a value based object, OK option will have the same == identity as the YES option, this PR is one way of addressing this issue before applications break.

We should offer builds compatible with that state well before that happens in a controlled manner so netbeans and netbeans based applications can be made future proof. Waiting makes no sense here IMO.

[eirikbakke]

(I haven't formed strong opinions on this, so please bear with me...)

once Integer is a value based object, OK option will have the same == identity as the YES option, this PR is one way of addressing this issue before applications break.

1. Is this the only thing that would break?
2. Is there ever a case where a dialog should have both a Yes button and an OK button? Typically you have yes/no/cancel or OK/cancel, but never yes/ok. The latter would be a bit of a usability bug IMO.

If the answer is (1) yes and (2) no, then I'd be inclined to agree with Neil's earlier position:

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

[mbien]

once Integer is a value based object, OK option will have the same == identity as the YES option, this PR is one way of addressing this issue before applications break.

Is this the only thing that would break?

well the little key identity crisis would already break our impl, the tests did notice it too although indirectly (thats why this PR has to change impl code when OK is made identical to YES). JEP 401 is worth reading btw, well written including ASCII art.

If the constructors would get removed our build would fail and NB 27 based apps will throw Exceptions on newer JDKs since NotifyDescriptor would fail during static class initialization phase. There is now a chance that constructors will stay though.

Set.of(NotifyDescriptor.YES_OPTION, NotifyDescriptor.OK_OPTION).contains(NotifyDescriptor.OK_OPTION) will consistently throw exceptions with all proposals and on all timelines :)

so I am +1 for Neil's proposal assuming nothing shows up during testing. But I think we should update the doc to mention that the key values are boxed JOptionPane keys to "become clean" API-wise.

I would be curious if changing the keys from Object to Integer would be a compatible change? (I would have to read up on that - it might be) This would have a self-documenting effect.

[neilcsmith-net]

What a lot of discussion! 😄

Let's ignore any usage of these values outside of the Dialog API and our implementation. For any other purposes they're unknown Objects that might be equal or identical to each other, and this is not specified anywhere.

The values here serve two purposes - input (custom options, etc.) or output (return value) from the Dialog API.

On the return values, the only need to differentiate the identities of OK and YES is if the Dialog can offer both. This is not supported by the standard option sets, and was only supported if you passed both in as custom options. YES and NO are not specified as supported custom options in the API, although they were supported in the underlying implementation in NbPresenter. If you have a dialog that currently has both OK and YES this change might break it, but if you're doing that you're ignoring the API spec, and quite frankly have bigger UI concerns!

The NbPresenter implementation code @mbien linked above is changed in this PR to prefer the API supported option (OK) while trying to keep the implementation details of supporting YES and NO as much as possible. That's probably preferable to following the spec to the letter and removing all support for YES and NO there.

That change fixed some of the test issues. It is also worth considering what broke in those tests. The return value test passed, the test failed on the button text. In a lot of the rare cases where this change might affect an application it should be a wrongly labelled button, not functionality.

I considered adding some documentation to clarify what the values, identities are. I decided not to at this stage because it potentially locks down the API spec and it might be better to still allow ourselves further leeway in case of regressions. But open to changing that if you think we should?

[mbien]

going to link #8257 with this PR, because I believe those should be the last occurrences of boxing constructors in the (released) codebase. (assuming that #8808 is merged first which gets a few more)