RFR: 6510914: JScrollBar.getMinimumSize() breaks the contract of JComponent.setMinimumSize() [v12]

[Alexey Ivanov]

On Mon, 29 Jan 2024 06:47:50 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> 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..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   javadoc update in JScrollbar and JComponent

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/JComponent.java line 1:

> 1: /*

Update the copyright year.

src/java.desktop/share/classes/javax/swing/JComponent.java line 1747:

> 1745:      * Subclasses may choose to override this by returning its own maximum size
> 1746:      * in its {@code getMaximumSize} method.
> 1747:      * Setting the maximum size to <code>null</code> restores the default behavior.

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.
Suggestion:

     * 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.

src/java.desktop/share/classes/javax/swing/JScrollBar.java line 1:

> 1: /*

Update the copyright year.

src/java.desktop/share/classes/javax/swing/JScrollBar.java line 785:

> 783:      * Returns the minimum size for the {@code JScrollBar}.
> 784:      * The scrollbar is flexible along its scrolling axis and
> 785:      * rigid along the other axis.

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?

src/java.desktop/share/classes/javax/swing/JScrollBar.java line 786:

> 784:      * The scrollbar is flexible along its scrolling axis and
> 785:      * rigid along the other axis.
> 786:      * As specified in {@code setMinimumSize} JScrollBar will derive the

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.

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

PR Review: https://git.openjdk.org/jdk/pull/15325#pullrequestreview-1848268260
PR Review Comment: https://git.openjdk.org/jdk/pull/15325#discussion_r1469346534
PR Review Comment: https://git.openjdk.org/jdk/pull/15325#discussion_r1469344802
PR Review Comment: https://git.openjdk.org/jdk/pull/15325#discussion_r1469346296
PR Review Comment: https://git.openjdk.org/jdk/pull/15325#discussion_r1469358497
PR Review Comment: https://git.openjdk.org/jdk/pull/15325#discussion_r1469351797