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

[Nir Lisker]

On Sat, 10 Aug 2024 11:53:00 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>>> 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...
>
>> 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.
> 
> Why is that the expected order?  It is "a" possible, but valid order.  When only sorting on `Col1` then in my opinion [1, 0, 2, 3], [1, 0, 3, 2], [1, 2, 0, 3], etc.. are all valid sort orders.
> 
> If we want to distinguish between equal elements to give them a fixed sub-ordering (and the inverse one I suppose when the sort is descending) then what should the distinguishing factor be?  Its position in the underlying unsorted list?  The time it was added to the underlying list?  They need not be the same.  Using the position in the underlying list could be quite random if the source of the list doesn't guarantee a fixed position (from an unsorted database query for example).
> 
> What if the underlying list is another sorted list or a filtered list, can we even rely on the indices those provide?

@hjohn I was confused as well, but the [`TableView` docs](https://openjfx.io/javadoc/22/javafx.controls/javafx/scene/control/TableView.html#sorting-heading) say:
> Starting with JavaFX 8.0 (and the introduction of [SortedList](https://openjfx.io/javadoc/22/javafx.base/javafx/collections/transformation/SortedList.html)), it is now possible to have the collection return to the unsorted state when there are no columns as part of the TableView [sort order](https://openjfx.io/javadoc/22/javafx.controls/javafx/scene/control/TableView.html#getSortOrder()). To do this, you must create a SortedList instance, and bind its [comparator](https://openjfx.io/javadoc/22/javafx.base/javafx/collections/transformation/SortedList.html#comparatorProperty()) property to the TableView [comparator](https://openjfx.io/javadoc/22/javafx.controls/javafx/scene/control/TableView.html#comparatorProperty()) property

So I think that the confusion stems from the fuzzy guarantee that removing the column-based sorting will return the table to *the* unsorted state before that sorting (and not to *a* unsorted state). That might mean that `TableView` will have to maintain the initial sorting somewhere. Or... it's a bad documentation :)

I still don't see how this is a bug in `SortedList`. It could be a bug in `TableView`.

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

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