On Tue, 19 Mar 2024 01:14:29 GMT, Alexander Zvegintsev <azveg...@openjdk.org> wrote:
>> we need to add next to Pass/Fail a "Pause Timer" button, that > (a) stops the count down > (b) changes the Pause Timer to "Resume Timer" > ~~(c) disables Pass/Fail until the timer is resumed~~ > the test will not have to pause or be aware - only the PassFailJFrame > machinery. > ~~So the tester can do anything they want except exit the test in the paused > mode.~~ > > This PR implements everything except the (c). > I see nothing wrong with completing the test from the paused state, and it > does not add an extra click. > > > Example screenshots: > >  >  Looks good overall. However, the _Pause Timer_ is quite large, it's larger than Pass or Fail button, and it attracts more attention. I would place the Pause button into the timeout panel at the top. Yet the button is larger than the label. The font may be reduced, a symbol '⏸' (U+23F8) and '⏵' (U+23F5) could be used (see [Media control symbols](https://en.wikipedia.org/wiki/Media_control_symbols)), or an image icon with a tooltip. The timeout and its UI elements — the label and pause button — may be encapsulated into `TimeoutHandler` and return a `JPanel`. Yet the above improvements could be handled separately. I agree there should be a way to pause the test. I agree that Pass and Fail button can remain enabled, the tester could proceed with the test even when the timer isn't running. test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 647: > 645: private static final String PAUSE_BUTTON_LABEL = "Pause Timer"; > 646: private static final String RESUME_BUTTON_LABEL = "Resume Timer"; > 647: private long endTime; Suggestion: private static final String RESUME_BUTTON_LABEL = "Resume Timer"; private long endTime; test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 648: > 646: private static final String RESUME_BUTTON_LABEL = "Resume Timer"; > 647: private long endTime; > 648: private long pauseTimeLeft = 0L; Suggestion: private long pauseTimeLeft; Zero is the default value; you never use `pauseTimeLeft` before assigning it a value when the Pause button is pressed. test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 704: > 702: } else { > 703: > label.setText(label.getText().replace(PAUSED_LABEL_SUFFIX, "")); > 704: endTime = System.currentTimeMillis() + pauseTimeLeft; I'd rather call `updateTime(pauseTimeLeft)` here instead of editing the text. Suggestion: endTime = System.currentTimeMillis() + pauseTimeLeft; updateTime(pauseTimeLeft); ------------- Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18368#pullrequestreview-1949607461 PR Review Comment: https://git.openjdk.org/jdk/pull/18368#discussion_r1532523214 PR Review Comment: https://git.openjdk.org/jdk/pull/18368#discussion_r1532529917 PR Review Comment: https://git.openjdk.org/jdk/pull/18368#discussion_r1532538482
