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

[Prasanta Sadhukhan]

On Tue, 20 Aug 2024 04:30:25 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> 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();

same spacing issue in other places too as mentioned above...

> 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

ok

> 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` ?

ok

> 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())
> `

No, we need to break if frame is enabled and not look for its parent..

> 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();

other places in same method has same spacing...they can be done under one cleanup issue..

> test/jdk/javax/swing/JScrollBar/DisableFrameFromScrollBar.java line 117:
> 
>> 115:                 do {
>> 116:                     Thread.sleep(200);
>> 117:                 } while(isAdjusting && !doCheck);
> 
> Suggestion:
> 
>                 } while (isAdjusting && !doCheck);

ok

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722762061
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722761162
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722761309
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722761821
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722761077
PR Review Comment: https://git.openjdk.org/jdk/pull/20346#discussion_r1722762234