On Tue, 25 Feb 2020 15:10:50 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewKeyInputTest.java
>> line 1141:
>>
>>> 1140: keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
>>> 1141: }
>>> 1142: keyboard.doKeyPress(KeyCode.SPACE,
>>
>> having such if blocks in all tests regarding horizontal navigation feels
>> like the parametrization isn't quite complete, IMO: the test method
>> shouldn't need to worry, instead just call semantic horizontal navigation
>> methods.
>>
>> A simple change would be to extract that differentiation into a dedicated
>> method, like f.i. forward/backward, test would get simpler (and make it
>> easier to add those that are missing :)
>>
>> @Test
>> public void testForwardFocus() {
>> sm.setCellSelectionEnabled(true);
>> sm.select(0, col0);
>> // current
>> //if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
>> // keyboard.doRightArrowPress(KeyModifier.getShortcutKey());
>> //} else {
>> // keyboard.doLeftArrowPress(KeyModifier.getShortcutKey());
>> //}
>> // extracted
>> forward(KeyModifier.getShortcutKey());
>> assertTrue("selected cell must still be selected", sm.isSelected(0,
>> col0));
>> assertFalse("next cell must not be selected", sm.isSelected(0,
>> col1));
>> TablePosition focusedCell = fm.getFocusedCell();
>> assertEquals("focused cell must moved to next", col1,
>> focusedCell.getTableColumn());
>> }
>>
>> /**
>> * Orientation-aware horizontal navigation with arrow keys.
>> * @param modifiers the modifiers to use on keyboard
>> */
>> protected void forward(KeyModifier... modifiers) {
>> if (orientation == NodeOrientation.LEFT_TO_RIGHT) {
>> keyboard.doRightArrowPress(modifiers);
>> } else {
>> keyboard.doLeftArrowPress(modifiers);
>> }
>> }
>
> Also, I'm a bit weary about the "else if" (vs a simple "else") - wouldn't it
> be some kind of setup error if the node orientation is neither rtl nor ltr?
> If so, I would add a test to check for it once.
Initially I had thought about adding separate test file for RTL - something
like RTLTableViewKeyInputTest - but, realized that although it's a cleaner
approach, we would simply duplicate the tests. Also, the fact is only
LEFT/RIGHT key navigation is sensitive to NodeOrientation - hence only a subset
of tests needed modification. This is the reason I have parameterized the test.
To your specific question, since it is a parameterized test, only possible
values are LTR and RTL which are specified as @Parameterized.Parameters. I
don't think, we need additional check for some other value.
-------------
PR: https://git.openjdk.java.net/jfx/pull/114
