-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
6510914: JScrollBar.getMinimumSize() breaks the contract of JComponent.setMinimumSize() #15325
Conversation
This reverts commit d52de28.
…onent.setMinimumSize()
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
|
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.
Looks good to me.
Could you update the copyright year in JScrollBar.java
?
@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 180 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 |
It looks like the same bug exists in the implementation of getMaximumSize(). BTW the GA are all read, I guess most of that issues were fixed since June 12. Please merge from master. |
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.
Test verified.
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 comment is below…
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.
JScrollBar
overrides both getMinimumSize
and getMaximumSize
and specifies different behaviour: “The scrollbar is flexible along it's scrolling axis and rigid along the other axis.”
Should it follow the rules set by JComponent
? It don't think so.
How does the scrollbar look like if you set its minimum or maximum size to a larger size along its other axis?
Should we rather update the spec for JScrollBar
? It may override setMinimumSize
and setMaximumSize
to specify different behaviour.
I dont see any difference in scrollbar look if I set minimum and maximum size to larger size.... but getMinimumSize/getMaximumSize returns 275 and 800.. |
@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! |
I dont see any visual change with this fix and it solves so not sure where to update the spec..in the JScrollBar class top summary or in @aivanov-jdk @prrace Please provide your opinion.. |
@@ -760,6 +760,9 @@ public void stateChanged(ChangeEvent e) { | |||
* rigid along the other axis. | |||
*/ | |||
public Dimension getMinimumSize() { | |||
if (isMinimumSizeSet()) { | |||
return super.getMinimumSize(); | |||
} |
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.
This looks like it has potential to cause many compatibility problems.
I think it better to simply update the Jscrollbar spec on these methods to reflect what JScrollbar does.
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. Fair enough...Updated the spec of getMinimumSize
and getMaximumSize
to mention this behaviour change as JScrollBar doesn't override setMinimumSize
and setMaximumSize
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.
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 admit I'm lost…
Do we implement the hybrid approach? That is getMinimumSize
and getMaximumSize
return a value derived from getPreferredSize
(modifying it so that the scroll bar is flexible along its scrolling axis rather than returning it directly as ComponentUI
does). If minimum or maximum size is set, the set value is returned.
Is it correct?
If it is, do we even need to override setMinimumSize
and setMaximumSize
?
The contract specified in JComponent
is satisfied. Yet, not in full: neither of these methods returns ui.get*Size()
, either method calls getPreferredSize
(which in its turn calls ui.getPreferredSize
) and adjusts the value.
Hence, we still need to override them to update get min/max size to say the value is derived from preferred size rather than ui
delegate's min/max size. (This can be asserted with a unit test.)
* @return the value of the {@code minimumSize} property if set by user | ||
* or if not set, minimum size derived from | ||
* preferred size in one axis and fixed size in another axis |
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 think you should refer the derived size first is the most common returned value, and then mention minimumSize
if it's set.
* Unlike most components, JScrollBar will derive the minimum size from | ||
* the preferred size in one axis and a fixed minimum size in the other | ||
* unless otherwise set by the user. | ||
* Thus, it overrides {@code JComponent.setMinimumSize} contract | ||
* that subsequent calls to getMinimumSize will return the | ||
* same value as set in {@code JComponent.setMinimumSize} |
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.
Doesn't the javadoc contradict the implementation?
This fix has restored the contract of setMinimumSize
/ getMinimumSize
, so JScrollBar
is now like other components.
What we can add though is how the size is calculated when it's not set, however, it's documented in the get-
methods.
*/ | ||
public void setMaximumSize(Dimension maximumSize) { | ||
super.setMaximumSize(maximumSize); | ||
} | ||
|
||
/** | ||
* The scrollbar is flexible along it's scrolling axis and |
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 scrollbar is flexible along it's scrolling axis and | |
* The scrollbar is flexible along its scrolling axis and |
Let's fix it too.
@@ -771,8 +804,15 @@ public Dimension getMinimumSize() { | |||
/** | |||
* The scrollbar is flexible along it's scrolling axis and |
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 scrollbar is flexible along it's scrolling axis and | |
* The scrollbar is flexible along its scrolling axis and |
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.
Yes, I just noticed that too. Pre-existing but clearly not correct grammar.
@@ -771,8 +804,15 @@ public Dimension getMinimumSize() { | |||
/** | |||
* The scrollbar is flexible along it's scrolling axis and |
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.
Yes, I just noticed that too. Pre-existing but clearly not correct grammar.
|
||
/** | ||
* The scrollbar is flexible along it's scrolling axis and | ||
* rigid along the other axis. | ||
* | ||
* @return the value of the {@code minimumSize} property if set by user | ||
* or if not set, minimum size derived from |
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.
Seems we are not there yet. What is in this PR now is internally inconsistent.
It adopts doc on setSize that reads as if getSize will always ignore the super-class contract,
but on the get*Size methods we have new doc and implementation that actually in large part honours it.
One of these needs to change and then the CSR needs to match.
So, revisiting old ground, ie we've been back and forth on this.
we can keep the current PR code which is "if the user calls set*Size, then honour it, accepting the low risk, OR
we can keep the long standing behaviour, which is as near as we can get to no risk.
I'm still a bit on the fence, but I think I never want to hear about JScrollBar size again, :-)
So can we delete the new code in the getSize methods and copy some of the wording from set
*JScrollBar will derive the maximum size from
* the preferred size in one axis and a fixed maximum size in the other
then the return doc can reduce to
@ return the minimum size calculated as described above.
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.
So can we delete the new code in the get_Size methods and copy some of the wording from set_ *JScrollBar will derive the maximum size from * the preferred size in one axis and a fixed maximum size in the other
then the return doc can reduce to @ return the minimum size calculated as described above.
Updated as per suggestion..
/** | ||
* Unlike most components, JScrollBar will derive the maximum size from | ||
* the preferred size in one axis and a fixed maximum size in the other | ||
* unless otherwise set by the user. |
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.
that last line (unless ...) needs to go if we adopt what I suggest below
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've also reviewed the CSR. I think that can be finalized but Joe may yet ask us to add some more info in the JComponent methods.
* Unlike most components, JScrollBar will derive the minimum size from | ||
* the preferred size in one axis and a fixed minimum size in the other. | ||
* Thus, it overrides {@code JComponent.setMinimumSize} contract | ||
* that subsequent calls to getMinimumSize will return the | ||
* same value as set in {@code JComponent.setMinimumSize} |
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.
* Unlike most components, JScrollBar will derive the minimum size from | |
* the preferred size in one axis and a fixed minimum size in the other. | |
* Thus, it overrides {@code JComponent.setMinimumSize} contract | |
* that subsequent calls to getMinimumSize will return the | |
* same value as set in {@code JComponent.setMinimumSize} | |
* Unlike most components, {@code JScrollBar} derives the minimum size from | |
* the preferred size in one axis and a fixed minimum size in the other. | |
* Thus, it overrides {@code JComponent.setMinimumSize} contract | |
* that subsequent calls to {@code getMinimumSize} will return the | |
* same value as set in {@code JComponent.setMinimumSize}. |
@prrace Should the first sentence use the present tense instead of the future?
There should be a period in the end of the sentence.
Should we add a paragraph saying, calling setMinimumSize
has no effect?
* The scrollbar is flexible along its scrolling axis and | ||
* rigid along the other axis. | ||
* As specified in {@code setMinimumSize} JScrollBar will derive the | ||
* minimum size from the preferred size in one axis and a | ||
* fixed minimum size in the other. | ||
* | ||
* @return the minimum size as specified above. |
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 first sentence of the javadoc is put into the method summary, therefore the first sentence should explain what is returned. Is it worth addressing it too?
Something like: Returns the minimum size for the {@code JScrollBar}. The minimum size is equal to a fixed minimum value along its scrolling axis and the preferred size along the other axis.
* The scrollbar is flexible along its scrolling axis and | ||
* rigid along the other axis. | ||
* As specified in {@code setMaximumSize} JScrollBar will derive the | ||
* maximum size from the preferred size in one axis and a | ||
* fixed maximum size in the other. | ||
* | ||
* @return the maximum size as specified above. |
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.
This can be amended in a similar way to getMinimumSize
.
Returns the maximum size for the {@code JScrollBar}. The maximum size is equal to a fixed maximum value its scrolling axis and the preferred size along the other axis. This makes the scrollbar flexible along its scrolling axis and rigid along the other axis.
Added JComponent clarification to address CSR concern.. |
* to compute it. | ||
* Subclasses may choose to override this by returning its own maximum size | ||
* in its {@code getMaximumSize} method. | ||
* Setting the maximum size to <code>null</code> restores the default behavior. |
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'd leave null-related where it was because it belongs to the description of the default behaviour. And subclasses can override the default behaviour.
* to compute it. | |
* Subclasses may choose to override this by returning its own maximum size | |
* in its {@code getMaximumSize} method. | |
* Setting the maximum size to <code>null</code> restores the default behavior. | |
* to compute it. Setting the maximum size to <code>null</code> | |
* restores the default behavior. | |
* <p> | |
* Subclasses may choose to override this by returning its own maximum size | |
* in its {@code getMaximumSize} method. |
Add <p>
to start a new paragraph in the javadoc so that the statement is easier to notice.
I suggest to keep one style, either use <code>
or {@code}
everywhere. Using different styles doesn't look good.
Since the modern way is to use {@code}
, it makes sense to update the entire method doc. Yet it will make it harder to find the important changes.
@@ -780,6 +780,7 @@ public void setMaximumSize(Dimension maximumSize) { | |||
} | |||
|
|||
/** | |||
* Returns the minimum size for the {@code JScrollBar}. | |||
* The scrollbar is flexible along its scrolling axis and | |||
* rigid along the other axis. | |||
* As specified in {@code setMinimumSize} JScrollBar will derive the |
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.
This doesn't look good to me. It is getMinimumSize
that specifies what is returned; setMinimumSize
just warns the programmer that getMinimumSize
ignores the value passed to the set-
method.
It's the other way around: getMinimumSize
should specify and specifies what it returns — setMinimumSize
is being updated to align with the implementation of the get-
.
* The scrollbar is flexible along its scrolling axis and | ||
* rigid along the other axis. |
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 would rather clarify this property by describing how the returned value is derived from the preferred size. Does it make sense? Should it be an @implNote
rather than a spec? Or is it completely unnecessary?
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.
Not sure..Since it is already reviewed and approved by Phil, I chose to keep it same..
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.
This change requires a CSR which hasn't been approved yet. It's an opportunity to improve the specification for getMinimumSize
and getMaximumSize
.
After the CSR is approved, further updates will be more of a hassle.
This is why I'm asking these questions here. “No, we don't want to update,” is also a valid answer. Let's see what @prrace has to say.
@prsadhuk this pull request can not be integrated into git checkout JDK-6510914
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
* Subclasses may choose to override this by returning its own maximum size | ||
* in its {@code getMaximumSize} method. |
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.
* Subclasses may choose to override this by returning its own maximum size | |
* in its {@code getMaximumSize} method. | |
* Subclasses may choose to override this by returning their own maximum size | |
* in the {@code getMaximumSize} method. |
“Subclasses” is plural and therefore requires plural possessive pronoun.
* Subclasses may choose to override this by returning its own minimum size | ||
* in its {@code getMinimumSize} method. |
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.
* Subclasses may choose to override this by returning its own minimum size | |
* in its {@code getMinimumSize} method. | |
* Subclasses may choose to override this by returning their own minimum size | |
* in the {@code getMinimumSize} method. |
* The scrollbar is flexible along its scrolling axis and | ||
* rigid along the other axis. |
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.
This change requires a CSR which hasn't been approved yet. It's an opportunity to improve the specification for getMinimumSize
and getMaximumSize
.
After the CSR is approved, further updates will be more of a hassle.
This is why I'm asking these questions here. “No, we don't want to update,” is also a valid answer. Let's see what @prrace has to say.
* minimum size from the preferred size in one axis and a | ||
* fixed minimum size in the other. | ||
* | ||
* @return the minimum size as specified above. |
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.
* @return the minimum size as specified above. | |
* @return the minimum size as specified above |
@return
tags usually don't have a period in the end. Let's follow it for consistency.
/integrate |
Going to push as commit 39627bc.
Your commit was automatically rebased without conflicts. |
javadoc contract for JComponent.setMinimumSize(Dimension) states:
"Sets the minimum size of this component to a constant value. Subsequent calls to getMinimumSize will always return this value..."
However, JScrollBar overrides getMinimumSize() and breaks this contract - it always returns a minimum size derived from the preferred size even if you have previously called setMinimumSize()
Fix is made to check if mnimumSize is set and if so, honour it..
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15325/head:pull/15325
$ git checkout pull/15325
Update a local copy of the PR:
$ git checkout pull/15325
$ git pull https://git.openjdk.org/jdk.git pull/15325/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15325
View PR using the GUI difftool:
$ git pr show -t 15325
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15325.diff
Webrev
Link to Webrev Comment