On Tue, 25 Feb 2020 15:04:05 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> The pull request has been updated with 1 additional commit.
>
> 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.
-------------
PR: https://git.openjdk.java.net/jfx/pull/114
