On Wed, 26 Apr 2023 10:16:54 GMT, Nir Lisker <nlis...@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
