RFR: 6318027: BasicScrollBarUI does not disable timer when enclosing frame is disabled. [v5]

[Abhishek Kumar]

On Mon, 19 Aug 2024 02:25:29 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Issue is
>> BasicScrollBarUI.ArrowButtonListener starts a timer in mousePressed(), and stops it in mouseReleased(). If the frame containing the scrollbar is disabled between the MOUSE_PRESSED and the MOUSE_RELEASED events, the mouseReleased() method is never called. If the frame is then re-enabled, the still-running timer causes it to scroll all the way to the end.
>> Fix is to check if [ArrowButtonListener.handledEvent](https://github.com/openjdk/jdk/blame/ee839b7f0ebe471d3877cddd2c87019ccb8ee5ae/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java#L1567) is still set when ActionEvent is processed then stop the timer and reset this variable.
>> 
>> CI testing is green and also SwingSet2 JScrollPane scrolling with this modification..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   test typo

src/java.desktop/macosx/classes/com/apple/laf/AquaScrollBarUI.java line 534:

> 532:                 if (parent instanceof javax.swing.JFrame par) {
> 533:                     if (!par.isEnabled()) {
> 534:                         ((Timer)e.getSource()).stop();

Suggestion:

                        ((Timer) e.getSource()).stop();

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java line 1611:

> 1609:         /** {@inheritDoc} */
> 1610:         public void actionPerformed(ActionEvent e) {
> 1611:             // If frame is disabled and time is started in mousePressed

Suggestion:

            // If frame is disabled and timer is started in mousePressed

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java line 1616:

> 1614:             Component parent = scrollbar.getParent();
> 1615:             do {
> 1616:                 if (parent instanceof javax.swing.JFrame par) {

should we import `JFrame` and just use `instanceof JFrame` ?

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java line 1617:

> 1615:             do {
> 1616:                 if (parent instanceof javax.swing.JFrame par) {
> 1617:                     if (!par.isEnabled()) {

I think this two condition can be combined into one.

`if (parent instanceof javax.swing.JFrame par && !par.isEnabled())
`

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicScrollBarUI.java line 1618:

> 1616:                 if (parent instanceof javax.swing.JFrame par) {
> 1617:                     if (!par.isEnabled()) {
> 1618:                         ((Timer)e.getSource()).stop();

Suggestion:

                        ((Timer) e.getSource()).stop();

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java line 46:

> 44: import javax.swing.SwingUtilities;
> 45: import javax.swing.UIManager;
> 46: import javax.swing.UnsupportedLookAndFeelException;

Suggestion:

import javax.swing.SwingUtilities;
import javax.swing.UIManager;
import javax.swing.UnsupportedLookAndFeelException;
import javax.swing.event.ChangeEvent;
import javax.swing.event.ChangeListener;

test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java line 117:

> 115:                 do {
> 116:                     Thread.sleep(200);
> 117:                 } while(isAdjusting && !doCheck);

Suggestion:

                } while (isAdjusting && !doCheck);

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722670897
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722659957
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722662343
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722670563
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722661711
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722672027
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722673254