On Fri, 10 Feb 2023 11:46:22 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org> wrote:
>> SwingWorker done() method [spec >> ](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/SwingWorker.java#L452) >> says "Executed on the Event Dispatch Thread after the doInBackground method >> is finished" >> but there's no mechanism in place to honor that claim. >> The >> [spec](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/SwingWorker.java#L289) >> also says the state should be DONE after doInBackground() returns which is >> also not done. >> >> Modified the code to honour the specification. > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix and test updated > - Fix and test updated The changes look good except for the minor comments. The updated fix doesn't require the CSR. Please update the copyright year. src/java.desktop/share/classes/javax/swing/SwingWorker.java line 313: > 311: }; > 312: > 313: future = new FutureTask<T>(callable); Suggestion: future = new FutureTask<T>(callable); An extra space breaks the correct indentation. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 43: > 41: private static final int WAIT_TIME = 200; > 42: private static final long CLEANUP_TIME = 1000; > 43: private static final CountDownLatch doneLatch = new CountDownLatch(1); Suggestion: private static final int WAIT_TIME = 200; private static final long CLEANUP_TIME = 1000; private static final AtomicBoolean doInBackgroundStarted = new AtomicBoolean(false); private static final AtomicBoolean doInBackgroundFinished = new AtomicBoolean(false); private static final CountDownLatch doneLatch = new CountDownLatch(1); Separate the real constants from the objects which track the state. The flags should also be marked `final`. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 57: > 55: System.out.println("Got interrupted!"); > 56: } > 57: try { Maybe add a blank line between the two try-catch blocks? And before `return`-statement? test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 73: > 71: if (!doInBackgroundFinished.get()) { > 72: throw new RuntimeException("done called before " + > 73: " doInBackground is > finished"); Suggestion: throw new RuntimeException("done called before " + "doInBackground is finished"); Add two spaces before the quote and to align it to the one on the line above; remove one space after the quote so that the error message doesn't have two consecutive spaces. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 99: > 97: "PropertyChangeListeners called with " + > 98: "state STARTED before doInBackground is > finished"); > 99: } I think one condition is enough. I propose moving the state to be the first condition. if (worker.getState() == SwingWorker.StateValue.STARTED && (doInBackgroundStarted.get() || doInBackgroundFinished.get())) { We start with *the state* and ensure both `doInBackgroundStarted` and `doInBackgroundFinished` are `false`. Yes, the error message can't be very specific in this case. --- Java Code Style suggests binary operators should be wrapped to the next line rather than left on the previous line. I do prefer this style because it highlights *it is a continuation line* rather than a new statement. I know it isn't followed consistently though. Nor do we have an updated code style guide in the clientlibs that we adhere when writing new code. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 100: > 98: "state STARTED before doInBackground is > finished"); > 99: } > 100: // After the doInBackground method is finished Adding a blank line between the conditions would visually separate them. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 104: > 102: // property change to StateValue.DONE > 103: if (doInBackgroundFinished.get() && > 104: worker.getState() != > SwingWorker.StateValue.DONE) { The condition here should take both flags into account. Again, I propose moving the state condition to be the first one. if (worker.getState() != SwingWorker.StateValue.DONE && doInBackgroundFinished.get()) { We may also check that `doInBackgroundStarted` is `true`. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 128: > 126: " getState " + worker.getState()); > 127: if (doInBackgroundFinished.get() && > 128: worker.getState() != SwingWorker.StateValue.DONE) { Suggestion: if (doInBackgroundFinished.get() && worker.getState() != SwingWorker.StateValue.DONE) { If you follow the style of double indentation on continuation lines, two more spaces should be added. Consider moving the `&&` operator to the continuation line. test/jdk/javax/swing/SwingWorker/TestDoneBeforeDoInBackground.java line 130: > 128: worker.getState() != SwingWorker.StateValue.DONE) { > 129: throw new RuntimeException("doInBackground is finished " + > 130: " but State is not DONE"); Suggestion: "but the State is not DONE"); Two consecutive spaces in an error message are redundant, one is enough. The article seems missing? ------------- Changes requested by aivanov (Reviewer). PR: https://git.openjdk.org/jdk/pull/11940
