On Wed, 9 Feb 2022 10:51:54 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
>> Issue was TreeView commit editing implementation violated the spec'ed >> mechanism: >> >> - no default commit handler on TreeView >> - TreeCell modifying the data directly >> >> Fix is to move the saving of the edited value from cell into a default >> commit handler in tree and set that handler in the constructor. >> >> Added tests that failed/passed before/after the fix (along with a sanity >> test for default commit that passed also before). Also fixed a test bug >> (incorrect registration of custom commit handler, see >> [JDK-8280951)](https://bugs.openjdk.java.net/browse/JDK-8280951) in >> TreeViewTest.test_rt_29650 to keep it passing. > > modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java line > 341: > >> 339: setFocusModel(new TreeViewFocusModel<T>(this)); >> 340: >> 341: setOnEditCommit(DEFAULT_EDIT_COMMIT_HANDLER); > > You are adding the default edit commit handler to TreeView. Although it seems > to be correct, this default handler is likely to be overwritten if the user > code already has a setOnEditCommit() call. This is shown by the > test_rt_29650() failure in TreeviewTest.java which you have corrected. > > The changes done in this PR makes TreeView similar to ListView and TableView > in terms of having the default edit commit handler. > > Unfortunately, I do not see how can we avoid user code accidentally > overwriting the default edit commit handler. Documenting this possibility > seems to be the only way ahead. > Any other idea? Yes, the change might break application code, though that code would be buggy. Actually, the behavior as implemented now, _already is_ documented :) > It is very important to note that if you call > setOnEditCommit(javafx.event.EventHandler) with your own EventHandler, then > you will be removing the default handler. Unless you then handle the > writeback to the property (or the relevant data source), nothing will happen. so: if they have a custom handler that doesn't save the data, they were violating the specification (though were getting away with it due to the bug). Personally, I think that we cannot keep backward compatibility to bugs. ------------- PR: https://git.openjdk.java.net/jfx/pull/724
