8091429: ObservableList<E>#replaceRange(int from, int to, Collection<? extends E> col)

[hjohn]

This PR implements a new default method on ObservableList to be able to replace elements within a specified range.

Justification for this change is to allow an ObservableList to be bulk modified resulting in a single ListChangeListener call back. In this way the callbacks don't observe the list changing its size from S to S-X back to S again(*). Currently the only way to bulk replace a range of items is to remove X items then add X items, resulting in two listener callbacks in between which the size of the list can be observed to change.

The other alternative is to call set individually for each item, which results in many change notifications.

With the addition of this PR, and the changes in ModifiableObservableListBase, replacing a range of items becomes a single change callback.

(*) The list may indeed change size still as plain List does not have setAll operations; size listeners may observe this, but it will no longer be observable from a ListChangeListener due to multiple separate callbacks.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
• Change requires CSR request JDK-8371355 to be approved

Issues

• JDK-8091429: ObservableList<E>#replaceRange(int from, int to, Collection<? extends E> col) (Enhancement - P4)
• JDK-8371355: ObservableList<E>#replaceRange(int from, int to, Collection<? extends E> col) (CSR)

Reviewers

• Michael Strauß (@mstr2 - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1937

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8091429: ObservableList<E>#replaceRange(int from, int to, Collection<? extends E> col)

Reviewed-by: mstrauss, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

• bb66021: 8366202: RichTextArea: wrong style used for typed text
• 5684edf: 8371080: RichTextArea: missing styles in Caspian
• 02756a8: 8359599: Calling refresh() for all virtualized controls recreates all cells instead of refreshing the cells
• ... and 14 more: https://git.openjdk.org/jfx/compare/38300ca6c6c3483d35abec6f3fb2431402a26af0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 04: Full - Incremental (980b3c54)
• 03: Full - Incremental (3e3bb966)
• 02: Full - Incremental (730ea77f)
• 01: Full - Incremental (0b42426b)
• 00: Full (776dd9aa)

[nlisker]

How about doing this through the SubList view? JavaFX's implementation already has a SubObservableList inner class that propagates changes on it to the list's listeners:

		ObservableList<Integer> list = FXCollections.observableArrayList();
		list.addAll(0, 1, 2, 3, 4, 5, 6);
		list.addListener((ListChangeListener<Integer>) c -> System.out.println(c));
		List<Integer> subList = list.subList(2, 4);
		System.out.println(list);
		System.out.println(subList);
		subList.clear(); // equivalent to list.remove(2, 4);

prints

[0, 1, 2, 3, 4, 5, 6]
[2, 3]
{ [2, 3] removed at 2 }

The issue is that it's a List and not an ObservableList, so it doesn't have JavaFX's additional bulk operation methods. However, "just" having ObservableList#subList return an ObservableList will allow to make all range operations easily:

		ObservableList<Integer> list = FXCollections.observableArrayList();
		list.addAll(0, 1, 2, 3, 4, 5, 6);
		list.addListener((ListChangeListener<Integer>) c -> System.out.println(c));
		ObservableList<Integer> subList = list.subList(2, 4); // suggestion
		subList.setAll(7, 8);

I think that this would be more flexible/composable and result in a smaller API surface (current bulk methods would also be made redundant technically).

[mstr2]

The issue is that it's a List and not an ObservableList, so it doesn't have JavaFX's additional bulk operation methods. However, "just" having ObservableList#subList return an ObservableList will allow to make all range operations easily:

This change would be neither source compatible nor binary compatible, so this probably rules it out.

[hjohn]

How about doing this through the SubList view? JavaFX's implementation already has a SubObservableList inner class that propagates changes on it to the list's listeners:

I think that this would be more flexible/composable and result in a smaller API surface (current bulk methods would also be made redundant technically).

This is the first thing I tried, but we can't change the return type of ObservableList#subList. So you'd be forced to do a cast to get access to the setAll method which is ObservableList specific. Aside from that, I don't know what would happen if SubObservableList becomes a full ObservableList (with listeners, etc) as it currently definitely does not take such use into account (it basically is only using a standard List as its input). For sure that would require many tests, and I'm not looking forward to ensure that listening on some random sublist will behave properly in cases where the main list was modified (but did not include the sublist range), or when it did include only part of the sublist range (would need to transform the Change provided I think), etc...

I thought about just not allowing listeners on these sublists, but the required cast still is super annoying.

So, since ObservableList has already decided to extend the List interface with additional methods (setAll, remove(int, int)) I think doing it in the proposed way is much simpler and in line with what is already there.

[kevinrushforth]

/reviewers 2
/csr

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn please create a CSR request for issue JDK-8091429 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[hjohn]

With this modification, two lists can be kept in sync with each other with this minimal code, and with minimal notifications:

change -> {
  while(change.next()) {
    int from = change.getFrom();
    int to = change.getTo();

    if(change.wasPermutated()) {
      destination.replaceRange(from, to, change.getList().subList(from, to).stream().map(mapper).toList());
    }
    else {
      int removed = change.getRemovedSize();

      destination.replaceRange(from, from + removed, change.getList().subList(from, to).stream().map(mapper).toList());
    }
  }
};

[kevinrushforth]

I can review this.

@hjohn If you're happy with the current replaceRange name (which seems good to me), can you update the JBS and PR title to reflect this?

[hjohn]

I can review this.

@hjohn If you're happy with the current replaceRange name (which seems good to me), can you update the JBS and PR title to reflect this?

@kevinrushforth Yeah, I think the name suggested by Michael captures the intent well, and offers the most flexible option for doing large element replacements.

I've updated the JBS and PR.

[hjohn]

Looks like I'll need this change to write a test for replacing a range of elements in ListView. It seems that currently replacing a range of items will always clear the whole selection. Filed https://bugs.openjdk.org/browse/JDK-8371090 to track those problems.

[kevinrushforth]

Looks good. I left a question as to whether a couple additional tests might be useful.

[kevinrushforth]

@hjohn You can create the CSR when you are ready. I'll review it once you do.

[kevinrushforth]

A single re-review of the new tests should be sufficient.

/reviewers 1

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).