Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v3]
On Mon, 7 Feb 2022 18:46:55 GMT, Jose Pereda wrote: >> This PR adds a predicate to TableView and TreeTableView selection models >> order to remove rows from the selection only when there are no selected >> cells in that given row, when cell selection is enabled. >> >> Two tests have been added as well, that fail without this PR and pass with >> it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v3]
On Mon, 7 Feb 2022 18:46:55 GMT, Jose Pereda wrote: >> This PR adds a predicate to TableView and TreeTableView selection models >> order to remove rows from the selection only when there are no selected >> cells in that given row, when cell selection is enabled. >> >> Two tests have been added as well, that fail without this PR and pass with >> it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v3]
> This PR adds a predicate to TableView and TreeTableView selection models > order to remove rows from the selection only when there are no selected cells > in that given row, when cell selection is enabled. > > Two tests have been added as well, that fail without this PR and pass with it. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewer - Changes: - all: https://git.openjdk.java.net/jfx/pull/709/files - new: https://git.openjdk.java.net/jfx/pull/709/files/fe943d4b..8e96bb3d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=709=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=709=01-02 Stats: 9 lines in 3 files changed: 1 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/709.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/709/head:pull/709 PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v2]
On Mon, 7 Feb 2022 10:24:01 GMT, Jose Pereda wrote: >> This PR adds a predicate to TableView and TreeTableView selection models >> order to remove rows from the selection only when there are no selected >> cells in that given row, when cell selection is enabled. >> >> Two tests have been added as well, that fail without this PR and pass with >> it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Address feedback from reviewer getRow () returns an int. It is more efficient to process the primitive as it is. The code below will be less boxing. Because filter and distinct are processes for primitives It can be done at high speed. ``` java final List removed = c.getRemoved().stream() .mapToInt(TablePositionBase::getRow) .distinct() .filter(removeRowFilter) .boxed() .peek(sm.selectedIndices::clear) .collect(Collectors.toList()); final int addedSize = (int)c.getAddedSubList().stream() .mapToInt(TablePositionBase::getRow) .distinct() .boxed() .peek(sm.selectedIndices::set) .count(); - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v2]
On Fri, 4 Feb 2022 17:33:18 GMT, Michael Strauß wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address feedback from reviewer > > modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java > line 172: > >> 170: .map(TablePositionBase::getRow) >> 171: .filter(removeRowFilter) >> 172: .distinct() > > Maybe `distinct` should be applied before `filter`. This can cut down the > number of times the predicate is invoked (which iterates over all selected > cells, so it may be a performance issue for large selections). Makes sense. Done - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected [v2]
> This PR adds a predicate to TableView and TreeTableView selection models > order to remove rows from the selection only when there are no selected cells > in that given row, when cell selection is enabled. > > Two tests have been added as well, that fail without this PR and pass with it. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Address feedback from reviewer - Changes: - all: https://git.openjdk.java.net/jfx/pull/709/files - new: https://git.openjdk.java.net/jfx/pull/709/files/34491051..fe943d4b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=709=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=709=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/709.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/709/head:pull/709 PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected
On Sat, 5 Feb 2022 05:08:54 GMT, yosbits wrote:
>> This PR adds a predicate to TableView and TreeTableView selection models
>> order to remove rows from the selection only when there are no selected
>> cells in that given row, when cell selection is enabled.
>>
>> Two tests have been added as well, that fail without this PR and pass with
>> it.
>
> Why not use IntPredicate?
>
> before
>
> ``` java
> public static void updateSelectedIndices(MultipleSelectionModelBase sm,
> ListChangeListener.Change> c,
> Predicate removeRowFilter) {
>
>
> after
>
> ``` java
> public static void updateSelectedIndices(MultipleSelectionModelBase
> sm,
> ListChangeListener.Change> c,
> IntPredicate removeRowFilter) {
>
>
> before
> ``` java
> .map(TablePositionBase::getRow)
>
>
> after
> ``` java
> .mapToInt(TablePositionBase::getRow)
@yososs Do you see any possible gain by using `IntPredicate` vs
`Predicate?
It changes the `Stream` to `IntStream`, and that needs an extra
`.boxed()` operation (or alternatively an extra operation to transform the int
array with `.collect(ArrayList::new, ArrayList::add, ArrayList::addAll)`).
So I'm wondering if this is worthy?
-
PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected
On Fri, 7 Jan 2022 19:36:45 GMT, Jose Pereda wrote:
> This PR adds a predicate to TableView and TreeTableView selection models
> order to remove rows from the selection only when there are no selected cells
> in that given row, when cell selection is enabled.
>
> Two tests have been added as well, that fail without this PR and pass with it.
Why not use IntPredicate?
before
``` java
public static void updateSelectedIndices(MultipleSelectionModelBase sm,
ListChangeListener.Change> c,
Predicate removeRowFilter) {
after
``` java
public static void updateSelectedIndices(MultipleSelectionModelBase sm,
ListChangeListener.Change> c,
IntPredicate removeRowFilter) {
before
``` java
.map(TablePositionBase::getRow)
after
``` java
.mapToInt(TablePositionBase::getRow)
-
PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected
On Fri, 7 Jan 2022 19:36:45 GMT, Jose Pereda wrote: > This PR adds a predicate to TableView and TreeTableView selection models > order to remove rows from the selection only when there are no selected cells > in that given row, when cell selection is enabled. > > Two tests have been added as well, that fail without this PR and pass with it. modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 172: > 170: .map(TablePositionBase::getRow) > 171: .filter(removeRowFilter) > 172: .distinct() Maybe `distinct` should be applied before `filter`. This can cut down the number of times the predicate is invoked (which iterates over all selected cells, so it may be a performance issue for large selections). - PR: https://git.openjdk.java.net/jfx/pull/709
Re: RFR: 8273336: Clicking a selected cell from a group of selected cells in a TableView clears the selected items list but remains selected
On Fri, 7 Jan 2022 19:36:45 GMT, Jose Pereda wrote: > This PR adds a predicate to TableView and TreeTableView selection models > order to remove rows from the selection only when there are no selected cells > in that given row, when cell selection is enabled. > > Two tests have been added as well, that fail without this PR and pass with it. This looks good to me. @mstr2, I see that you have some made some suggestions on JBS. It would be great if you can review this PR. - PR: https://git.openjdk.java.net/jfx/pull/709
