RFR: 8302816: Refactor sorting-related classes [v2]

[Ambarish Rapte]

On Wed, 26 Apr 2023 10:16:54 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Most of the changes revolve around unifying the sorting methods for a collection with `Comparable` elements with sorting methods that take an external `Comparator` by passing `Comparator.naturalOrder()` from the former to the latter. This eliminates method duplication and some warnings suppressions.
>> 
>> Note that I get 1 failing test: VersionInfoTest.testMajorVersion. This PR is unrelated to this test.
>
> Nir Lisker has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - Added missing space
>  - Merge branch 'master' into 8302816_Refactor_sorting-related_classes
>  - Initial commit

Providing few minor comments.
Please allow a day more to review the changes in `SortHelper.java`

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 45:

> 43:  *
> 44:  */
> 45: public class ObservableListWrapper<E> extends ModifiableObservableListBase<E> implements SortableList<E>, RandomAccess {

Looks like an unintended change. Can be reverted.

modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableSequentialListWrapper.java line 244:

> 242:     }
> 243: 
> 244:     private SortHelper helper;

Looks like this only got moved from line# 41. Can be placed back to reduce diff.

modules/javafx.base/src/main/java/com/sun/javafx/collections/SortHelper.java line 36:

> 34: /**
> 35:  * A helper class containing algorithms taken from JDK 6 that additionally track the permutations that're created
> 36:  * during the process.

Break line to <= 80 length.

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 670:

> 668: 
> 669:     /**
> 670:      * Sorts the provided observable list as specified in {@link java.util.Collections#sort(List) Collections.sort(List)}.

Could you please break the line into two, similar to the lines# 681, 682.

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 673:

> 671:      * Fires only <b>one</b> change notification on the list.
> 672:      *
> 673:      * @param <T> the element type of the list

Looks like this type of wording is widely used in jfx source : `The type of the elements contained within the List.`
[Example in ComboBoxTreeTableCell](https://github.com/openjdk/jfx/blob/1975165bed2942eaad9a1d7685839d2f77339ccb/modules/javafx.controls/src/main/java/javafx/scene/control/cell/ComboBoxTreeTableCell.java#L56)
May be adapt similar wording here, and other modified places too. ?

Or use similar as Collections.sort() as:
`the type of the elements in the list`

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 687:

> 685:      * @param <T> the element type of the list
> 686:      * @param list the list to sort
> 687:      * @param comparator the comparator to determine the order of the list. A {@code null} value indicates that the

Line looks lengthy, may be let's break into two

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

Changes requested by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1041#pullrequestreview-1405289087
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1179981604
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1180057892
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1180063363
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1179970773
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1179976388
PR Review Comment: https://git.openjdk.org/jfx/pull/1041#discussion_r1180003549