Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Wed, 26 Jun 2024 08:36:45 GMT, Prasanta Sadhukhan
wrote:
>> Alisen's concern is valid.
>>
>> What if `setEnabled(false)` is called when `isOneTouchExpandable` is `false`
>> and then `setOneTouchExpandable(true)` is called which adds the buttons? The
>> buttons are enabled when they should be disabled.
>
> Why buttons will be enabled? The buttons state is determined by `setEnabled
> `param, if `setEnabled `is false, then irrespective of
> `isOneTouchExpandable`, it will be disabled...`isOneTouchExpandable `is just
> extra layer of check as that determines whether arrow buttons are rendered or
> notIf oneTouchExpandable is not there then arrow buttons itself is not
> created so no point set their state...
Consider the following scenario:
JSplitPane jsp = new JSplitPane(JSplitPane.HORIZONTAL_SPLIT,
new JButton("Left"),
new JButton("Right"));
jsp.setOneTouchExpandable(false);
jsp.setEnabled(false);
// At this point neither `leftButton` or `rightButton` are created
jsp.setOneTouchExpandable(true);
// This has created both `leftButton` or `rightButton`,
// and both buttons are enabled, aren't they?
// I can't see how either button could get disabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654434650
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Wed, 26 Jun 2024 08:43:40 GMT, Prasanta Sadhukhan wrote: >> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java >> line 376: >> >>> 374: leftButton.setEnabled(enabled); >>> 375: } >>> 376: } >> >> Is it possible to override `isEnabled` in `rightButton` and `leftButton` so >> that it returns the state of `JSplitPane`? >> >> Alternatively, the buttons could install a `PropertyChangeListener` for >> `"enabled"` property and align their state to the host split pane. > > Right now, the buttons are always enabled and rendered when > isOneTouchExpandable is enabled without any scope to disable...We need this > code anyway to set/reset buttons state..I guess on top of this code, we can > add isEnabled if it is needed but for this issue, isEnabled is not needed.. This approach will automatically resolve [the scenario above](https://github.com/openjdk/jdk/pull/19695/files#r1646653721) where buttons may have inconsistent state. - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654437914
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Tue, 18 Jun 2024 12:19:39 GMT, Alexey Ivanov wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Omit gtk > > test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 100: > >> 98: robot.mouseMove(loc.x, loc.y); >> 99: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); >> 100: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); > > Isn't it enough to verify the state of the buttons whether they're enabled or > not. It should be simpler, it could be done without rendering the frame. Ok..modified to just check for button state... - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654415363
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Tue, 18 Jun 2024 12:17:41 GMT, Alexey Ivanov wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Omit gtk > > src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java > line 376: > >> 374: leftButton.setEnabled(enabled); >> 375: } >> 376: } > > Is it possible to override `isEnabled` in `rightButton` and `leftButton` so > that it returns the state of `JSplitPane`? > > Alternatively, the buttons could install a `PropertyChangeListener` for > `"enabled"` property and align their state to the host split pane. Right now, the buttons are always enabled and rendered when isOneTouchExpandable is enabled without any scope to disable...We need this code anyway to set/reset buttons state..I guess on top of this code, we can add isEnabled if it is needed but for this issue, isEnabled is not needed.. - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654393332
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Tue, 18 Jun 2024 12:13:49 GMT, Alexey Ivanov wrote: >> All L including Aqua extends BasicSplitPaneUI so it's ok...The existing >> test iterates through all L without any issue.. > >> All L including Aqua extends BasicSplitPaneUI so it's ok...The existing >> test iterates through all L without any issue.. > > That is true, yet it is still possible to set a L that doesn't extend > `BasicSplitPaneUI` and the updated code will throw `ClassCastException`. > > I'm sure such a situation is rare, if it exists at all, yet I don't think the > public API should have such a limitation: `JSplitPane.setUI` accepts > `SplitPaneUI`, so it is valid to pass an object that is not subclass of > `BasicSplitPaneUI`. ok...Will add check.. >> No, the left and right arrow buttons are created ONLY when >> `isOneTouchExpandable` is true so it should be checked here too to >> enable/disable actions > > Alisen's concern is valid. > > What if `setEnabled(false)` is called when `isOneTouchExpandable` is `false` > and then `setOneTouchExpandable(true)` is called which adds the buttons? The > buttons are enabled when they should be disabled. Why buttons will be enabled? The buttons state is determined by `setEnabled `param, if `setEnabled `is false, then irrespective of `isOneTouchExpandable`, it will be disabled...`isOneTouchExpandable `is just extra layer of check as that determines whether arrow buttons are rendered or notIf oneTouchExpandable is not there then arrow buttons itself is not created so no point set their state... - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654383055 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1654382971
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Mon, 17 Jun 2024 05:07:22 GMT, Prasanta Sadhukhan
wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
>> line 369:
>>
>>> 367: @Override
>>> 368: public void setEnabled(boolean enabled) {
>>> 369: if (splitPane.isOneTouchExpandable() &&
>>
>> i think for setEnabled the buttons should be enabled/disabled regardless of
>> if splitPane isOneTouchExpandable is true so that the state is preserved
>> even if setEnabled(false) is called before
>> setOneTouchExpandable(true)
>
> No, the left and right arrow buttons are created ONLY when
> `isOneTouchExpandable` is true so it should be checked here too to
> enable/disable actions
Alisen's concern is valid.
What if `setEnabled(false)` is called when `isOneTouchExpandable` is `false`
and then `setOneTouchExpandable(true)` is called which adds the buttons? The
buttons are enabled when they should be disabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1646653721
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 09:28:28 GMT, Prasanta Sadhukhan wrote: >> Issue is seen in that if we call setEnabled(false) over JSplitPane than it >> can't be dragged via its divider, But if SplitPane have one touch expandable >> true than user can click those buttons and change the divider position. >> So, if splitpane is disabled, then both dragging in divider and >> one-touch-expandable click should be disabled. >> Fix is made to override setEnabled and disable one-touch-expandable buttons >> actions.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Omit gtk Changes requested by aivanov (Reviewer). src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java line 376: > 374: leftButton.setEnabled(enabled); > 375: } > 376: } Is it possible to override `isEnabled` in `rightButton` and `leftButton` so that it returns the state of `JSplitPane`? Alternatively, the buttons could install a `PropertyChangeListener` for `"enabled"` property and align their state to the host split pane. test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 100: > 98: robot.mouseMove(loc.x, loc.y); > 99: robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); > 100: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); Isn't it enough to verify the state of the buttons whether they're enabled or not. It should be simpler, it could be done without rendering the frame. test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 147: > 145: rightOneTouchButton = super.createRightOneTouchButton(); > 146: return rightOneTouchButton; > 147: } Alternatively, you could add methods which return `leftButton` and `rightButton` fields. - PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2125353120 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644368671 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644371184 PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644374062
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Mon, 17 Jun 2024 05:08:29 GMT, Prasanta Sadhukhan wrote: > All L including Aqua extends BasicSplitPaneUI so it's ok...The existing > test iterates through all L without any issue.. That is true, yet it is still possible to set a L that doesn't extend `BasicSplitPaneUI` and the updated code will throw `ClassCastException`. I'm sure such a situation is rare, if it exists at all, yet I don't think the public API should have such a limitation: `JSplitPane.setUI` accepts `SplitPaneUI`, so it is valid to pass an object that is not subclass of `BasicSplitPaneUI`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1644363798
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 09:28:28 GMT, Prasanta Sadhukhan wrote: >> Issue is seen in that if we call setEnabled(false) over JSplitPane than it >> can't be dragged via its divider, But if SplitPane have one touch expandable >> true than user can click those buttons and change the divider position. >> So, if splitpane is disabled, then both dragging in divider and >> one-touch-expandable click should be disabled. >> Fix is made to override setEnabled and disable one-touch-expandable buttons >> actions.. > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Omit gtk Marked as reviewed by achung (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2124113003
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 20:39:25 GMT, Alisen Chung wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Omit gtk
>
> src/java.desktop/share/classes/javax/swing/JSplitPane.java line 372:
>
>> 370: public void setEnabled(boolean enabled) {
>> 371: super.setEnabled(enabled);
>> 372:
>> ((BasicSplitPaneUI)(this.getUI())).getDivider().setEnabled(enabled);
>
> is it fine to assume that the UI is a BasicSplitPaneUI object? what happens
> if the JSplitPane UI is an AquaSplitPaneUI for example?
All L including Aqua extends BasicSplitPaneUI so it's ok...The existing test
iterates through all L without any issue..
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
> line 369:
>
>> 367: @Override
>> 368: public void setEnabled(boolean enabled) {
>> 369: if (splitPane.isOneTouchExpandable() &&
>
> i think for setEnabled the buttons should be enabled/disabled regardless of
> if splitPane isOneTouchExpandable is true so that the state is preserved even
> if setEnabled(false) is called before
> setOneTouchExpandable(true)
No, the left and right arrow buttons are created ONLY when
`isOneTouchExpandable` is true so it should be checked here too to
enable/disable actions
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1642181720
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1642179508
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 09:28:28 GMT, Prasanta Sadhukhan
wrote:
>> Issue is seen in that if we call setEnabled(false) over JSplitPane than it
>> can't be dragged via its divider, But if SplitPane have one touch expandable
>> true than user can click those buttons and change the divider position.
>> So, if splitpane is disabled, then both dragging in divider and
>> one-touch-expandable click should be disabled.
>> Fix is made to override setEnabled and disable one-touch-expandable buttons
>> actions..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Omit gtk
src/java.desktop/share/classes/javax/swing/JSplitPane.java line 372:
> 370: public void setEnabled(boolean enabled) {
> 371: super.setEnabled(enabled);
> 372:
> ((BasicSplitPaneUI)(this.getUI())).getDivider().setEnabled(enabled);
is it fine to assume that the UI is a BasicSplitPaneUI object? what happens if
the JSplitPane UI is an AquaSplitPaneUI for example?
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
line 369:
> 367: @Override
> 368: public void setEnabled(boolean enabled) {
> 369: if (splitPane.isOneTouchExpandable() &&
i think for setEnabled the buttons should be enabled/disabled regardless of if
splitPane isOneTouchExpandable is true so that the state is preserved even if
setEnabled(false) is called before
setOneTouchExpandable(true)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1640334460
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1640329308
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
On Fri, 14 Jun 2024 09:28:28 GMT, Prasanta Sadhukhan
wrote:
>> Issue is seen in that if we call setEnabled(false) over JSplitPane than it
>> can't be dragged via its divider, But if SplitPane have one touch expandable
>> true than user can click those buttons and change the divider position.
>> So, if splitpane is disabled, then both dragging in divider and
>> one-touch-expandable click should be disabled.
>> Fix is made to override setEnabled and disable one-touch-expandable buttons
>> actions..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Omit gtk
Ran the test with suggested fix. Looks good to me.
test/jdk/javax/swing/JSplitPane/TestSplitPaneEnableTest.java line 57:
> 55: UIManager.setLookAndFeel(laf.getClassName());
> 56: } catch (UnsupportedLookAndFeelException ignored) {
> 57: System.out.println("Unsupported LAF: " + laf.getClassName());
May be updated to throw `jtreg.SkippedException`.
-
Marked as reviewed by abhiscxk (Committer).
PR Review: https://git.openjdk.org/jdk/pull/19695#pullrequestreview-2118024993
PR Review Comment: https://git.openjdk.org/jdk/pull/19695#discussion_r1639603739
Re: RFR: 5021949: JSplitPane setEnabled(false) shouldn't be partially functional [v3]
> Issue is seen in that if we call setEnabled(false) over JSplitPane than it > can't be dragged via its divider, But if SplitPane have one touch expandable > true than user can click those buttons and change the divider position. > So, if splitpane is disabled, then both dragging in divider and > one-touch-expandable click should be disabled. > Fix is made to override setEnabled and disable one-touch-expandable buttons > actions.. Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision: Omit gtk - Changes: - all: https://git.openjdk.org/jdk/pull/19695/files - new: https://git.openjdk.org/jdk/pull/19695/files/e8ac1bb0..7aadfcc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19695=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19695=01-02 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19695.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19695/head:pull/19695 PR: https://git.openjdk.org/jdk/pull/19695
