On Tue, 19 Nov 2019 18:02:23 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> On Mon, 18 Nov 2019 18:12:13 GMT, Kevin Rushforth <k...@openjdk.org> wrote: > >> On Wed, 13 Nov 2019 23:47:58 GMT, Kevin Rushforth <k...@openjdk.org> wrote: >> >>> On Wed, 6 Nov 2019 12:54:40 GMT, Kevin Rushforth <k...@openjdk.org> wrote: >>> >>>> On Wed, 6 Nov 2019 11:50:27 GMT, Nir Lisker <nlis...@openjdk.org> wrote: >>>> >>>>> On Wed, 6 Nov 2019 07:12:26 GMT, Robin Westberg <rwestb...@openjdk.org> >>>>> wrote: >>>>> >>>>>> On Wed, 6 Nov 2019 05:07:56 GMT, Kevin Rushforth <k...@openjdk.org> >>>>>> wrote: >>>>>> >>>>>>> On Wed, 6 Nov 2019 05:04:28 GMT, Kevin Rushforth <k...@openjdk.org> >>>>>>> wrote: >>>>>>> >>>>>>>> On Tue, 5 Nov 2019 18:10:57 GMT, Nir Lisker <nlis...@openjdk.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8229472 >>>>>>>>> >>>>>>>>> CSR will be created after the changes are approved. >>>>>>>>> >>>>>>>>> ---------------- >>>>>>>>> >>>>>>>>> Commits: >>>>>>>>> - 6d29e034: Initial push of 8229472 >>>>>>>>> >>>>>>>>> Changes: https://git.openjdk.java.net/jfx/pull/30/files >>>>>>>>> Webrev: https://webrevs.openjdk.java.net/jfx/30/webrev.00 >>>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8229472 >>>>>>>>> Stats: 14 lines in 7 files changed: 7 ins; 0 del; 7 mod >>>>>>>>> Patch: https://git.openjdk.java.net/jfx/pull/30.diff >>>>>>>>> Fetch: git fetch https://git.openjdk.java.net/jfx >>>>>>>>> pull/30/head:pull/30 >>>>>>>> >>>>>>>> modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanBooleanPropertyBuilder.java >>>>>>>> line 68: >>>>>>>> >>>>>>>>> 67: @Deprecated(since = "14", forRemoval = true) >>>>>>>>> 68: public JavaBeanBooleanPropertyBuilder() {} >>>>>>>>> 69: >>>>>>>> >>>>>>>> Minor: I checked other places in JavaFX and JDK, and they consistently >>>>>>>> omit the spaces surrounding the `=`. >>>>>>> >>>>>>> I changed the bug summary to include `for removal` in the title. Can >>>>>>> you change the PR title to match? >>>>>> >>>>>> Thanks for the notification, looks like GitHub returned 500 for a few >>>>>> minutes. This seem to happen from time to time, so nice to know that the >>>>>> retry logic works. :) >>>>> >>>>> For both `since` and `forRemoval`? >>>> >>>> Yes. The pattern used consistently (in all cases that I could find) would >>>> be: >>>> >>>> @Deprecated(since="14", forRemoval=true) >>> >>> @arapte can you also review this? >> >>> Please add @deprecated javadoc tag in the description for all constructors. >> >> Good idea. > > Good suggestion. I will change to > > /** > * @deprecated This constructor was exposed erroneously and will be removed in > the next version. Use {@link #create()} instead. > */ > @Deprecated(since="14", forRemoval=true) > > I noticed that the `ReadOnlyJavaBeanXxxPropertyBuilder`s also have the same > issue. Can I fix them in this PR too or do we need a new one? > > Also, the CSR is finalized, can I change it at this point? > I noticed that the ReadOnlyJavaBeanXxxPropertyBuilders also have the same > issue. Can I fix them in this PR too or do we need a new one? Good catch. Not sure how we missed this before now (the change for JavaFX 13 also missed it), but yes, I'd prefer to fix this now for the ReadOnlyJavaBeanXxxPropertyBuilders, too. For the CSR, you can move it back to Draft, edit it, and then once the review is done, Finalize it again (no need to go through the Proposed phase a second time). PR: https://git.openjdk.java.net/jfx/pull/30
