Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264127: ListCell editing status is true, when index changes while editing #441

Conversation

@FlorianKirmaier
Copy link
Member

@FlorianKirmaier FlorianKirmaier commented Mar 24, 2021

Fixing ListCell editing status is true, when index changes while editing.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264127: ListCell editing status is true, when index changes while editing

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/441/head:pull/441
$ git checkout pull/441

Update a local copy of the PR:
$ git checkout pull/441
$ git pull https://git.openjdk.java.net/jfx pull/441/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 441

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/441.diff

Fixing ListCell editing status is true, when index changes while editing
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 24, 2021

👋 Welcome back fkirmaier! 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 openjdk bot added the rfr label Mar 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Mar 25, 2021

good catch, yet another property that's not correctly updated on index change :)

Quick questions:

  • what are the macroscopic effects (that is in a running list) that you see without fixing it?
  • do we want mere cell re-use fire a editCancel? (which the list seems to ignore in the test, don't know why)

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 25, 2021

To clarify, this with happening with ListViews in production code.
In some conditions, the status of a single cell doesn't update. The effect is, that a Cell is stuck in the Editing mode.
If I remember correctly, it was irreversible stuck. It happened rarely, it might be connected to cells with variable sizes.

Handling the logic from the ListView seems wrong to me, I think the ListCell should update its state automatically dependent of the properties. But I wouldn't know where to add it in the ListView. If that's how it's done in the other cell-based components, then it would make sense and the code more linear. I'm not really opinionated on how to solve it, just went for the simplest fix I've found.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Mar 26, 2021

Handling the logic from the ListView seems wrong to me,

looks like I was unclear, because that's a 100% me-too :)

Reformulating my second sentence in test snippets:

 A:
 cell.updateIndex(1);
 list.edit(1);
 cell.updateIndex(0);
 // failing/passing before/after fix
 assertFalse("cell re-use must update cell editing state" , cell.isEditing());

 B:
List<EditEvent> events = new ArrayList<EditEvent>();
list.setOnEditCancel(e -> {
    events.add(e);
});
 .... setup test state as in A
// passing/failing before/after fix
assertTrue("cell re-use must not trigger edit events", events.isEmpty()); 

C:
 .... setup test state as in A
 // passing/passing before/after fix     
 assertEquals("cell re-use must not change list editing state", 1, list.getEditingIndex);

My question was, whether we agree on B. My wondering was about C passing in the light of B failing.

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Mar 31, 2021

I don't think B is right.
I would expect a editCancel event when the index of an editing cell is changed.
If there would be another cell, which will get the index 1 (which isn't the case in this artifical test) then i would expect another editStart event.

On the perspective of the ListView, the index never changes, but it's more relevant that some of the cells cancel/start the editing. If one would want to avoid the cancelEdit, when the index is not changing, then the solution should be to make sure, the index of editing cells never changes, which isn't the scope of this PR.

@kevinrushforth kevinrushforth requested a review from aghaisas Mar 31, 2021
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 1, 2021

I don't think B is right.
I would expect a editCancel event when the index of an editing cell is changed.
If there would be another cell, which will get the index 1 (which isn't the case in this artifical test) then i would expect another editStart event.

well, I see your point but think it's arguable :)

  • editEvent not/firing on cell re-use is not really specified (even overall edit spec is .. suboptimal, see my oldish summary) - without that spec, we might feel free to fire
  • from the perspective of edit handlers, that editCancel (and the assumed editStart - suspect it doesn't happen, didn't test though) are just spurious events - they are not really interested in volatile state changes, just introduced by an implementation detail like cell re-use.

As to the scope of this: as I understand it, its goal is to keep the cell's editing state (flag and visual state) in sync with listview's editingIndex when updating the cell index. That should include both directions

  • index == editingIndex -> index != editingIndex
  • index != editingIndex -> index == editingIndex

The first is already handled (modulo our disagreement on editEvents :), the second is not.

Added another test for ListCell, now when updating the index of the ListCell to the editingIndex
removed accidentally commited line
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 3, 2021

@kleopatra
I've added another test for the case, when index changes in such a way, that the cell should no longer be in the editing state,
and the test seems to work. Do I miss something?

About the Events, when i think about it, I was a bit of biased so I don't have to change anything. So I don't really have an opinion about it. But the PR should only fix the case, which is obviously wrong, and i don't really want to change anything in the event logic.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 6, 2021

@kleopatra
I've added another test for the case, when index changes in such a way, that the cell should no longer be in the editing state,
and the test seems to work. Do I miss something?

hmm .. that's weird: your test (cell 1 -> listEdit 0 -> cell 0) indeed passes, doing it the other way round (cell 0 -> listEdit 1 -> cell 1) fails

@Test
public void testChangeCellIndex0ToListEditingIndex1() {
    assertChangeIndexToEditing(0, 1);
}

@Test
public void testChangeCellIndex1ToListEditingIndex0() {
    assertChangeIndexToEditing(1, 0);
}

private void assertChangeIndexToEditing(int initialCellIndex, int listEditingIndex) {
    list.setEditable(true);
    cell.updateListView(list);
    cell.updateIndex(initialCellIndex);
    list.edit(listEditingIndex);
    assertEquals("sanity: list editingIndex ", listEditingIndex, list.getEditingIndex());
    assertFalse("sanity: cell must not be editing", cell.isEditing());
    cell.updateIndex(listEditingIndex);
    assertEquals("sanity: index updated ", listEditingIndex, cell.getIndex());
    assertEquals("list editingIndex unchanged by cell", listEditingIndex, list.getEditingIndex());
    assertTrue(cell.isEditing());
}

Something obvious that I missed or a bummer in the test setup or what else? Any ideas?

@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 9, 2021

I've looked into it.
This is somehow linked to the fact, that the default focusIndex for the list is 0 instead of -1.
If you change the updateDefaultFocus methods in ListView, then it works.

The tests are somehow synthetic and don't reflect the real world.
So it's probably ok to leave it as it is. A better test which reflects the real world would be better, but that's probably way harder.
Then we would have to get the ListCells from the ListView ... Which would basically be reasonable for all of these tests, but i don't think it should be done as part of this PR.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 10, 2021

I've looked into it.
This is somehow linked to the fact, that the default focusIndex for the list is 0 instead of -1.
If you change the updateDefaultFocus methods in ListView, then it works.

good dig :) Wouldn't have expected the focus state to have any effect .. so we should make certain it's part of the test setup, f.i. by always setting the focusedIndex (of the model) to -1 (or whatever is required to make all pass) always and document why it's done.

The tests are somehow synthetic and don't reflect the real world.

.. true, nevertheless they souldn't fail/pass on some magic number ;)

And yet another failing test, when changing cell index from -1 -> listEditingIndex:

@Test
public void testChangeCellIndexMinusOneToListEditingIndex0() {
    assertChangeIndexToEditing(-1, 1);
}

This looks not fixable by pre-setting focusIndex ..

Coming back to our disagreement about event firing: I'm still weary with those intermediate events from simple re-use - but found a precedence in TableCell: the same broken cell editing state led to data corruption and was fixed by calling updateEditing (including firing). So doing the same here looks okay after all. Should be documented though, at least by tests (for cancel, B in my very first comment with flipped assertion, similar for start edit).

Fixed some more testcases
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 14, 2021

I've updated the Fix and the tests.
It contains now the 3 cases you've mentioned and all are passing.
The test from 0 works with setting the initial focus.
The test from -1 works, after updating how the fix works.
Now the index is updated, after updating the item.

Added checks, whether the correction ammount of editStart/cancel events are triggered
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 14, 2021

I've now expanded the tests, to check for the correct number of events, similar to your case B - just with reverted conditions, like you've suggested.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 15, 2021

  • there's still a failing transition (and not covered by the latest test ;) cellIndex == editingIndex to cellIndex == -1
  • as to your fix: don't see a difference between calling updateEditing before/after updateFocus - simply moving it after the if block seems to solve all toEditing issues

That said: I'm aware that getting this right is tedious and a bit hard, not because it's rocket science but because we need to cover the corner cases as completely as possible (and often fail in doing so, me included :) Chances are that if we detect a problem with a transition "-1 -> 1", there's a similar problem in the invers "1 -> -1". Even if there isn't, we should make sure there isn't by adding a test. That's why I personally prefer writing dedicated parameterized tests (vs. adding to the existing general purpose test), here in cell/editingIndex. Don't know if you noticed, yesterday I attached such a test to the issue (had to write it for JDK-8265206 anyway, so no big deal to replace TableCell with ListCell) - whether you take it or adjust yours accordingly doesn't matter, as long as the coverage is complete.

BTW, in TableCell the remaining issue (editingIndex -> -1) seems to be related to updateEditing backing out if the current index is -1. It probably should cleanup if in editing state (didn't try yet).

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 15, 2021

curious: do you understand the dependency of update sequence (updateEditing relative to updateFocus)?

What I see is

  • focusIndex == cellIndex and calling updateEditing before updateFocus - all toEditing transitions are failing due to corrupting list editing state (its editingIndex seems to be changed to -1, that is effectively canceling the edit)
  • focusIndex == editingIndex and calling updateEditing after updateFocus - all offEditing transitions are failing due to corrupting list editing state

TableCell does the latter and has no dependency on focus state - at least none I can see ;)

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 16, 2021

curious: do you understand the dependency of update sequence (updateEditing relative to updateFocus)?

[... ]

* `focusIndex == editingIndex` and calling updateEditing after updateFocus - all offEditing transitions are failing due to corrupting list editing state

TableCell does the latter and has no dependency on focus state - at least none I can see ;)

actually, TableCell behavior only appears to not depend on focus state .. because the cell is never moved into focused state if its TableRow is null ;) With a setup including updateTableRow, its focused state is updated to false when moving off the editing row which triggers the listener in cell which cancels the edit ... so we basically have the same behavior as in ListCell.

To keep the tests focused on editing (vs a mix-in of other properties), we should probably (?) setup focus to -1 always.

Small update to the tests and the fix
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 21, 2021

Finally found time to update the PR.

  1. As suggested, I've moved the updateIndex method below the if.
  2. The test from 1 to -1 is a bit more complicated. Because -1 represents "not editing" the test would be differnt. I've adapted the test to check for "not editing" in the case of -1. With this change the test is green.

Honestly im not really aware of the Ordering of the methods. I didn't spend a lot of time understanding the inner works of all the controls - im usually more focused on fixing memory leaks.

About the initial focus, i see 3 options:

  • set it to -1 in testChangeIndexToEditing1_jdk_8264127
  • set it to -1 in assertChangeIndexToEditing
  • set it to -1 in the setup method.
    All 3 options work. Im fine with keeping it at the concrete test, because it's the only test who needs it, but im fine with any of the 3 options.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 21, 2021

Finally found time to update the PR.

1. As suggested, I've moved the updateIndex method below the if.

looks good :)

2. The test from 1 to -1 is a bit more complicated. Because -1 represents "not editing" the test would be differnt. I've adapted the test to check for "not editing" in the case of -1. With this change the test is green.

looks like I haven't been clear enough in my recent comments: the list is always really editing (state transitions of list's editingIndex and their effects on a cell at that index seem to be working and have tests), that is editingIndex > 0. What I meant is updating the cell index from that editingIndex to -1, here is a test that's still failing

@Test
public void testUpdateCellIndexOffEditing() {
    list.setEditable(true);
    cell.updateListView(list);
    int editingIndex = 1; // list editing
    cell.updateIndex(editingIndex);
    list.edit(editingIndex); 
    // here we are certain that the cell is in editing state
    assertTrue("sanity: cell must be editing", cell.isEditing());
    cell.updateIndex(-1); // change cell index to negative
    assertFalse("cell must not be editing if cell index is " + cell.getIndex(), cell.isEditing());
}

For a fix, have a look at JDK-8264127 - I think it's similar for ListCell updateEditing, though its logic block clouds it a bit (instead of immediately backing out for cellIndex == -1, it doesn't enter the if.

Honestly im not really aware of the Ordering of the methods. I didn't spend a lot of time understanding the inner works of all the controls - im usually more focused on fixing memory leaks.

About the initial focus, i see 3 options:

* set it to -1 in testChangeIndexToEditing1_jdk_8264127

* set it to -1 in assertChangeIndexToEditing

* set it to -1 in the setup method.
  All 3 options work. Im fine with keeping it at the concrete test, because it's the only test who needs it, but im fine with any of the 3 options.

if you intend to keep the tests in the big-fat ListCellTest, then option 3 would be a bit brittle, IMO: it might lead to unexpected behavior in the other/future tests. Keeping focus on -1 should be done in each of the tests here (otherwise changes in default focus might - or not - break the tests), so I would suggest option 2 and in all whileEditing methods.

Fixed another index case based on code review
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented Apr 22, 2021

I've fixed the case similar as the fix in the TreeCell!

Option 2 for Keeping focus on -1 is also implemented (seems like it was done in the previous commit)

Copy link
Collaborator

@kleopatra kleopatra left a comment

Fix and tests are not yet quite complete - see my inline comments :)

A general suggestion for the tests: I would prefer to have them consistent in both toEditing and offEditing (or whileEditing)

The transitions in the direction cell-not-editing to cell-editing are factored into a common method assertChangeIndexToEditing(...) and have test methods calling that common method with varied concrete indices (f.i 0 ->1, -1 -> 1 etc). The other way round - cell-editing to cell-not-editing - should be factored in a similar way, with the same index pairs. The advantages, IMO: guaranteed same setup and test completeness for both directions and easy read of the variations

final boolean editing = isEditing();

// Check that the list is specified, and my index is not -1
if (index != -1 && list != null) {
if (index == -1 || list == null) {
if(isEditing()) {
cancelEdit();
}
} else {
Copy link
Collaborator

@kleopatra kleopatra Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: not a big fan of local fields, but .. if there already is a local field, we should use it instead of calling the method again (here: editing vs. isEditing)

minor: the code comment does no longer fit the code

major: this will change the editing state of the list - the call to cancelEdit must be wrapped into un/setting the updateEditingIndex flag (see the else-if block later in the method)

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

List<EditEvent> events = new ArrayList<EditEvent>();
list.setOnEditCancel(e -> {
events.add(e);
});
list.setEditable(true);
Copy link
Collaborator

@kleopatra kleopatra Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing setup of focus to -1

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this test in favor of the test-method below.

public void testChangeIndexToEditing3_jdk_8264127() {
assertChangeIndexToEditing(1, -1);
Copy link
Collaborator

@kleopatra kleopatra Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm .. don't quite understand this: the list's editing state shouldn't be changed and here it is never editing. What's the intention?

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't make sense. Probably missunderstanding somewhere. Should i remove? This would also get rid of the if in the test.

Copy link
Collaborator

@kleopatra kleopatra Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, remove it: the focus of this issue is that the cell sync's its own editing state on updateIndex off/to list editing index - I think that implies the list always editing with a valid editingIndex :)

Copy link
Collaborator

@kleopatra kleopatra Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, remove it: the focus of this issue is that the cell sync's its own editing state on updateIndex off/to list editing index - I think that implies the list always editing with a valid editingIndex :)

*grrr - why was this not added to my review? Really don't understand this tool .. seems to hate me ..

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's added 3 times. : P The if is now removed.

cell.updateIndex(-1); // change cell index to negative
assertFalse("cell must not be editing if cell index is " + cell.getIndex(), cell.isEditing());
Copy link
Collaborator

@kleopatra kleopatra Apr 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing: assert that a cancelEvent is fired and the list editing state is unchanged (they fail until updateEditing is fixed)

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check of cancel-events is now added.

More changes based on codereview.
Simplified the tests, we are now using for both cases "test-factory-methods"
updateSelection();
updateFocus();
}
updateEditing();
}
Copy link
Collaborator

@kleopatra kleopatra May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot yesterday: to keep consistent with TreeTableCell (and TreeCell), the updateEditing should be moved into the else-block (as last call) - couldn't find any difference (in fact couldn't reproduce the error the if/else is supposed to solve) as long as updateEditing behaves, though.

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to the if-else. Probably doesn't matter.

More changes, simplifications and tests based on CR
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented May 14, 2021

I've added a test for the try-catch-finally block, based on the test in the TreeTableCell. (for some reason i can't comment it in the CR)

Copy link
Collaborator

@kleopatra kleopatra left a comment

fix looks okay, commented the added test method inline.

BTW: I assume you will fix the trailing whitespace error in your next commit :)

try {
list.edit(intermediate);
} catch (Exception ex) {
// just catching to test in finally
} finally {
assertFalse("cell must not be editing", cell.isEditing());
assertEquals("table must be editing at intermediate index", intermediate, list.getEditingIndex());
}
Copy link
Collaborator

@kleopatra kleopatra May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's good - except for a minor omission: the catch-block is never reached, instead it is printed to sysout (or syserr, don't nail me) That happens because ExpressionHelper kind of swallows all exceptions in change notifications by passing them to the uncaughtExceptionHandler. To fix this, a test should un-/install (in After/Before methods) an uncaught exception handler. For examples, see f.i. TableCellTest.

really minor: replace "table" by "list" in the failure message :)

Copy link
Member Author

@FlorianKirmaier FlorianKirmaier May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added the exception handler, like in TreeCellTest.

And I've fixed the texts of the failure messages.

fixed trailing whitespace
fixed assertion text,
exception handler is now installed/uninstalled like in TableCellTest
@openjdk openjdk bot added the rfr label May 18, 2021
Copy link
Collaborator

@kleopatra kleopatra left a comment

fix and tests look good :)

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented May 18, 2021

wondering why the bot removed the rfr label (about three weeks ago) and re-added it a earlier today?

Copy link
Collaborator

@aghaisas aghaisas left a comment

Fix and tests look good to me.
I have listed minor editing comments.

small changes based on CR
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented May 20, 2021

I've done the 3 requested minor code convention changes

@openjdk
Copy link

@openjdk openjdk bot commented May 20, 2021

@FlorianKirmaier 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:

8264127: ListCell editing status is true, when index changes while editing

Reviewed-by: fastegal, aghaisas

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 67 new commits pushed to the master branch:

  • 5843910: 8267392: ENTER key press on editable TableView throws NPE
  • 111bac4: 8263760: Update gradle to version 7.0.1
  • 485b242: 8264219: tools/scripts/build.ps1 is out of date and no longer works
  • e76b7b1: 8186904: TableColumnHeader: resize cursor lost on right click
  • 4619cdd: 8264138: Replace uses of Class.newInstance
  • 93de584: 8266966: Wrong CSS properties are applied to other nodes after fix for JDK-8204568
  • c511789: 8266860: [macos] Incorrect duration reported for HLS live streams
  • d2d145d: 8266643: Intermittent failure of HonorDeveloperSettingsTest unit test
  • 9c97d9b: 8267121: Illegal access to private "size" field of ArrayList from build.gradle
  • f236a7d: 8266539: [TreeView]: Change.getRemoved() contains null item when deselecting a TreeItem
  • ... and 57 more: https://git.openjdk.java.net/jfx/compare/3bbcf9779f08090be75413350c8b2ca31d4bef35...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kleopatra, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 20, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 20, 2021

This one should have been set to require 2 reviewers. I'll do it now, but even if Skara doesn't remove the ready label, please wait to integrate it until @kleopatra re-reviews it.

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented May 20, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot removed the ready label May 20, 2021
@openjdk openjdk bot added the ready label May 21, 2021
@FlorianKirmaier
Copy link
Member Author

@FlorianKirmaier FlorianKirmaier commented May 21, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 21, 2021

@FlorianKirmaier
Your change (at version 7c22f03) is now ready to be sponsored by a Committer.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented May 22, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented May 22, 2021

@kleopatra @FlorianKirmaier Since your change was applied there have been 67 commits pushed to the master branch:

  • 5843910: 8267392: ENTER key press on editable TableView throws NPE
  • 111bac4: 8263760: Update gradle to version 7.0.1
  • 485b242: 8264219: tools/scripts/build.ps1 is out of date and no longer works
  • e76b7b1: 8186904: TableColumnHeader: resize cursor lost on right click
  • 4619cdd: 8264138: Replace uses of Class.newInstance
  • 93de584: 8266966: Wrong CSS properties are applied to other nodes after fix for JDK-8204568
  • c511789: 8266860: [macos] Incorrect duration reported for HLS live streams
  • d2d145d: 8266643: Intermittent failure of HonorDeveloperSettingsTest unit test
  • 9c97d9b: 8267121: Illegal access to private "size" field of ArrayList from build.gradle
  • f236a7d: 8266539: [TreeView]: Change.getRemoved() contains null item when deselecting a TreeItem
  • ... and 57 more: https://git.openjdk.java.net/jfx/compare/3bbcf9779f08090be75413350c8b2ca31d4bef35...master

Your commit was automatically rebased without conflicts.

Pushed as commit 240d28f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants