-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5021949: JSplitPane setEnabled(false) shouldn't be partially functional #19695
Conversation
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
@prsadhuk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 195 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the test can be modified to iterate for all installed LAFs.
try { | ||
Robot robot = new Robot(); | ||
SwingUtilities.invokeAndWait(() -> { | ||
frame = new JFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frame title can be added.
robot.delay(1000); | ||
|
||
SwingUtilities.invokeAndWait(() -> { | ||
loc = leftOneTouchButton.getLocationOnScreen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new JButton("Left"), | ||
new JButton("Right")); | ||
|
||
frame.getContentPane().add(jsp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frame.getContentPane().add(jsp); | |
frame.add(jsp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the test with suggested fix. Looks good to me.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
Show resolved
Hide resolved
@Override | ||
public void setEnabled(boolean enabled) { | ||
super.setEnabled(enabled); | ||
((BasicSplitPaneUI)(this.getUI())).getDivider().setEnabled(enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it fine to assume that the UI is a BasicSplitPaneUI object? what happens if the JSplitPane UI is an AquaSplitPaneUI for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All L&F including Aqua extends BasicSplitPaneUI so it's ok...The existing test iterates through all L&F without any issue..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All L&F including Aqua extends BasicSplitPaneUI so it's ok...The existing test iterates through all L&F without any issue..
That is true, yet it is still possible to set a L&F 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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok...Will add check..
@Override | ||
public void setEnabled(boolean enabled) { | ||
super.setEnabled(enabled); | ||
((BasicSplitPaneUI)(this.getUI())).getDivider().setEnabled(enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All L&F including Aqua extends BasicSplitPaneUI so it's ok...The existing test iterates through all L&F without any issue..
That is true, yet it is still possible to set a L&F 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
.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
Show resolved
Hide resolved
robot.mouseMove(loc.x, loc.y); | ||
robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); | ||
robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok..modified to just check for button state...
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneDivider.java
Show resolved
Hide resolved
jsp.setUI(new TestSplitPaneUI()); | ||
jsp.setOneTouchExpandable(true); | ||
|
||
jsp.setEnabled(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be simpler, it could be done without rendering the frame.
I'm pretty sure it could be done without creating a frame and without showing the frame:
jsp.setUI(new TestSplitPaneUI()); | |
jsp.setOneTouchExpandable(true); | |
jsp.setEnabled(false); | |
jsp.setUI(new TestSplitPaneUI()); | |
jsp.setOneTouchExpandable(true); | |
jsp.setEnabled(false); | |
if (leftOneTouchButton.isEnabled()) { | |
throw new RuntimeException("leftButton is enabled for disabled JSplitPane"); | |
} | |
if (rightOneTouchButton.isEnabled()) { | |
throw new RuntimeException("rightButton is enabled for disabled JSplitPane"); | |
} |
Calling jsp.setEnabled(false);
changes the state of the buttons immediately, doesn't it?
I guess modified PR will solve the problem you reported... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, except for a couple of minor comments.
@@ -60,46 +57,27 @@ private static void setLookAndFeel(UIManager.LookAndFeelInfo laf) { | |||
} | |||
|
|||
public static void main(String[] args) throws Exception { | |||
Robot robot = new Robot(); | |||
for (UIManager.LookAndFeelInfo laf : UIManager.getInstalledLookAndFeels()) { | |||
if (laf.getClassName().toLowerCase().contains("gtk")) { | |||
continue; | |||
} | |||
System.out.println("Testing LAF : " + laf.getClassName()); | |||
SwingUtilities.invokeAndWait(() -> setLookAndFeel(laf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setLookAndFeel(laf)
can be moved into the main invokeAndWait
block.
private static JSplitPane jsp; | ||
private static volatile boolean btnEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both jsp
and btnEnabled
are unused now.
/* | ||
* @test | ||
* @bug 5021949 | ||
* @key headful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify if the updated test still requires headful environment.
/integrate |
Going to push as commit 5fe07b3.
Your commit was automatically rebased without conflicts. |
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..
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19695/head:pull/19695
$ git checkout pull/19695
Update a local copy of the PR:
$ git checkout pull/19695
$ git pull https://git.openjdk.org/jdk.git pull/19695/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19695
View PR using the GUI difftool:
$ git pr show -t 19695
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19695.diff
Webrev
Link to Webrev Comment