4797982: Setting negative size of JSplitPane divider leads to unexpected results. #9566

prsadhuk · 2022-07-20T09:12:38Z

Setting JSplitPane divider size to negative value leads to unexpected results and is not desirable and seems to be not practical.
I guess we should return IAE but it might break existing app so fixed to clamp it to 0 incase negative value is tried to be set for divider size.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires a CSR request to be approved

Issues

JDK-4797982: Setting negative size of JSplitPane divider leads to unexpected results.
JDK-8290992: Setting negative size of JSplitPane divider leads to unexpected results. (CSR)

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer) ⚠️ Review applies to 40ddca94
Phil Race (@prrace - Reviewer) ⚠️ Review applies to ec9429aa

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9566/head:pull/9566
$ git checkout pull/9566

Update a local copy of the PR:
$ git checkout pull/9566
$ git pull https://git.openjdk.org/jdk pull/9566/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9566

View PR using the GUI difftool:
$ git pr show -t 9566

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9566.diff

…ted results.

bridgekeeper · 2022-07-20T09:13:12Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-07-20T09:15:59Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-07-20T09:20:50Z

Webrevs

openjdk · 2022-07-21T17:46:56Z

@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:

4797982: Setting negative size of JSplitPane divider leads to unexpected results.

Reviewed-by: azvegint, prr

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 857 new commits pushed to the master branch:

07afa3f: 8294110: compiler/uncommontrap/Decompile.java fails after JDK-8293798
0746bcb: 8294083: RISC-V: Minimal build failed with --disable-precompiled-headers
95ec2ea: 8293897: Synthetic final modifier is part of the AST for a try-with-resource resource
d14e96d: 8293493: Signal Handlers printout should show signal block state
da4fdfb: 8293659: Improve UnsatisfiedLinkError error message to include dlopen error details
cd1cdcd: 8293116: Incremental JDK build could be sped up
e9401e6: 8293364: IGV: Refactor Action in EditorTopComponent and fix minor bugs
844a95b: 8292892: Javadoc index descriptions are not deterministic
8d1dd6a: 8294076: Improve ant detection in idea.sh
4e7cb15: 8293480: IGV: Update Bytecode and ControlFlow Component immediately when opening a new graph
... and 847 more: https://git.openjdk.org/jdk/compare/46251bc6e248a19e8d78173ff8d0502c68ee1acb...master

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 master branch, type /integrate in a new comment.

prrace · 2022-07-22T20:19:02Z

src/java.desktop/share/classes/javax/swing/JSplitPane.java

@@ -424,6 +424,9 @@ public String getUIClassID() {
    @BeanProperty(description
            = "The size of the divider.")
    public void setDividerSize(int newSize) {
+        if (newSize < 0) {


What are the "unexpected results" of < 0 ?
Should it perhaps be 1 ?
And what about this ?
https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/javax/swing/plaf/basic/BasicSplitPaneDivider.html#setDividerSize(int)

And we need a CSR ...

What are the "unexpected results" of < 0 ?

Normally, JSplitPane is expected to show 2 components either horizontally or vertically depending on if it is VERTICAL_SPLIT or HORIZONTAL_SPLIT but in case of -ve size, it only shows 1 component which might be construed as unexpected. KeyPress in 1st component if it's JTextArea seems to get lost as nothing is shown.

Should it perhaps be 1 ?

It could have 3 cases

set to 0 (it will not have any visible divider)

set to 1 (it will have least visible divider)

just return
I initially thought setting it to 0 as by default, the "dividerSize" variable is 0

but now, I think 3rd option of "just return" should be better as then it will fallback to default SplitPane.dividerSize value as L&F set it to some default value like
in Metal L&F it is 10
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/plaf/metal/MetalLookAndFeel.java#L1371
in Aqua, it is 9
https://github.com/openjdk/jdk/blob/master/src/java.desktop/macosx/classes/com/apple/laf/AquaLookAndFeel.java#L847
in Basic L&F, it is 7
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicLookAndFeel.java#L1342

so if we set divider size to -ve and then call getDividerSize() it will return the default L&F specific divider size.

we need a CSR

Canyou please clarify why? Is it because we are changing the behavior?
But, I guess, all bug fix(es) change the code to "correct" behavior and here also, we are rectifying the behavior without changing any spec/signature.
I thought a CSR would have been needed if we had returned IAE. Here, I guess we are changing the implementation detail. Should I raise CSR for implementation detail?

prrace · 2022-07-25T21:31:52Z

we need a CSR

Canyou please clarify why? Is it because we are changing the behavior?

So instead you propose to change from one un-specified behaviour to another un-specified behaviour ?

It seems to me we should be updating javadoc to say what should happen.
The "just return" sounds good to me .. in javadoc terms that would be "values < 1 are ignored" ..

prsadhuk · 2022-07-26T04:45:55Z

OK. javadoc clarified and CSR raised https://bugs.openjdk.org/browse/JDK-8290992

prrace · 2022-07-26T21:49:21Z

src/java.desktop/share/classes/javax/swing/JSplitPane.java

@@ -418,12 +418,18 @@ public String getUIClassID() {

    /**
     * Sets the size of the divider.
+     * @implNote Divider sizes < 1 are ignored.
+     *           {@code SplitPane.dividerSize} L&amp;F specific value
+     *           will instead be used.


This isn't the same as ignoring it. Suppose we do
setDividerSize(5);
setDividerSize(-1);

what is the value after both of these - per your ignoring code 5, but per your doc it will be whatever the default was before you started to change it.

I think it sufficient to say it will be ignored and drop the 2nd clause.

Ok. Modified javadoc and CSR

My apologies, but because I focused on the rest of it, I overlooked you had made this an implNote.
That seems odd. Do you really intend to imply that some other implementation of Java should be allowed
to do something with negative values ? If so that would be a compatibility issue ..

Yes, actually I was thinking in that sense, as I was not sure if other impl. will allow -ve values and just wanted to mention that our impl dont.
But, I guess as you pointed, we should make it disallowed for all. Modified PR and CSR.

DevCharly

Hmm, have some concerns with this change.

Why not allow zero divider size?

The look and feel could use an invisible 6px wide divider that partly overlaps the left and right components.
Then moving divider is still possible, but no divider is visible, which is often used in modern UI.
I actually plan to implement this for FlatLaf.

Sure, with current L&Fs it is not possible to move divider when dividerSize is zero,
but applications may still control the divider location with JSplitPane.setDividerLocation().
So it is possible that dividerSize zero is used in existing applications.

Why ignoring negative values?

Wouldn't it better to throw an IllegalArgumentException to give the developer some feedback?
Isn't this common practice in Java?
There are several parameter checks in JSplitPane and all throw IllegalArgumentException.
E.g. JSplitPane.setResizeWeight(double)

prsadhuk · 2022-08-18T12:08:43Z

We were not sure if -ve divider size can be used by third party implementation. It probably will be considered a compatibility issue as mentioned earlier.
@prrace Will you be ok to throw IAE instead of ignoring -ve value as suggested or should we keep it as it is now in this PR?

prrace · 2022-08-21T21:34:10Z

We were not sure if -ve divider size can be used by third party implementation. It probably will be considered a compatibility issue as mentioned earlier.
@prrace Will you be ok to throw IAE instead of ignoring -ve value as suggested or should we keep it as it is now in this PR?

IAE is just too incompatible. The program would probably stop working if anyone is actually doing that.
If it had been done since day one, yes, but too late now.

I didn't get why some effect like the "visibility" of the divider in some L&F should be allowed to over-rule the specified size.

DevCharly · 2022-08-21T22:50:48Z

All look and feels initially set the divider size to what they need. This is done here:

jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicSplitPaneUI.java

Lines 357 to 358 in eab4c0c

    
           Integer temp = (Integer)UIManager.get("SplitPane.dividerSize"); 
        
           LookAndFeel.installProperty(splitPane, "dividerSize", temp == null? 10: temp);

Each L&F uses its own divider size.
E.g. Windows L&F uses 5:

jdk/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java

Line 1247 in 0a65e8b

"SplitPane.dividerSize", Integer.valueOf(5),

Or Metal L&F uses 10:

jdk/src/java.desktop/share/classes/javax/swing/plaf/metal/MetalLookAndFeel.java

Line 1371 in d4b040f

"SplitPane.dividerSize", 10,

So a L&F could also use divider size zero to implement an invisible divider.
Don't see a reason, why divider size zero should be no longer allowed...

I didn't get why some effect like the "visibility" of the divider in some L&F should be allowed to over-rule the specified size.

If the application invokes JSplitPane.setDividerSize(int), then this overrules the divider size specified by the L&F.
There is a field JSplitPane.dividerSizeSet that is used for this.

L&Fs do not invoke JSplitPane.setDividerSize(int) directly. Instead they invoke LookAndFeel.installProperty(...), which calls JSplitPane.setUIProperty(...), which checks flag dividerSizeSet and does not change divider size if the application has invoked setDividerSize(...):

jdk/src/java.desktop/share/classes/javax/swing/JSplitPane.java

Lines 1059 to 1064 in 096bca4

    
           void setUIProperty(String propertyName, Object value) { 
        
               if (propertyName == "dividerSize") { 
        
                   if (!dividerSizeSet) { 
        
                       setDividerSize(((Number)value).intValue()); 
        
                       dividerSizeSet = false; 
        
                   }

prsadhuk · 2022-08-22T03:33:18Z

@prrace Probably we could reinstate @implNote to the javadoc as I did earlier, to allow 3rd party L&F -ve divider size?

bridgekeeper · 2022-09-19T09:05:16Z

@prsadhuk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

prsadhuk · 2022-09-26T10:59:27Z

/integrate

openjdk · 2022-09-26T11:00:50Z

Going to push as commit 2be3158.
Since your change was applied there have been 901 commits pushed to the master branch:

050eebf: 8294245: Make Compile::print_inlining_stream stack allocated
91a23d7: 8294142: make test should report only on executed tests
169a5d4: 8294193: Files.createDirectories throws FileAlreadyExistsException for a symbolic link whose target is an existing directory
3675f4c: 8293252: Shenandoah: ThreadMXBean synchronizer tests crash with aggressive heuristics
543851d: 8289607: Change hotspot/jtreg tests to not use Thread.suspend/resume
e2f8251: 8293618: x86: Wrong code generation in class Assembler
6ecd081: 8294270: make test passes awkward -status:-status:error,fail to jtreg
eca9749: 8288325: [windows] Actual and Preferred Size of AWT Non-resizable frame are different
2e20e7e: 8294271: Remove use of ThreadDeath from make utilities
e45f3d5: 8294281: Allow warnings to be disabled on a per-file basis
... and 891 more: https://git.openjdk.org/jdk/compare/46251bc6e248a19e8d78173ff8d0502c68ee1acb...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-26T11:01:06Z

@prsadhuk Pushed as commit 2be3158.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

prsadhuk added 3 commits July 20, 2022 10:05

4797982: Setting negative size of JSplitPane divider leads to unexpec…

5f50042

…ted results.

4797982: Setting negative size of JSplitPane divider leads to unexpec…

de6badd

…ted results.

Fix

40ddca9

openjdk bot added the rfr Pull request is ready for review label Jul 20, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Jul 20, 2022

azvegint approved these changes Jul 21, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 21, 2022

prrace reviewed Jul 22, 2022

View reviewed changes

Fix to return L&F specific divider size

758dfb8

Clarify behavior in javadoc

a5a9ec8

Fix

b486ea4

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Jul 26, 2022

prrace reviewed Jul 26, 2022

View reviewed changes

prsadhuk added 2 commits July 27, 2022 05:56

Javadoc correction

beb1f57

Remove implNote tag

ec9429a

prrace approved these changes Jul 28, 2022

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Jul 29, 2022

DevCharly suggested changes Aug 13, 2022

View reviewed changes

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Sep 19, 2022

Do not ignore 0 divider size

e876ddb

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 21, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2022

openjdk bot closed this Sep 26, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 26, 2022

prsadhuk deleted the JDK-4797982 branch September 26, 2022 11:01

4797982: Setting negative size of JSplitPane divider leads to unexpected results. #9566

4797982: Setting negative size of JSplitPane divider leads to unexpected results. #9566

prsadhuk commented Jul 20, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

mlbridge bot commented Jul 20, 2022 •

edited

openjdk bot commented Jul 21, 2022 •

edited

prrace Jul 22, 2022

prsadhuk Jul 25, 2022

prrace commented Jul 25, 2022

prsadhuk commented Jul 26, 2022

prrace Jul 26, 2022

prsadhuk Jul 27, 2022

prrace Jul 27, 2022

prsadhuk Jul 28, 2022

DevCharly left a comment

prsadhuk commented Aug 18, 2022

prrace commented Aug 21, 2022

DevCharly commented Aug 21, 2022

prsadhuk commented Aug 22, 2022

bridgekeeper bot commented Sep 19, 2022

prsadhuk commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

4797982: Setting negative size of JSplitPane divider leads to unexpected results. #9566

4797982: Setting negative size of JSplitPane divider leads to unexpected results. #9566

Conversation

prsadhuk commented Jul 20, 2022 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

mlbridge bot commented Jul 20, 2022 • edited

Webrevs

openjdk bot commented Jul 21, 2022 • edited

prrace Jul 22, 2022

Choose a reason for hiding this comment

prsadhuk Jul 25, 2022

Choose a reason for hiding this comment

prrace commented Jul 25, 2022

prsadhuk commented Jul 26, 2022

prrace Jul 26, 2022

Choose a reason for hiding this comment

prsadhuk Jul 27, 2022

Choose a reason for hiding this comment

prrace Jul 27, 2022

Choose a reason for hiding this comment

prsadhuk Jul 28, 2022

Choose a reason for hiding this comment

DevCharly left a comment

Choose a reason for hiding this comment

Why not allow zero divider size?

Why ignoring negative values?

prsadhuk commented Aug 18, 2022

prrace commented Aug 21, 2022

DevCharly commented Aug 21, 2022

prsadhuk commented Aug 22, 2022

bridgekeeper bot commented Sep 19, 2022

prsadhuk commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

openjdk bot commented Sep 26, 2022

prsadhuk commented Jul 20, 2022 •

edited by openjdk bot

mlbridge bot commented Jul 20, 2022 •

edited

openjdk bot commented Jul 21, 2022 •

edited