RFR: 8301761: The sorting of the SortedList can become invalid [v2]

[Kevin Rushforth]

On Sat, 10 Aug 2024 07:10:03 GMT, Loay Ghreeb <duke at openjdk.org> wrote:

>> The PR title "The sorting of the SortedList can become invalid" seems to imply that the sorting can become incorrect, as in the incorrect order.
>> 
>> This is not the case. The sorting is still correct. Claiming that the sorting is invalid because a newly added item was not placed in a specific location relative to other equal items is a bit of misrepresentation.
>> 
>> So, if this PR wants to move forward, I think we first need to decide if we want to guarantee a specific behavior when inserting new but equal elements.  If we do, then it should be documented, and also maintained when the list gets resorted (ie. resorts must be stable).
>
>> This is not the case. The sorting is still correct. Claiming that the sorting is invalid because a newly added item was not placed in a specific location relative to other equal items is a bit of misrepresentation.
>> 
>> So, if this PR wants to move forward, I think we first need to decide if we want to guarantee a specific behavior when inserting new but equal elements.
> 
> The issue isn’t just with adding new elements. It also affects the `TableView` that populated with items from a `SortedList`, The `SortedList`'s comparator is bound to the `TableView`'s comparator.
> When the table is sorted by two columns and the second sort is removed, the table should revert to sorting by just the first column. However, currently, removing the second column from the sort order doesn't change the items order.
> 
> For example:
> ![image](https://github.com/user-attachments/assets/9ed8b2ea-9879-4dc9-a285-4d03be2d2691)
> 
> 1. The initial order is [0, 1, 2, 3] (index column).
> 2. Sort by `Col1`, the order will be: [1, 0, 2, 3].
> 3. Add a second sort (Shift + Click) on `Col2`, the order will be: [1, 2, 3, 0].
> 4. Shift + Click again on `Col2` to sort in descending order: [1, 0, 3, 2].
> 5. Shift + Click again on `Col2` to remove the sort from `Col2`. The order will stay the same [1, 0, 3, 2], but the expected order is [1, 0, 2, 3] as in step 2.
> 
> <details>
> <summary> Code </summary>
> 
> 
> import javafx.application.Application;
> import javafx.beans.property.ReadOnlyObjectWrapper;
> import javafx.collections.FXCollections;
> import javafx.collections.ObservableList;
> import javafx.collections.transformation.SortedList;
> import javafx.scene.Scene;
> import javafx.scene.control.TableColumn;
> import javafx.scene.control.TableView;
> import javafx.stage.Stage;
> 
> public class Main extends Application {
> 
>     @Override
>     public void start(Stage primaryStage) {
>         primaryStage.setScene(new Scene(createTableView()));
>         primaryStage.show();
>     }
> 
>     private TableView<Data> createTableView() {
>         TableView<Data> tableView = new TableView<>();
> 
>         TableColumn<Data, Integer> indexColumn = new TableColumn<>("#");
>         indexColumn.setCellValueFactory(cell -> new ReadOnlyObjectWrapper<>(cell.getValue().index()));
>         indexColumn.setSortable(false);
> 
>         TableColumn<Data, Integer> column1 = new TableColumn<>("Col1");
>         column1.setCellValueFactory(cell -> new ReadOnlyObjectWrapper<>(cell.getValue().val1()));
> 
>         TableColumn<Data, Integer> column2 = new TableColumn<>("Col2");
>      ...

@LoayGhreeb I agree with @hjohn and @nlisker. Absent any new information as to why this should be considered a bug in SortedList, this PR is unlikely to move forward.

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

PR Comment: https://git.openjdk.org/jfx/pull/1519#issuecomment-2400242972