8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit by kleopatra · Pull Request #638 · openjdk/jfx

kleopatra · 2021-10-05T13:18:07Z

cell startEdit is supposed to update the editing location on its associated control - was done in ListCell, not in Tree-/TableCell nor TreeCell.

Fix was to add control.edit(..). Note that ListCell was also touched to use the exact same method call pattern as the fixed cell types.

Added/unignored cell tests that are failing/passing before/after the fix.

Progress

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

Issue

JDK-8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit

Reviewers

Marius Hanl (@Maran23 - Author)
Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 638

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

Using diff file

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

cell.startEdit

bridgekeeper · 2021-10-05T13:18:15Z

👋 Welcome back fastegal! 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.

mlbridge · 2021-10-05T13:22:22Z

Webrevs

Maran23 · 2021-10-06T15:15:16Z

Interesting, I just saw that it worked before because of the TableCellBehavior (edit method). Does this mean this can be removed from the behaviour in future?

kleopatra · 2021-10-06T15:34:14Z

Interesting, I just saw that it worked before because of the TableCellBehavior (edit method). Does this mean this can be removed from the behaviour in future?

hmm .. the behavior talks directly to the control (not the cell) by invoking control.edit(...) - which might be a problem (or not, didn't look closely yet - left to later when dealing with with big big edit issue ;)

The issue here is cell.startEdit which must call the control.edit(...) to switch the control into editing, independent on other parties.

kleopatra · 2021-10-07T09:30:45Z

Interesting, I just saw that it worked before because of the TableCellBehavior (edit method). Does this mean this can be removed from the behaviour in future?

hmm .. the behavior talks directly to the control (not the cell) by invoking control.edit(...) - which might be a problem (or not, didn't look closely yet - left to later when dealing with with big big edit issue ;)

an aside I forgot to mention yesterday: this interference from behavior (triggered by mouseEvents) is one of the major stumbling-blocks in solving the ominous commit-on-focusLost issue - by cancelling an edit.

Maran23 · 2021-10-07T10:32:50Z

Interesting, I just saw that it worked before because of the TableCellBehavior (edit method). Does this mean this can be removed from the behaviour in future?

hmm .. the behavior talks directly to the control (not the cell) by invoking control.edit(...) - which might be a problem (or not, didn't look closely yet - left to later when dealing with with big big edit issue ;)

The issue here is cell.startEdit which must call the control.edit(...) to switch the control into editing, independent on other parties.

Yeah right, just want to share my findings though. :)
The whole behaviour stuff is weird anyway, also given that there is still no way do change the behaviour without accessing the (internal) api.

Maran23 · 2021-10-07T10:35:12Z

And by the way, you know why there is a requestFocus() in List/TreeCell, but not in the other two cells? :)

kleopatra · 2021-10-07T11:22:16Z

And by the way, you know why there is a requestFocus() in List/TreeCell, but not in the other two cells? :)

plain oversight? or the wrong thing to do, anyway? or having weird side-effects in tabular controls which were evaded by not requesting focus? We'll see the deeper we go in the cleanup quest :)

Maran23

Looks good, just left one minor comment. :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

and removed unused (by this fix) import in same file

Maran23 · 2021-10-21T18:50:45Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

+        setupForEditing();
+        int editingRow = 1;
+        cell.updateIndex(editingRow);
+        TablePosition<?,?> editingCell = new TablePosition<>(table, editingRow, editingColumn);


Just saw you added the space for the <?,?> in line 559 (which I didn't saw 😄) but not here :)

darn .. ;) Thanks

hmm .. I'm all for consistency, so don't mind trying again but ... what is the formatting rule? Searching in controls:

wildcard search: <?,?> = 1000+ vs. <?, ?> = 379

verbatim: <\?,\?> = 173 vs. <\?, \?> = 98

Looks .. inconclusive .. 😁?

But then, generics tutorial has the space - so the space it will be.

Weird. Also standard classes like HashMap, AbstractMap or just Map doesn't have the space. I'm also for consisteny, but don't know which would be 'the best' 😄

Maran23

Looks good. 👍

openjdk · 2021-10-29T10:17:46Z

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

8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit

Reviewed-by: mhanl, 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 10 new commits pushed to the master branch:

a947405: 8271091: Missing API docs in UI controls classes
e7a106f: 8271090: Missing API docs in scenegraph classes
717cfdc: 8274669: Dialog sometimes ignores max height
d244b30: 8275815: OCA link in README.md and CONTRIBUTING.md is broken
a118d33: 8236689: macOS 10.15 Catalina: LCD text renders badly
d6f78e2: 8275138: WebView: UserAgent string is empty for first request
b591912: 8274137: TableView scrollbar/header misaligned when reloading data
bb73d43: 8272870: Add convenience factory methods for border and background

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

kleopatra · 2021-10-29T12:27:33Z

/integrate

openjdk · 2021-10-29T12:28:46Z

Going to push as commit adcc40d.
Since your change was applied there have been 10 commits pushed to the master branch:

a947405: 8271091: Missing API docs in UI controls classes
e7a106f: 8271090: Missing API docs in scenegraph classes
717cfdc: 8274669: Dialog sometimes ignores max height
d244b30: 8275815: OCA link in README.md and CONTRIBUTING.md is broken
a118d33: 8236689: macOS 10.15 Catalina: LCD text renders badly
d6f78e2: 8275138: WebView: UserAgent string is empty for first request
b591912: 8274137: TableView scrollbar/header misaligned when reloading data
bb73d43: 8272870: Add convenience factory methods for border and background

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-29T12:29:02Z

@kleopatra Pushed as commit adcc40d.

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

8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in

050a6ca

cell.startEdit

openjdk bot added the rfr Ready for review label Oct 5, 2021

kevinrushforth requested a review from aghaisas October 12, 2021 13:35

Maran23 approved these changes Oct 13, 2021

View reviewed changes

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java Outdated Show resolved Hide resolved

fixed formatting as suggested in review

4f76e1f

and removed unused (by this fix) import in same file

Maran23 reviewed Oct 21, 2021

View reviewed changes

next try with fixing the formatting

4fd55ff

Maran23 approved these changes Oct 25, 2021

View reviewed changes

aghaisas approved these changes Oct 29, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 29, 2021

openjdk bot closed this Oct 29, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Oct 29, 2021

kleopatra deleted the bug-fix-8187474 branch October 29, 2021 12:29

Conversation

kleopatra commented Oct 5, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 5, 2021

Uh oh!

mlbridge bot commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Maran23 commented Oct 6, 2021

Uh oh!

kleopatra commented Oct 6, 2021

Uh oh!

kleopatra commented Oct 7, 2021

Uh oh!

Maran23 commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maran23 commented Oct 7, 2021

Uh oh!

kleopatra commented Oct 7, 2021

Uh oh!

Maran23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Maran23 Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

kleopatra Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

kleopatra Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maran23 Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

Maran23 left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 29, 2021

Uh oh!

kleopatra commented Oct 29, 2021

Uh oh!

openjdk bot commented Oct 29, 2021

Uh oh!

openjdk bot commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

kleopatra commented Oct 5, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 5, 2021 •

edited

Loading

Maran23 commented Oct 7, 2021 •

edited

Loading

kleopatra Oct 22, 2021 •

edited

Loading