RFR: 8187309: TreeCell must not change tree's data

[Jeanette Winzenburg]

On Wed, 9 Feb 2022 10:51:54 GMT, Ajit Ghaisas <aghaisas at 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