On Thu, 12 Mar 2026 18:38:37 GMT, Marius Hanl <[email protected]> wrote:
>> This is an implementation for the long standing issue of allowing to commit >> a cell value when the focus is lost or the editing index (cell) changed. >> This also contains >> [JDK-8089311](https://bugs.openjdk.org/browse/JDK-8089311) (for better >> understanding the usecase with `TextField` cells, but we may split this >> later on). >> >> API >> - >> - Instead of calling `cancelEdit`, every cell now calls `stopEdit` when the >> focus is lost or the editing index changed. The default behavior is >> cancelling the edit, but developers can now override the behavior and allow >> a `commitEdit` instead >> - There are multiple 'events' that can lead to a editing change. Every >> change will now call `stopEdit`. >> It is therefore the responsibility of the developer to decide, when it makes >> sense to actually commit the value instead of cancelling it. >> This decision was made as the behavior is manipulating the editing index, >> but you as a developer can as well. Therefore, we do not always know what >> intention led to a change of the editing index. >> - Every `MOUSE_PRESSED` shifts the focus to the cell container, which is >> undesired in case of editing the cell. So this event is now consumed. The >> cell container will request focus before (if it is not yet set) >> Note: So basically, we request focus a tiny bit earlier (for commit reasons) >> instead of after, which interruppts the commit >> - All `TextField` cells now commit their value (instead of cancel) on focus >> loss >> - `TextField` Escape handling was badly implemented (it was never really >> called, as the cell container handled Escape before) >> >> Considerations >> - >> - I tried to make the API minimal, and without breaking changes (other than >> the `TextField` cells committing their values, but we may split this up) >> - Clicking the `ScrollBar` now commits/cancels the edit. I checked other >> applications and this is very common. But something I need to note here. >> This probably can be fixed in the same way mentioned above (`focusWithin`) >> - It might be hard for a developer to exactly know the cause why `stopEdit` >> is called. This does not seem like a problem, as e.g. for a `TextField`, you >> normally register listeners for e.g. focus loss for commit or pressing the >> Escape key for cancel, so you keep full control. >> >> Possible next PRs >> - >> - The Cell Container focus behavior is, well, weird right now. That is why >> consuming the event is needed to better support this PR. One thing we may >> can consider is using the `focusWithin` property instead for all 4 C... > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > make it final, improve docs Some early (and minor) code review comments. This needs more testing and consideration. Would it be possible to expand the PR description to illustrate a couple of examples wrt changes in behavior and also the kind of changes the application need to make to adapt to the new behavior? Perhaps extract it into a JEP-like markdown page written for people who have minimal knowledge of the problem? One question I must ask is whether it's possible / worth it to hide the new behavior behind a system property, for people who spent a lot of time making their apps work with the existing behavior and who are reluctant / unable to spend more time fixing it again? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/CellBehaviorBase.java line 176: > 174: latePress = isSelected(); > 175: if (!latePress) { > 176: doSelect(e, e.getX(), e.getY(), e.getButton(), > e.getClickCount(), might as well pass only the event pointer modules/javafx.controls/src/main/java/javafx/scene/control/Cell.java line 385: > 383: } > 384: > 385: cancelEdit(); this might be clearer: if (isEditing()) { cancelEdit(); } modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java line 596: > 594: * If not overwritten, will cancel the edit without changing > control'sediting state. > 595: */ > 596: private void doStopEdit() { feels like a misnomer: typically `doSomething` denotes a terminal operation, but here it calls stopEdit()... it's ok, just confusing. modules/javafx.controls/src/main/java/javafx/scene/control/TableRow.java line 329: > 327: } else if (isEditing() && !rowMatch) { > 328: stopEdit(); > 329: } I know it is in the original code, but we evaluate isEditing() twice. This might be slightly better: if (isEditing()) { if(!rowMatch) { stopEdit(); } } else { if(rowMatch) { startEdit(); } } modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java line 614: > 612: /** > 613: * Stops the edit operation. > 614: * If not overwritten, will cancel the edit without changing > control'sediting state. `control'sediting` Also, what does it mean: "without changing control's editing state" exactly? modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java line 588: > 586: /** > 587: * Stops the edit operation. > 588: * If not overwritten, will cancel the edit without changing > control'sediting state. `control'sediting` Also, what does it mean: "without changing control's editing state" exactly? ------------- PR Review: https://git.openjdk.org/jfx/pull/1935#pullrequestreview-4000198549 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982466863 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982496910 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982545858 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982569322 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982575263 PR Review Comment: https://git.openjdk.org/jfx/pull/1935#discussion_r2982588064
