8301761: The sorting of the SortedList can become invalid

[LoayGhreeb]

Fix an issue in SortedList where the sorting became incorrect when adding new items that are equal to existing items according to the comparator. The SortedList should consider the insertion index of these items to maintain the correct order.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8301761: The sorting of the SortedList can become invalid (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1519/head:pull/1519
$ git checkout pull/1519

Update a local copy of the PR:
$ git checkout pull/1519
$ git pull https://git.openjdk.org/jfx.git pull/1519/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1519

View PR using the GUI difftool:
$ git pr show -t 1519

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1519.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @LoayGhreeb, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user LoayGhreeb" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[LoayGhreeb]

/signed
I submitted the OCA, but it's still under review.

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge]

Webrevs

• 03: Full - Incremental (fc3b2f0e)
• 02: Full - Incremental (0ef88ea8)
• 01: Full - Incremental (15f1a9ab)
• 00: Full (fa2deb48)

[hjohn]

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

[LoayGhreeb]

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:

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.

Code

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");
        column2.setCellValueFactory(cell -> new ReadOnlyObjectWrapper<>(cell.getValue().val2()));

        tableView.getColumns().addAll(indexColumn, column1, column2);

        ObservableList<Data> data = FXCollections.observableArrayList(
                new Data(0, 5, 4),
                new Data(1, 1, 7),
                new Data(2, 5, 2),
                new Data(3, 5, 3)
        );

        SortedList<Data> sortedData = new SortedList<>(data);

        sortedData.comparatorProperty().bind(tableView.comparatorProperty());
        tableView.setItems(sortedData);
        return tableView;
    }

    public record Data(int index, int val1, int val2) {
    }
}

[hjohn]

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?

[nlisker]

@hjohn I was confused as well, but the TableView docs say:

Starting with JavaFX 8.0 (and the introduction of SortedList), 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. To do this, you must create a SortedList instance, and bind its comparator property to the TableView comparator 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.

[bridgekeeper]

@LoayGhreeb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[kevinrushforth]

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

[koppor]

TL;DR: This change is covered by an additional test. All other tests remain intact. Thus, this change does not break anything; it merely introduces a new feature.

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?

In the role of a user, I persieve removing an additional sort order as undo operation.

Applying that thought to the example: "undo" is the reverse operation of a "do" operation. Meaning: First, Col2 should be considered as additional sorting. Then, it should not be considered as additional sorting.

Applying that thought to a longer table: I as user have a dataset with n columns. If the dataset is in 1NF only, there might be many columns having the same values. If I as user begin to sort starting from column 1, then column 2, then column 3. Then I see: Oh, wait, no, I want to skip that column and sort by column 4 instead of 3. Then I undo the sort by column 3 and continue with 4. -- If the sort is not reverted to the previous state, but some other state, I need to start from the beginning. For two columnsd, this "annoyance" might be OK, but what for 10 columns? If I made a sorting mistake at column 9, I really need to redo all 8 columns before?

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

I like the position in the list. At the time of "materialization" of the list, the position of an element is known - indepenent of it was retrieved by a SQL query or other sources.

Since this change "only" narrows the order, does not contradict other expectations, and really helps our desktop application to be more usable, I am strongly in favour to use the index as second criterion. -- An indication for no-change-for-others is that there was only an additional test case needed - and all existing test cases were unmodified.

What if the underlying list is another sorted list or a filtered list, can we even rely on the indices those provide?

It would be even natural if the index change, the result of the SortedList also changes. I think, this is OK.

[nlisker]

Your example is only applicable to TableView. I don't see how you can add and remove sort orders to a single SortedList. What you can do is modify its Comparator, but there is no guarantee that reverting that change will get you back to the previous order regarding equal elements. Even if the sorted list provided a stable sort, this would not be guaranteed.

Given a SortedList with a case-insensitive comparator of
["a", "b", "B", "C", "c"]
If I then change the comparator to case-sensitive (using ASCII table values), it becomes
["B", "C", "a", "b", "c"]
then if I restore the previous comparator and take into account stable sorting, it becomes
["a", "B", "b", "C", "c"]
which is different than the initial sorting. So, stable sort doesn't solve "undo".

If you're trying to solve this at the list-level, you'll need an implementation of SortedList with memory - an "undo sorted list".

This means that the problem is probably not in SortedList. As I wrote above, TableView says it offers a special case of this behavior: when reverting back to no sorting. SortedList says that with a null comparator it defaults back to the wrapped list order. Perhaps when all columns are unselected for sorting, a null comparator is imposed on the list. However, there is no guarantee that switching between sortings preserves any memory, as in your example. It's also not clear why it should.

To summarize, the "memory" of the list exists in one place only, which is the backing list, and the sorted list gives a sorted "view" of it. There is no memory between sorts, as requested in your example. It looks like you will want to implement your own TableView sorting behavior.