On Wed, 9 Sep 2020 11:49:55 GMT, Jeanette Winzenburg <[email protected]>
wrote:
>> Ambarish Rapte has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> rename tests
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java
> line 87:
>
>> 85: Predicate<KeyEvent> pIsInComboBox = e -> isListViewOfComboBox !=
>> null;
>> 86: Predicate<KeyEvent> pIsInEditableComboBox =
>> 87: e -> isListViewOfComboBox != null &&
>> isListViewOfComboBox.get();
>
> nit-pick about naming: I think we don't use (crippled) hungarian notation, or
> do we? If we don't, the leading "p"
> should be removed, isIn/Editable/Combo is expressive enough (and no need to
> differentiate by type of functional
> interface, IMO)
Even I was bit skeptical about prefix p, before it those names sounded like a
boolean. So I wanted to give it a
different name. But as you said, isIn/Editable/Combo looks enough expressive.
Changed names.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java
> line 509:
>
>> 507: getProperties().put("selectFirstRowByDefault", false);
>> 508: getProperties().put("editableComboBoxEditor",
>> (Supplier<Boolean>) () ->
>> getSkinnable().isEditable()); 509: }
>
> nit-pick about naming: it's the editable state of the combo (vs. the editable
> state of the editor) that's in the center
> of interest, so maybe rename the key to express that? Like "editableComboBox"?
> Another thingy (which I think is a bit under-used in the fx code-base;) - how
> about a code comment to why this marker
> is added and which collaborator is using it?
Changed to `editableComboBox` and added a short two liner comment about it.
-------------
PR: https://git.openjdk.java.net/jfx/pull/172
